Closed Bug 1398630 Opened 7 years ago Closed 7 years ago

Quantum code cleanup

Categories

(WebExtensions :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: qe-verify-)

Attachments

(7 files)

      No description provided.
Comment on attachment 8906412 [details]
Bug 1398630: Part 1 - Remove/cleanup some old ExtensionUtils helpers.

https://reviewboard.mozilla.org/r/178120/#review183088

::: toolkit/components/extensions/ExtensionCommon.jsm:382
(Diff revision 1)
>      }
>      let message, fileName;
> -    if (instanceOf(error, "Object") || error instanceof ExtensionError ||
> -        (typeof error == "object" && this.principal.subsumes(Cu.getObjectPrincipal(error)))) {
> +    if (error && typeof error === "object" &&
> +        (ChromeUtils.getClassName(error) === "Object" ||
> +         error instanceof ExtensionError ||
> +         this.principal.subsumes(Cu.getObjectPrincipal(error)))) {

Oh neat, we don't propagate errors to less privileged contexts.

::: toolkit/components/extensions/ExtensionUtils.jsm:63
(Diff revision 1)
> -  return runSafeWithoutClone(f, ...args);
> -}
> -
>  // Return true if the given value is an instance of the given
>  // native type.
>  function instanceOf(value, type) {

This now seems to be used only in `ExtensionTestCommon.jsm`, so perhaps move it there?
Attachment #8906412 - Flags: review?(tomica) → review+
Comment on attachment 8906413 [details]
Bug 1398630: Part 2 - Avoid unnecessary Map/Set lookups.

https://reviewboard.mozilla.org/r/178122/#review183066
Attachment #8906413 - Flags: review?(tomica) → review+
Comment on attachment 8906414 [details]
Bug 1398630: Part 3 - Use document.docShell rather than longer/slower XPC paths.

https://reviewboard.mozilla.org/r/178124/#review183068

::: browser/components/extensions/ext-browser.js:520
(Diff revision 1)
>  
>    getBrowserData(browser) {
>      if (browser.ownerDocument.documentURI === "about:addons") {
>        // When we're loaded into a <browser> inside about:addons, we need to go up
>        // one more level.
> -      browser = browser.ownerGlobal.QueryInterface(Ci.nsIInterfaceRequestor)
> +      browser = browser.ownerGlobal.document.docShell

nit: `browser.ownerDocument...`

::: browser/components/extensions/ext-tabs.js:736
(Diff revision 1)
>            let zoomListener = event => {
>              let browser = event.originalTarget;
>  
>              // For non-remote browsers, this event is dispatched on the document
>              // rather than on the <browser>.
>              if (browser instanceof Ci.nsIDOMDocument) {

I can't find the `docShell` property on the `nsIDOMDocument` idl, is that new?  (I see that it works, just not why / what changed that XPC is not needed).

::: toolkit/components/extensions/ExtensionParent.jsm:487
(Diff revision 1)
>    }
>  
>    // The window that contains this context. This may change due to moving tabs.
>    get xulWindow() {
>      let win = this.xulBrowser.ownerGlobal;
> -    return win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell)
> +    return win.document.docShell.rootTreeItem

Don't even need to QI for `rootTreeItem`?  Madness!
Attachment #8906414 - Flags: review?(tomica) → review+
Comment on attachment 8906415 [details]
Bug 1398630: Part 4 - Use getWinUtils everywhere we use DOMWindowUtils.

https://reviewboard.mozilla.org/r/178126/#review183080

Something seems wrong if we need to do this.  When are you ripping out XPConnect (as a fresh peer)?  :P

::: toolkit/components/extensions/ext-theme.js:14
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyGetter(this, "gThemesEnabled", () => {
>    return Services.prefs.getBoolPref("extensions.webextensions.themes.enabled");
>  });
>  
> +var {

Why `var`?
Attachment #8906415 - Flags: review?(tomica) → review+
Comment on attachment 8906416 [details]
Bug 1398630: Part 5 - User iteration helpers for nsISimpleEnumerator.

https://reviewboard.mozilla.org/r/178128/#review183086
Attachment #8906416 - Flags: review?(tomica) → review+
Comment on attachment 8906417 [details]
Bug 1398630: Part 6 - Avoid some avoidable uses of nsIURI.

https://reviewboard.mozilla.org/r/178130/#review183302

::: toolkit/components/extensions/ext-cookies.js:214
(Diff revision 1)
>          return true;
>        }
>  
>        // URL path is a substring of the cookie path, so it matches if, and
>        // only if, the next character is a path delimiter.
> -      let pathDelimiters = ["/", "?", "#", ";"];
> +      return path[cookiePath.length] === "/";

I don't know what are the rules here, and I don't understand this change.

Can you please add a jsdoc comment above the function that explains/references the rules for matching paths?
Comment on attachment 8906418 [details]
Bug 1398630: Part 7 - Random cleanup.

https://reviewboard.mozilla.org/r/178132/#review183332
Attachment #8906418 - Flags: review?(tomica) → review+
Comment on attachment 8906415 [details]
Bug 1398630: Part 4 - Use getWinUtils everywhere we use DOMWindowUtils.

https://reviewboard.mozilla.org/r/178126/#review183080

Believe me, if it were that simple, we'd have done it already :P

> Why `var`?

Because all of the API scripts run in the same lexical scope, and if two scripts declare the same variables with `let` or `const`, it throws.
Comment on attachment 8906414 [details]
Bug 1398630: Part 3 - Use document.docShell rather than longer/slower XPC paths.

https://reviewboard.mozilla.org/r/178124/#review183068

> I can't find the `docShell` property on the `nsIDOMDocument` idl, is that new?  (I see that it works, just not why / what changed that XPC is not needed).

`nsIDOMDocument` isn't actually used anymore. It just still works for type checks. The actual binding is implemented in WebIDL: http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/dom/webidl/Document.webidl#380
Comment on attachment 8906417 [details]
Bug 1398630: Part 6 - Avoid some avoidable uses of nsIURI.

https://reviewboard.mozilla.org/r/178130/#review183302

> I don't know what are the rules here, and I don't understand this change.
> 
> Can you please add a jsdoc comment above the function that explains/references the rules for matching paths?

The only change is that the path we get from URL objects doesn't include the query string or reference fragments, so there's no need to check for those characters anymore.
Comment on attachment 8906417 [details]
Bug 1398630: Part 6 - Avoid some avoidable uses of nsIURI.

https://reviewboard.mozilla.org/r/178130/#review183392
Attachment #8906417 - Flags: review?(tomica) → review+
Comment on attachment 8906417 [details]
Bug 1398630: Part 6 - Avoid some avoidable uses of nsIURI.

https://reviewboard.mozilla.org/r/178130/#review183302

> The only change is that the path we get from URL objects doesn't include the query string or reference fragments, so there's no need to check for those characters anymore.

Ah right, sorry I missed that.
Summary: Code cleanup → Quantum code cleanup
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf7f1a4526d
Part 1 - Remove/cleanup some old ExtensionUtils helpers. r=zombie
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c80b55b7755
Part 2 - Avoid unnecessary Map/Set lookups. r=zombie
https://hg.mozilla.org/integration/mozilla-inbound/rev/d72a3343d794
Part 3 - Use document.docShell rather than longer/slower XPC paths. r=zombie
https://hg.mozilla.org/integration/mozilla-inbound/rev/a71d68e6d81e
Part 4 - Use getWinUtils everywhere we use DOMWindowUtils. r=zombie
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce90ba36975e
Part 5 - User iteration helpers for nsISimpleEnumerator. r=zombie
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c96a5d7b54
Part 6 - Avoid some avoidable uses of nsIURI. r=zombie
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbffd53d2da
Part 7 - Random cleanup. r=zombie
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
Whiteboard: qe-verify-
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.