Webextension review changes #2591

jaredhirsch · 2017-04-06T23:09:06Z

This seems to be working fine for me locally. The outerHTML commit is reverted, I'll work on fixing that right now.

Still unaddressed things from the big review:

webext review changes: callBackground.js #2592 callBackground.js:
- need to see if Mark has an .eslintrc (comment at line 1), then remove the eslint globals
- look into rejecting the Promise from the parent side (comment on line 8)
webext review changes: shared/shot.js #2593 shared/shot.js: deepEqual function changes, makeUuid changes, asJson vs toJson (all kinda scary and not clear we have to change them before uplift)
webext review changes: communication.js #2594 background/communication.js: return a promise.resolve (line 34), don't parse error string (line 56, already an open question there)
webext review changes: sitehelper.js #2595 sitehelper.js: replace JSON.stringify with cloneInto (line 16, kinda seems optional IMO)
(done) webext review changes: randomString.js #2596 randomString.js: array indexing fix (line 8)

edited to add bug numbers

[Ian] Also added:

Change sendToBootstrap error check #2603 to change error.message check
Create logging level #2604 to have a logging level
Investigate moz-extension iframe #2605 to experiment with a moz-extension: iframe
Use Set in shot.js deepEqual for seen #2666 for using a Set in deepEqual

jaredhirsch · 2017-04-06T23:55:39Z

@ianb It seems like setting the iframe's contentDocument innerHTML works equivalently to the previous use of DOMParser (see most recent commit, 7f03043). The html document inside the iframe is now not fully replaced when the overlay is rendered, but if I take multiple screenshots without reloading a content page, I don't see any errors related to multiple copies of scripts or handlers running in the iframe. Thoughts?

jaredhirsch · 2017-04-07T00:40:21Z

Rebased against current master

ianb · 2017-04-07T16:47:54Z

addon/webextension/background/auth.js

@@ -29,8 +31,8 @@ window.auth = (function () {

  function generateRegistrationInfo() {
    let info = {
-      deviceId: "anon" + makeUuid() + "",
-      secret: makeUuid()+"",
+      deviceId: `anon ${makeUuid()}`,


This introduces a space into the string, so anon UUID instead of anonUUID

ianb · 2017-04-07T16:48:29Z

addon/webextension/background/analytics.js

@@ -13,7 +15,7 @@ window.analytics = (function () {
      return Promise.resolve();
    }
    if (! telemetryPref) {
-      console.info(`Cancelled sendEvent ${eventCategory}/${action}/${label || 'none'} ${JSON.stringify(options)}`);
+      dump(`Cancelled sendEvent ${eventCategory}/${action}/${label || 'none'} ${JSON.stringify(options)}`);


I opened #2604 for this, which I would prefer to dump()

ianb · 2017-04-07T16:49:49Z

addon/webextension/background/main.js

@@ -108,11 +110,6 @@ window.main = (function () {
    title: browser.i18n.getMessage("contextMenuLabel"),
    contexts: ["page"],
    documentUrlPatterns: ["<all_urls>"]
-  }, () => {


I think this catcher is the only way to catch an error from contextMenu, unlike some of the other uses of lastError.

ianb · 2017-04-07T16:51:18Z

addon/webextension/catcher.js

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


We should have catcher.unhandled(new Exception("registerHandler called after handler was already registered"))

I take that back, catcher calling catcher will lead to bad things

dannycoates · 2017-04-07T17:43:29Z

shared/shot.js

    } else {
-      num = Math.floor(Math.random() * 46656);
+      num = Math.floor(Math.random() * Math.pow(36, 3));


Do these ids need to be globally unique or just within a single account? If global, can we use crypto here? Date.now() and Math.random() make me nervous here.

We only want the ids for clips generated within a single shot to not conflict. This isn't used for any security-conscious situation. Things like clip image URLs are generated with UUIDs on the server.

jaredhirsch · 2017-04-10T22:53:42Z

@ianb Should I merge this, or do you want to leave it open till Kris finishes responding to the review?

ianb · 2017-04-10T22:54:35Z

@6a68 I think Kris can still leave comments after it closes, and it's already bitrotting, let's get it landed

jaredhirsch · 2017-04-10T22:57:05Z

👍 I"ll squash, rebase and land and push the fixes commit to the PR branch Kris is looking at

Addresses review comment #2471 (comment)

Addresses comments 2 and 3 inside selector/ui.js

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 onActivated listener to ensure the button state is properly toggled when switching between tabs

TODO: investigate executeScript return value...

…into iframe" This reverts commit 224927a0b072d2bfb851e6b45ac0ed367ab6bb93.

* 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

jaredhirsch force-pushed the webextension-review-changes branch from 7f03043 to ded7390 Compare April 7, 2017 00:40

This was referenced Apr 7, 2017

webext review changes: callBackground.js #2592

Open

webext review changes: shared/shot.js #2593

Closed

webext review changes: communication.js #2594

Closed

webext review changes: sitehelper.js #2595

Closed

webext review changes: randomString.js #2596

Closed

ianb suggested changes Apr 7, 2017

View reviewed changes

ianb force-pushed the webextension-review-changes branch from f7239ca to 37c23a2 Compare April 7, 2017 17:32

ianb approved these changes Apr 7, 2017

View reviewed changes

dannycoates reviewed Apr 7, 2017

View reviewed changes

jaredhirsch added 15 commits April 10, 2017 16:03

Add strict mode statement to all files

4a9da2d

Addresses review comment #2471 (comment)

Do not assign properties to window.

7169a70

Addresses review comment #2471 (comment)

Use docElement.outerHTML, not DOMParser, to insert html page into iframe

103a7ee

Addresses comments 2 and 3 inside selector/ui.js

Use for..of instead of for-loop

419fbc8

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

Replace IIFE with block scope, yay es6

8e21c18

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

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

81ae231

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

WIP fixes to shared/shot

4675a20

Nit - replace let..in with let..of

e6534fe

Nit: replace foo+"" with String(foo)

a460811

nits related to catcher.js

4efe604

auth updates

aa40905

review changes to main.js

f49ae02

- add onActivated listener to ensure the button state is properly toggled when switching between tabs

selectorLoader changes

24ae4e5

TODO: investigate executeScript return value...

last few review nits

53c888c

Revert "Use docElement.outerHTML, not DOMParser, to insert html page …

da9184f

…into iframe" This reverts commit 224927a0b072d2bfb851e6b45ac0ed367ab6bb93.

jaredhirsch and others added 7 commits April 10, 2017 16:09

Use innerHTML: outerHTML throws a NoModificationAllowedError

6f1923e

Please the linter

7128177

Fix #2596, address review nit in randomString.js

d156af9

Remove space in deviceId generation

f0ed360

Restore lastError handler for contextMenus.create

ea68171

Rename shot.js makeUuid to more accurate makeRandomId

69c6113

Fix #2603, abstract out sendToBootstrap error checking

7fc0e61

jaredhirsch force-pushed the webextension-review-changes branch from 549a61b to 7fc0e61 Compare April 10, 2017 23:09

jaredhirsch merged commit ebe57df into master Apr 10, 2017

jaredhirsch deleted the webextension-review-changes branch April 10, 2017 23:19

dannycoates mentioned this pull request Apr 11, 2017

unloadModules is broken #2633

Closed

ianb mentioned this pull request Apr 11, 2017

PageShot overlay is wrongly activated when its toolbar button is clicked more than once #2244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webextension review changes #2591

Webextension review changes #2591

jaredhirsch commented Apr 6, 2017 •

edited by ianb

Loading

jaredhirsch commented Apr 6, 2017

jaredhirsch commented Apr 7, 2017

ianb Apr 7, 2017

ianb Apr 7, 2017

ianb Apr 7, 2017

ianb Apr 7, 2017

ianb Apr 7, 2017

dannycoates Apr 7, 2017

ianb Apr 7, 2017

jaredhirsch commented Apr 10, 2017

ianb commented Apr 10, 2017

jaredhirsch commented Apr 10, 2017

Webextension review changes #2591

Webextension review changes #2591

Conversation

jaredhirsch commented Apr 6, 2017 • edited by ianb Loading

jaredhirsch commented Apr 6, 2017

jaredhirsch commented Apr 7, 2017

ianb Apr 7, 2017

Choose a reason for hiding this comment

ianb Apr 7, 2017

Choose a reason for hiding this comment

ianb Apr 7, 2017

Choose a reason for hiding this comment

ianb Apr 7, 2017

Choose a reason for hiding this comment

ianb Apr 7, 2017

Choose a reason for hiding this comment

dannycoates Apr 7, 2017

Choose a reason for hiding this comment

ianb Apr 7, 2017

Choose a reason for hiding this comment

jaredhirsch commented Apr 10, 2017

ianb commented Apr 10, 2017

jaredhirsch commented Apr 10, 2017

jaredhirsch commented Apr 6, 2017 •

edited by ianb

Loading