Webextension review changes (#2591)

* Add strict mode statement to all files

Addresses review comment #2471 (comment)

* Do not assign properties to `window`.

Addresses review comment #2471 (comment)

* 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

#2471 (comment)

* Replace IIFE with block scope, yay es6

Addresses selector/shooter.js comment 1

#2471 (comment)

* Use spread/rest operators to simplify assert() fn logic

Addresses shared/shot.js comment 1

#2471 (comment)

* 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

@@ -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({

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(() => {

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"};
 

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 = {};

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:

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);

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:

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;
       }

addon/webextension/catcher.js

@@ -1,4 +1,6 @@
-window.catcher = (function () {
+"use strict";
+
+var catcher = (function () {
   let exports = {};
 
   let handler;
@@ -24,15 +26,15 @@ window.catcher = (function () {
       result = {
         fromMakeError: true,
         name: exc.name || "ERROR",
-        message: exc+"",
+        message: String(exc),
         stack: exc.stack
       };
       for (let attr in exc) {
         result[attr] = exc[attr];
       }
     }
     if (info) {
-      for (let attr in info) {
+      for (let attr of Object.keys(info)) {
         result[attr] = info[attr];
       }
     }
@@ -42,27 +44,29 @@ 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;
     });
   };
 
   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);

addon/webextension/clipboard.js

@@ -1,6 +1,8 @@
 /* globals catcher */
 
-window.clipboard = (function () {
+"use strict";
+
+var clipboard = (function () {
   let exports = {};
 
   exports.copy = function (text) {

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";
       }

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

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;

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<length; i++) {
-    result += chars.charAt(Math.floor(Math.random() * chars.length));
+    result += chars[Math.floor(Math.random() * chars.length)]
   }
   return result;
-};
+}
 null;

addon/webextension/selector/callBackground.js

@@ -1,6 +1,8 @@
 /* globals browser */
 
-window.callBackground = function (funcName, ...args) {
+"use strict";
+
+function callBackground (funcName, ...args) {
   return browser.runtime.sendMessage({funcName, args}).then((result) => {
     if (result.type === "success") {
       return result.value;
@@ -15,5 +17,5 @@ window.callBackground = function (funcName, ...args) {
       throw exc;
     }
   });
-};
+}
 null;

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<elems.length; i++) {
-          let v = elems[i].getAttribute("content");
+        for (let elem of elems) {
+          let v = elem.getAttribute("content");
           if (v) {
             value.push(v);
           }