Add all the add-on files for review #2471

ianb · 2017-03-23T21:05:19Z

This is a PR specifically intended to review the entirety of the add-on code. This is not intended to be merged, only to collect comments

Mossop · 2017-03-23T22:04:17Z

addon/bootstrap.js

+function uninstall(data, reason) {} // eslint-disable-line no-unused-vars
+
+function getBoolPref(pref) {
+  return prefs.getPrefType(pref) && prefs.getBoolPref(pref);


You can now pass a second argument to getBoolPref to specify a default if the pref doesn't exist.

I was hoping we could keep Firefox 52 compatibility (for ESR), and @Standard8 mentioned that this was a 54 API change... except, if we distribute something for Firefox 52 is might be webextension/ only and then compatibility of bootstrap.js won't matter.

That's fine then. You might also consider using Preferences.jsm which gives you the ability to do this even on 52 and earlier.

Mossop · 2017-03-23T22:05:35Z

addon/bootstrap.js

+}
+
+function handleStartup() {
+  AddonManager.getAddonByID(ADDON_ID).then((addon) => {


Seems like you mostly do this to get the resource URI for the add-on? That is passed on the data object to startup already so you could just pass it through to here and not need this getAddonByID call which can cause a startup performance hit.

Mossop · 2017-03-23T22:16:51Z

addon/bootstrap.js

+
+function stop(webExtension) {
+  webExtension.shutdown().then(() => {
+    EmbeddedExtensionManager.untrackEmbeddedExtension(webExtension);


Looks like the shutdown call does this already.

Mossop · 2017-03-23T22:27:14Z

addon/bootstrap.js

+  }
+
+  if (msg.funcName === "getTelemetryPref") {
+    let enableTelemetry = getBoolPref(TELEMETRY_PREF);


This had me blinking for a moment as I thought you were turning telemetry on. Can you rename this variable to telemetryEnabled? I think that makes more sense.

Mossop · 2017-03-23T22:33:40Z

addon/bootstrap.js

+Cu.import("resource://gre/modules/AddonManager.jsm");
+Cu.import("resource://gre/modules/Console.jsm");
+Cu.import("resource://gre/modules/Services.jsm");
+const { EmbeddedExtensionManager } = Cu.import("resource://gre/modules/LegacyExtensionsUtils.jsm");


Can you just use the exposed LegacyExtensionsUtils instead since you don't need the untrack call below?

jaredhirsch · 2017-03-24T18:17:49Z

@Mossop Thanks for the feedback. I'll get a PR out today that addresses the points you mentioned above.

jaredhirsch · 2017-03-27T15:00:29Z

I've landed the requested bootstrap.js changes in master, and cherry-picked that commit into this branch.

jaredhirsch · 2017-03-27T15:13:52Z

Question for reviewers: It looks like window.dump() is available to WebExtensions. Should we switch to that instead of console.* throughout?

edit: added MDN link

kmaglione

I need to review some of the UI and shared code in more detail, but these are my initial comments.

kmaglione · 2017-03-24T19:09:47Z

addon/webextension/randomString.js

@@ -0,0 +1,12 @@
+/* exported randomString */
+
+window.randomString = function randomString(length, chars) {


Please don't add properties to window. Just function randomString() { ... } or var randomString = ... will do.

kmaglione · 2017-03-24T19:10:46Z

addon/webextension/randomString.js

+  chars = chars || randomStringChars;
+  let result = "";
+  for (let i=0; i<length; i++) {
+    result += chars.charAt(Math.floor(Math.random() * chars.length));


Nit: Please use array indexing rather than .charAt. chars[Math.floor(...)]

kmaglione · 2017-03-24T19:12:10Z

addon/webextension/selector/callBackground.js

+    if (result.type === "success") {
+      return result.value;
+    } else if (result.type === "error") {
+      throw new Error(result.name);


You can throw this error (or equivalently reject this promise) directly from the parent side.

kmaglione · 2017-03-24T19:14:57Z

addon/webextension/selector/ui.js

+          this.element.scrolling = "no";
+          this.updateElementSize();
+          this.element.onload = watchFunction(() => {
+            let parsedDom = (new DOMParser()).parseFromString(`


Is there a particular reason to do this rather than just assigning to documentElement.outerHTML?

kmaglione · 2017-03-24T19:20:11Z

addon/webextension/selector/documentMetadata.js

+      let value;
+      if (elems.length > 1) {
+        value = [];
+        for (let i=0; i<elems.length; i++) {


Nit: for (let elem of elems)

kmaglione · 2017-03-31T23:49:02Z

addon/webextension/selector/ui.js

+    display: function (installHandlerOnDocument) {
+      return new Promise((resolve, reject) => {
+        if (! this.element) {
+          this.element = document.createElement("iframe");


The contents of this iframe will be accessible to web content, which worries me. Can we load a web-accessible extension URL into them?

We took this approach from uBlock Origin's element picker. In general in my experiments it seemed like the content worker has no special ability to look into iframes, though I never specifically tried a moz-extension URL for the iframe; is that different?

I opened #2605 to investigate this. I'm not confident it's going to work, though it would be a security improvement if we could do it.

If the iframe has a moz-extension: URL, your content script will be able to access its contents, since it has an expanded principal with a moz-extension: URL, but the content page won't.

kmaglione · 2017-03-31T23:54:08Z

addon/webextension/sitehelper.js

+/** 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 () {


I'd rather we use export helpers (exportFunction, cloneInto, ...) than custom events for this.

I created #2573 for this – the refactoring worries me right now, so I'd like to save that work for later.

kmaglione · 2017-03-31T23:55:48Z

addon/webextension/sitehelper.js

+    if (typeof detail == "object") {
+      // Note sending an object can lead to security problems, while a string
+      // is safe to transfer:
+      detail = JSON.stringify(detail);


You would be better off with sendMessage than custom events, since messages are structured cloned by default. However, if you need to use custom events, you can use cloneInto to make the object accessible to content.

Let's switch to cloneInto and leave postMessage for now

kmaglione · 2017-03-31T23:57:45Z

shared/shot.js

+    are used as console.error() arguments. */
+function assert(condition) {
+  if (! condition) {
+    console.error.apply(console, ["Failed assertion:"].concat(Array.prototype.slice.call(arguments, 1)));


console.error("Failed assertion", ...args) along with ...args as a second argument to assert

kmaglione · 2017-03-31T23:59:28Z

shared/shot.js

+    for (let attr of this.REGULAR_ATTRS) {
+      var val = this[attr];
+      if (val && val.asJson) {
+        val = val.asJson();


The toJSON method is the standard way of doing this, and is automatically called by JSON.stringify

jaredhirsch · 2017-04-04T18:19:07Z

@kmaglione Thanks for the review! I'll get started addressing your feedback in the webextension-review-changes branch and ping you with any questions.

Addresses review comment #2471 (comment)

Addresses selector/documentMetadata.js comment 1 #2471 (comment)

Addresses selector/shooter.js comment 1 #2471 (comment)

Addresses shared/shot.js comment 1 #2471 (comment)

Addresses review comment #2471 (comment)

Addresses selector/documentMetadata.js comment 1 #2471 (comment)

Addresses selector/shooter.js comment 1 #2471 (comment)

Addresses shared/shot.js comment 1 #2471 (comment)

Addresses review comment #2471 (comment)

Addresses selector/documentMetadata.js comment 1 #2471 (comment)

Addresses selector/shooter.js comment 1 #2471 (comment)

Addresses shared/shot.js comment 1 #2471 (comment)

Addresses review comment #2471 (comment)

Addresses selector/documentMetadata.js comment 1 #2471 (comment)

Addresses selector/shooter.js comment 1 #2471 (comment)

Addresses shared/shot.js comment 1 #2471 (comment)

* 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

ianb · 2017-04-13T17:55:26Z

I'm going to close this PR – we've discussed all the comments, fixed a bunch, made followup bugs for all the rest. Thanks @kmaglione and @Mossop !

Add all the add-on files for review

d26f842

Mossop reviewed Mar 23, 2017

View reviewed changes

jaredhirsch mentioned this pull request Mar 24, 2017

Update bootstrap.js per review comments #2480

Merged

Update bootstrap.js per @Mossop review comments

a2de454

Fix stray variable renaming nit from bootstrap review

5daddc5

kmaglione suggested changes Apr 1, 2017

View reviewed changes

jaredhirsch added a commit that referenced this pull request Apr 4, 2017

Add strict mode statement to all files

4f91df5

Addresses review comment #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 4, 2017

Do not assign properties to window.

837f11f

Addresses review comment #2471 (comment)

ianb mentioned this pull request Apr 5, 2017

Convert sitehelper CustomEvent to exportFunction #2573

Closed

jaredhirsch added a commit that referenced this pull request Apr 6, 2017

Add strict mode statement to all files

754c4f2

Addresses review comment #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 6, 2017

Do not assign properties to window.

0ba629b

Addresses review comment #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 6, 2017

Use for..of instead of for-loop

b4d3c12

Addresses selector/documentMetadata.js comment 1 #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 6, 2017

Replace IIFE with block scope, yay es6

68a5903

Addresses selector/shooter.js comment 1 #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 6, 2017

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

9d21353

Addresses shared/shot.js comment 1 #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 7, 2017

Add strict mode statement to all files

3cb9040

Addresses review comment #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 7, 2017

Do not assign properties to window.

689c419

Addresses review comment #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 7, 2017

Use for..of instead of for-loop

6c98939

Addresses selector/documentMetadata.js comment 1 #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 7, 2017

Replace IIFE with block scope, yay es6

8a9bac4

Addresses selector/shooter.js comment 1 #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 7, 2017

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

674f894

Addresses shared/shot.js comment 1 #2471 (comment)

ianb mentioned this pull request Apr 7, 2017

Open questions on WebExtensions #2602

Closed

3 tasks

ianb pushed a commit that referenced this pull request Apr 7, 2017

Add strict mode statement to all files

fef04cc

Addresses review comment #2471 (comment)

ianb pushed a commit that referenced this pull request Apr 7, 2017

Do not assign properties to window.

a58ea0e

Addresses review comment #2471 (comment)

ianb pushed a commit that referenced this pull request Apr 7, 2017

Use for..of instead of for-loop

db3280e

Addresses selector/documentMetadata.js comment 1 #2471 (comment)

ianb pushed a commit that referenced this pull request Apr 7, 2017

Replace IIFE with block scope, yay es6

168068a

Addresses selector/shooter.js comment 1 #2471 (comment)

ianb pushed a commit that referenced this pull request Apr 7, 2017

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

2111e79

Addresses shared/shot.js comment 1 #2471 (comment)

ianb mentioned this pull request Apr 10, 2017

webext review changes: shared/shot.js #2593

Closed

jaredhirsch added a commit that referenced this pull request Apr 10, 2017

Add strict mode statement to all files

4a9da2d

Addresses review comment #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 10, 2017

Do not assign properties to window.

7169a70

Addresses review comment #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 10, 2017

Use for..of instead of for-loop

419fbc8

Addresses selector/documentMetadata.js comment 1 #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 10, 2017

Replace IIFE with block scope, yay es6

8e21c18

Addresses selector/shooter.js comment 1 #2471 (comment)

jaredhirsch added a commit that referenced this pull request Apr 10, 2017

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

81ae231

Addresses shared/shot.js comment 1 #2471 (comment)

This was referenced Apr 12, 2017

Remove browser.runtime.lastError handling for contextMenu #2662

Closed

Remove window.selectorLoader invocation and replace with messaging to detect live workers #2663

Open

Fix result.toString() == "true" #2664

Closed

ianb closed this Apr 13, 2017

chenba mentioned this pull request Mar 14, 2018

Rename asJson to toJSON. (#2593) #4221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all the add-on files for review #2471

Add all the add-on files for review #2471

ianb commented Mar 23, 2017

Mossop Mar 23, 2017

ianb Mar 24, 2017

Mossop Mar 24, 2017

Mossop Mar 23, 2017

Mossop Mar 23, 2017

Mossop Mar 23, 2017

Mossop Mar 23, 2017

jaredhirsch commented Mar 24, 2017

jaredhirsch commented Mar 27, 2017

jaredhirsch commented Mar 27, 2017 •

edited

Loading

kmaglione left a comment

kmaglione Mar 24, 2017

kmaglione Mar 24, 2017

kmaglione Mar 24, 2017

kmaglione Mar 24, 2017

kmaglione Mar 24, 2017

kmaglione Mar 31, 2017

ianb Apr 5, 2017

ianb Apr 7, 2017

kmaglione Apr 11, 2017

kmaglione Mar 31, 2017

ianb Apr 5, 2017

kmaglione Mar 31, 2017

ianb Apr 12, 2017

kmaglione Mar 31, 2017

kmaglione Mar 31, 2017

jaredhirsch commented Apr 4, 2017

ianb commented Apr 13, 2017

		@@ -0,0 +1,12 @@
		/* exported randomString */

		window.randomString = function randomString(length, chars) {

Add all the add-on files for review #2471

Add all the add-on files for review #2471

Conversation

ianb commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredhirsch commented Mar 24, 2017

jaredhirsch commented Mar 27, 2017

jaredhirsch commented Mar 27, 2017 • edited Loading

kmaglione left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredhirsch commented Apr 4, 2017

ianb commented Apr 13, 2017

jaredhirsch commented Mar 27, 2017 •

edited

Loading