From ebe57dfac756e9f0e83e6320f555908a877a3884 Mon Sep 17 00:00:00 2001 From: Jared Hirsch Date: Mon, 10 Apr 2017 16:19:07 -0700 Subject: [PATCH] Webextension review changes (#2591) * Add strict mode statement to all files Addresses review comment https://github.com/mozilla-services/screenshots/pull/2471#discussion_r107981601 * Do not assign properties to `window`. Addresses review comment https://github.com/mozilla-services/screenshots/pull/2471#discussion_r107979170 * Use docElement.outerHTML, not DOMParser, to insert html page into iframe Addresses comments 2 and 3 inside selector/ui.js * Use for..of instead of for-loop Addresses selector/documentMetadata.js comment 1 https://github.com/mozilla-services/screenshots/pull/2471#discussion_r107981070 * Replace IIFE with block scope, yay es6 Addresses selector/shooter.js comment 1 https://github.com/mozilla-services/screenshots/pull/2471#discussion_r107981298 * Use spread/rest operators to simplify assert() fn logic Addresses shared/shot.js comment 1 https://github.com/mozilla-services/screenshots/pull/2471#discussion_r109268966 * WIP fixes to shared/shot * Nit - replace let..in with let..of * Nit: replace foo+"" with String(foo) * nits related to catcher.js * auth updates * review changes to main.js - add onActivated listener to ensure the button state is properly toggled when switching between tabs * selectorLoader changes TODO: investigate executeScript return value... * last few review nits * Revert "Use docElement.outerHTML, not DOMParser, to insert html page into iframe" This reverts commit 224927a0b072d2bfb851e6b45ac0ed367ab6bb93. * Use innerHTML: outerHTML throws a NoModificationAllowedError * Please the linter * Fix #2596, address review nit in randomString.js * Remove space in deviceId generation * Restore lastError handler for contextMenus.create * Rename shot.js makeUuid to more accurate makeRandomId * Fix #2603, abstract out sendToBootstrap error checking --- addon/webextension/background/analytics.js | 8 +++-- addon/webextension/background/auth.js | 10 ++++-- .../webextension/background/communication.js | 16 +++++++-- addon/webextension/background/deviceInfo.js | 4 ++- addon/webextension/background/main.js | 29 +++++++++++----- .../webextension/background/selectorLoader.js | 7 ++-- addon/webextension/background/senderror.js | 4 ++- addon/webextension/background/takeshot.js | 6 ++-- addon/webextension/catcher.js | 18 ++++++---- addon/webextension/clipboard.js | 4 ++- addon/webextension/domainFromUrl.js | 13 ++++--- addon/webextension/makeUuid.js | 4 ++- addon/webextension/onboarding/slides.js | 4 ++- addon/webextension/randomString.js | 8 +++-- addon/webextension/selector/callBackground.js | 6 ++-- .../webextension/selector/documentMetadata.js | 8 +++-- addon/webextension/selector/shooter.js | 12 ++++--- addon/webextension/selector/ui.js | 32 +++++------------ addon/webextension/selector/uicontrol.js | 4 ++- addon/webextension/selector/util.js | 4 ++- addon/webextension/sitehelper.js | 4 ++- shared/shot.js | 34 ++++++++----------- 22 files changed, 142 insertions(+), 97 deletions(-) diff --git a/addon/webextension/background/analytics.js b/addon/webextension/background/analytics.js index b40209e01d..977763b34e 100644 --- a/addon/webextension/background/analytics.js +++ b/addon/webextension/background/analytics.js @@ -1,6 +1,8 @@ /* globals main, auth, catcher, deviceInfo, communication */ -window.analytics = (function () { +"use strict"; + +var analytics = (function () { let exports = {}; let telemetryPrefKnown = false; @@ -40,8 +42,8 @@ window.analytics = (function () { options.applicationName = di.appName; options.applicationVersion = di.addonVersion; let abTests = auth.getAbTests(); - for (let testName in abTests) { - options[abTests[testName].gaField] = abTests[testName].value; + for (let [gaField, value] of Object.entries(abTests)) { + options[gaField] = value; } console.info(`sendEvent ${eventCategory}/${action}/${label || 'none'} ${JSON.stringify(options)}`); req.send(JSON.stringify({ diff --git a/addon/webextension/background/auth.js b/addon/webextension/background/auth.js index 6ff159aa30..750330f2d7 100644 --- a/addon/webextension/background/auth.js +++ b/addon/webextension/background/auth.js @@ -1,7 +1,9 @@ /* globals browser */ /* globals main, makeUuid, deviceInfo, analytics, catcher, defaultSentryDsn, communication */ -window.auth = (function () { +"use strict"; + +var auth = (function () { let exports = {}; let registrationInfo; @@ -29,8 +31,8 @@ window.auth = (function () { function generateRegistrationInfo() { let info = { - deviceId: "anon" + makeUuid() + "", - secret: makeUuid()+"", + deviceId: `anon${makeUuid()}`, + secret: makeUuid(), registered: false }; return info; @@ -39,6 +41,7 @@ window.auth = (function () { function register() { return new Promise((resolve, reject) => { let registerUrl = main.getBackend() + "/api/register"; + // TODO: replace xhr with Fetch #2261 let req = new XMLHttpRequest(); req.open("POST", registerUrl); req.setRequestHeader("content-type", "application/x-www-form-urlencoded"); @@ -75,6 +78,7 @@ window.auth = (function () { let { ownershipCheck, noRegister } = options || {}; return new Promise((resolve, reject) => { let loginUrl = main.getBackend() + "/api/login"; + // TODO: replace xhr with Fetch #2261 let req = new XMLHttpRequest(); req.open("POST", loginUrl); req.onload = catcher.watchFunction(() => { diff --git a/addon/webextension/background/communication.js b/addon/webextension/background/communication.js index abbb2299e9..daddee74ba 100644 --- a/addon/webextension/background/communication.js +++ b/addon/webextension/background/communication.js @@ -1,5 +1,8 @@ /* globals browser, catcher */ -window.communication = (function () { + +"use strict"; + +var communication = (function () { let exports = {}; let registeredFunctions = {}; @@ -54,13 +57,22 @@ window.communication = (function () { throw new Error(`Error in ${funcName}: ${result.name || 'unknown'}`); } }, (error) => { - if (error && error.message === "Could not establish connection. Receiving end does not exist.") { + if (isBootstrapMissingError(error)) { return exports.NO_BOOTSTRAP; } throw error; }); }; + function isBootstrapMissingError(error) { + if (! error) { + return false; + } + return error.errorCode === "NO_RECEIVING_END" || + (! error.errorCode && error.message === "Could not establish connection. Receiving end does not exist."); + } + + // A singleton/sentinal (with a name): exports.NO_BOOTSTRAP = {name: "communication.NO_BOOTSTRAP"}; diff --git a/addon/webextension/background/deviceInfo.js b/addon/webextension/background/deviceInfo.js index 206b9ca069..b85f5b24dd 100644 --- a/addon/webextension/background/deviceInfo.js +++ b/addon/webextension/background/deviceInfo.js @@ -1,6 +1,8 @@ /* globals browser, catcher */ -window.deviceInfo = (function () { +"use strict"; + +var deviceInfo = (function () { let manifest = browser.runtime.getManifest(); let platformInfo = {}; diff --git a/addon/webextension/background/main.js b/addon/webextension/background/main.js index edd11fb766..b7ac059b0f 100644 --- a/addon/webextension/background/main.js +++ b/addon/webextension/background/main.js @@ -1,7 +1,9 @@ /* globals browser, console, XMLHttpRequest, Image, document, setTimeout, navigator */ /* globals selectorLoader, analytics, communication, catcher, makeUuid, auth, senderror */ -window.main = (function () { +"use strict"; + +var main = (function () { let exports = {}; const pasteSymbol = (window.navigator.platform.match(/Mac/i)) ? "\u2318" : "Ctrl"; @@ -39,7 +41,7 @@ window.main = (function () { } for (let permission of manifest.permissions) { - if (permission.search(/^https?:\/\//i) != -1) { + if (/^https?:\/\//.test(permission)) { exports.setBackend(permission); break; } @@ -168,18 +170,29 @@ window.main = (function () { } browser.tabs.onUpdated.addListener(catcher.watchFunction((id, info, tab) => { - if (info.url && tab.selected) { + if (info.url && tab.active) { if (urlEnabled(info.url)) { browser.browserAction.enable(tab.id); - } - else { - if (hasSeenOnboarding) { - browser.browserAction.disable(tab.id); - } + } else if (hasSeenOnboarding) { + browser.browserAction.disable(tab.id); } } })); + browser.tabs.onActivated.addListener(catcher.watchFunction(({tabId, windowId}) => { + catcher.watchPromise(browser.tabs.get(tabId).then((tab) => { + // onActivated may fire before the url is set + if (!tab.url) { + return; + } + if (urlEnabled(tab.url)) { + browser.browserAction.enable(tabId); + } else { + browser.browserAction.disable(tabId); + } + })); + })); + communication.register("sendEvent", (sender, ...args) => { catcher.watchPromise(sendEvent(...args)); // We don't wait for it to complete: diff --git a/addon/webextension/background/selectorLoader.js b/addon/webextension/background/selectorLoader.js index 8c164d3881..477fd805b1 100644 --- a/addon/webextension/background/selectorLoader.js +++ b/addon/webextension/background/selectorLoader.js @@ -1,5 +1,8 @@ /* globals browser, catcher */ -window.selectorLoader = (function () { + +"use strict"; + +var selectorLoader = (function () { const exports = {}; // These modules are loaded in order, first standardScripts, then optionally onboardingScripts, and then selectorScripts @@ -65,7 +68,7 @@ window.selectorLoader = (function () { }) }); return lastPromise.then(() => { - console.log("finished loading scripts:", scripts.join(" "), "->", browser.runtime.lastError || "no error"); + console.log("finished loading scripts:", scripts.join(" ")); }, (error) => { exports.unloadIfLoaded(tabId); diff --git a/addon/webextension/background/senderror.js b/addon/webextension/background/senderror.js index 80638f4995..4c9e722c7a 100644 --- a/addon/webextension/background/senderror.js +++ b/addon/webextension/background/senderror.js @@ -1,6 +1,8 @@ /* globals analytics, browser, communication, makeUuid, Raven, catcher, auth */ -window.senderror = (function () { +"use strict"; + +var senderror = (function () { let exports = {}; // Do not show an error more than every ERROR_TIME_LIMIT milliseconds: diff --git a/addon/webextension/background/takeshot.js b/addon/webextension/background/takeshot.js index 600b32d129..8bca6aa70f 100644 --- a/addon/webextension/background/takeshot.js +++ b/addon/webextension/background/takeshot.js @@ -1,6 +1,8 @@ /* globals communication, shot, main, auth, catcher, analytics, browser */ -window.takeshot = (function () { +"use strict"; + +var takeshot = (function () { let exports = {}; const Shot = shot.AbstractShot; const { sendEvent } = analytics; @@ -30,7 +32,7 @@ window.takeshot = (function () { } let shotAbTests = {}; let abTests = auth.getAbTests(); - for (let testName in abTests) { + for (let testName of Object.keys(abTests)) { if (abTests[testName].shotField) { shotAbTests[testName] = abTests[testName].value; } diff --git a/addon/webextension/catcher.js b/addon/webextension/catcher.js index 47e17e2a70..ae638c28c2 100644 --- a/addon/webextension/catcher.js +++ b/addon/webextension/catcher.js @@ -1,4 +1,6 @@ -window.catcher = (function () { +"use strict"; + +var catcher = (function () { let exports = {}; let handler; @@ -24,7 +26,7 @@ window.catcher = (function () { result = { fromMakeError: true, name: exc.name || "ERROR", - message: exc+"", + message: String(exc), stack: exc.stack }; for (let attr in exc) { @@ -32,7 +34,7 @@ window.catcher = (function () { } } if (info) { - for (let attr in info) { + for (let attr of Object.keys(info)) { result[attr] = info[attr]; } } @@ -42,20 +44,18 @@ window.catcher = (function () { /** Wrap the function, and if it raises any exceptions then call unhandled() */ exports.watchFunction = function watchFunction(func) { return function () { - var result; try { - result = func.apply(this, arguments); + return func.apply(this, arguments); } catch (e) { exports.unhandled(e); throw e; } - return result; }; }; exports.watchPromise = function watchPromise(promise) { return promise.catch((e) => { - console.error("------Error in promise:", e+""); + console.error("------Error in promise:", e); console.error(e.stack); exports.unhandled(makeError(e)); throw e; @@ -63,6 +63,10 @@ window.catcher = (function () { }; exports.registerHandler = function (h) { + if (handler) { + console.error("registerHandler called after handler was already registered"); + return; + } handler = h; for (let error of queue) { handler(error); diff --git a/addon/webextension/clipboard.js b/addon/webextension/clipboard.js index ca53a99a08..d3683cd90e 100644 --- a/addon/webextension/clipboard.js +++ b/addon/webextension/clipboard.js @@ -1,6 +1,8 @@ /* globals catcher */ -window.clipboard = (function () { +"use strict"; + +var clipboard = (function () { let exports = {}; exports.copy = function (text) { diff --git a/addon/webextension/domainFromUrl.js b/addon/webextension/domainFromUrl.js index 5416a0eae6..c21a275fee 100644 --- a/addon/webextension/domainFromUrl.js +++ b/addon/webextension/domainFromUrl.js @@ -1,16 +1,15 @@ /** Returns the domain of a URL, but safely and in ASCII; URLs without domains (such as about:blank) return the scheme, Unicode domains get stripped down to ASCII */ -window.domainFromUrl = (function () { + +"use strict"; + +var domainFromUrl = (function () { return function urlDomainForId(location) { // eslint-disable-line no-unused-vars let domain = location.hostname; - if (domain) { - if (domain.indexOf(":") !== -1) { - domain = domain.replace(/:.*/, ""); - } - } else { - domain = location.href.split(":")[0]; + if (!domain) { + domain = location.origin.split(":")[0]; if (! domain) { domain = "unknown"; } diff --git a/addon/webextension/makeUuid.js b/addon/webextension/makeUuid.js index 0be855410a..7a22f4d5ae 100644 --- a/addon/webextension/makeUuid.js +++ b/addon/webextension/makeUuid.js @@ -1,4 +1,6 @@ -window.makeUuid = (function () { +"use strict"; + +var makeUuid = (function () { // generates a v4 UUID return function makeUuid() { // eslint-disable-line no-unused-vars diff --git a/addon/webextension/onboarding/slides.js b/addon/webextension/onboarding/slides.js index eda418c472..0a2375f901 100644 --- a/addon/webextension/onboarding/slides.js +++ b/addon/webextension/onboarding/slides.js @@ -1,6 +1,8 @@ /* globals catcher, onboardingHtml, onboardingCss, browser, util, shooter, callBackground, assertIsTrusted */ -window.slides = (function () { +"use strict"; + +var slides = (function () { let exports = {}; const { watchFunction } = catcher; diff --git a/addon/webextension/randomString.js b/addon/webextension/randomString.js index 02626777f9..072c2bdcef 100644 --- a/addon/webextension/randomString.js +++ b/addon/webextension/randomString.js @@ -1,12 +1,14 @@ /* exported randomString */ -window.randomString = function randomString(length, chars) { +"use strict"; + +function randomString(length, chars) { let randomStringChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; chars = chars || randomStringChars; let result = ""; for (let i=0; i { if (result.type === "success") { return result.value; @@ -15,5 +17,5 @@ window.callBackground = function (funcName, ...args) { throw exc; } }); -}; +} null; diff --git a/addon/webextension/selector/documentMetadata.js b/addon/webextension/selector/documentMetadata.js index 1ef926547d..6085008c29 100644 --- a/addon/webextension/selector/documentMetadata.js +++ b/addon/webextension/selector/documentMetadata.js @@ -1,4 +1,6 @@ -window.documentMetadata = (function () { +"use strict"; + +var documentMetadata = (function () { function findSiteName() { let el = document.querySelector("meta[property='og:site_name']"); @@ -34,8 +36,8 @@ window.documentMetadata = (function () { let value; if (elems.length > 1) { value = []; - for (let i=0; i { - const copied = window.clipboard.copy(url); + const copied = clipboard.copy(url); return callBackground("openShot", { url, copied }); }, (error) => { if (error.name != "BackgroundError") { diff --git a/addon/webextension/selector/ui.js b/addon/webextension/selector/ui.js index 5f174c2143..dfa96a38e4 100644 --- a/addon/webextension/selector/ui.js +++ b/addon/webextension/selector/ui.js @@ -1,7 +1,9 @@ /* globals window, document, console, browser */ /* globals util, catcher, inlineSelectionCss, callBackground, assertIsTrusted */ -window.ui = (function () { // eslint-disable-line no-unused-vars +"use strict"; + +var ui = (function () { // eslint-disable-line no-unused-vars let exports = {}; const SAVE_BUTTON_HEIGHT = 50; @@ -91,21 +93,13 @@ window.ui = (function () { // eslint-disable-line no-unused-vars this.element.scrolling = "no"; this.updateElementSize(); this.element.onload = watchFunction(() => { - let parsedDom = (new DOMParser()).parseFromString(` - + this.document = this.element.contentDocument; + this.document.documentElement.innerHTML = ` - - `, - "text/html" - ); - this.document = this.element.contentDocument; - this.document.replaceChild( - this.document.adoptNode(parsedDom.documentElement), - this.document.documentElement - ); + `; installHandlerOnDocument(this.document); if (this.addClassName) { this.document.body.className = this.addClassName; @@ -223,8 +217,8 @@ window.ui = (function () { // eslint-disable-line no-unused-vars this.element.scrolling = "no"; this.updateElementSize(); this.element.onload = watchFunction(() => { - let parsedDom = (new DOMParser()).parseFromString(` - + this.document = this.element.contentDocument; + this.document.documentElement.innerHTML= ` @@ -241,15 +235,7 @@ window.ui = (function () { // eslint-disable-line no-unused-vars - - `, - "text/html" - ); - this.document = this.element.contentDocument; - this.document.replaceChild( - this.document.adoptNode(parsedDom.documentElement), - this.document.documentElement - ); + `; installHandlerOnDocument(this.document); if (this.addClassName) { this.document.body.className = this.addClassName; diff --git a/addon/webextension/selector/uicontrol.js b/addon/webextension/selector/uicontrol.js index 30b4b86995..19b446cc61 100644 --- a/addon/webextension/selector/uicontrol.js +++ b/addon/webextension/selector/uicontrol.js @@ -1,7 +1,9 @@ /* globals console, catcher, util, ui, slides */ /* globals window, document, location, shooter, callBackground, selectorLoader, assertIsTrusted */ -window.uicontrol = (function () { +"use strict"; + +var uicontrol = (function () { let exports = {}; /********************************************************** diff --git a/addon/webextension/selector/util.js b/addon/webextension/selector/util.js index f10f8871e4..cb12722cd4 100644 --- a/addon/webextension/selector/util.js +++ b/addon/webextension/selector/util.js @@ -1,4 +1,6 @@ -window.util = (function () { // eslint-disable-line no-unused-vars +"use strict"; + +var util = (function () { // eslint-disable-line no-unused-vars let exports = {}; /** Removes a node from its document, if it's a node and the node is attached to a parent */ diff --git a/addon/webextension/sitehelper.js b/addon/webextension/sitehelper.js index 9cbfe1ab68..179fc394bc 100644 --- a/addon/webextension/sitehelper.js +++ b/addon/webextension/sitehelper.js @@ -2,7 +2,9 @@ /** This is a content script added to all screenshots.firefox.com pages, and allows the site to communicate with the add-on */ -window.sitehelper = (function () { +"use strict"; + +var sitehelper = (function () { catcher.registerHandler((errorObj) => { callBackground("reportError", errorObj); diff --git a/shared/shot.js b/shared/shot.js index ddd198ab2c..e5665548fc 100644 --- a/shared/shot.js +++ b/shared/shot.js @@ -4,15 +4,12 @@ /** Throws an error if the condition isn't true. Any extra arguments after the condition are used as console.error() arguments. */ -function assert(condition) { - if (! condition) { - console.error.apply(console, ["Failed assertion:"].concat(Array.prototype.slice.call(arguments, 1))); - if (arguments.length > 1) { - throw new Error("Failed assertion: " + Array.prototype.slice.call(arguments, 1).join(" ")); - } else { - throw new Error("Failed assertion"); - } +function assert(condition, ...args) { + if (condition) { + return; } + console.error("Failed assertion", ...args); + throw new Error("Failed assertion", ...args); } /** True if `url` is a valid URL */ @@ -164,15 +161,15 @@ function deepEqual(a, b) { if (Array.isArray(b)) { return false; } - let seen = {}; - for (let attr in a) { + let seen = new Set(); + for (let attr of Object.keys(a)) { if (! deepEqual(a[attr], b[attr])) { return false; } - seen[attr] = true; + seen.add(attr); } - for (let attr in b) { - if (! seen[attr]) { + for (let attr of Object.keys(b)) { + if (! seen.has(attr)) { if (! deepEqual(a[attr], b[attr])) { return false; } @@ -181,16 +178,15 @@ function deepEqual(a, b) { return true; } -function makeUuid() { - // FIXME: not a proper uuid +function makeRandomId() { + // Note: this isn't for secure contexts, only for non-conflicting IDs let id = ""; while (id.length < 12) { - // 46656 == Math.pow(36, 3) let num; if (! id) { - num = Date.now() % 46656; + num = Date.now() % Math.pow(36, 3); } else { - num = Math.floor(Math.random() * 46656); + num = Math.floor(Math.random() * Math.pow(36, 3)); } id += num.toString(36); } @@ -485,7 +481,7 @@ class AbstractShot { return this._clips[name]; } addClip(val) { - let name = makeUuid(); + let name = makeRandomId(); this.setClip(name, val); return name; }