Closed
Bug 1398630
Opened 7 years ago
Closed 7 years ago
Quantum code cleanup
Categories
(WebExtensions :: General, enhancement)
WebExtensions
General
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)
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
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 9•7 years ago
|
||
mozreview-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 10•7 years ago
|
||
mozreview-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 11•7 years ago
|
||
mozreview-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 12•7 years ago
|
||
mozreview-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 13•7 years ago
|
||
mozreview-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 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8906418 [details] Bug 1398630: Part 7 - Random cleanup. https://reviewboard.mozilla.org/r/178132/#review183332
Attachment #8906418 -
Flags: review?(tomica) → review+
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
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 18•7 years ago
|
||
mozreview-review |
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 19•7 years ago
|
||
mozreview-review-reply |
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.
Updated•7 years ago
|
Summary: Code cleanup → Quantum code cleanup
Comment 20•7 years ago
|
||
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
Assignee | ||
Comment 21•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf7f1a4526dfb746d19f1086fedf680c087d776 Bug 1398630: Part 1 - Remove/cleanup some old ExtensionUtils helpers. r=zombie https://hg.mozilla.org/integration/mozilla-inbound/rev/4c80b55b7755f211c56761475170ba5f21e20950 Bug 1398630: Part 2 - Avoid unnecessary Map/Set lookups. r=zombie https://hg.mozilla.org/integration/mozilla-inbound/rev/d72a3343d7948cb2e886821e830653ddd980f5e5 Bug 1398630: Part 3 - Use document.docShell rather than longer/slower XPC paths. r=zombie https://hg.mozilla.org/integration/mozilla-inbound/rev/a71d68e6d81eb5af0bacb0578f3ea8d44ec407d3 Bug 1398630: Part 4 - Use getWinUtils everywhere we use DOMWindowUtils. r=zombie https://hg.mozilla.org/integration/mozilla-inbound/rev/ce90ba36975e1e4c90d32d03654f82b94da2512a Bug 1398630: Part 5 - User iteration helpers for nsISimpleEnumerator. r=zombie https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c96a5d7b546e09dabf14e2d6e24e3231c1c391 Bug 1398630: Part 6 - Avoid some avoidable uses of nsIURI. r=zombie https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbffd53d2da0fc066c2574c2a2db2f1cb211f39 Bug 1398630: Part 7 - Random cleanup. r=zombie
Assignee | ||
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/966b3dcca29215ada9d7ba1e8ff6739537a4b198 Bug 1398630: Follow-up: Fix typo. r=me
Assignee | ||
Comment 23•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1eb321f774988ad39f25a0794fa79e7ebb03233 Bug 1398630: Follow-up: Fix another typo. r=me
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2cf7f1a4526d https://hg.mozilla.org/mozilla-central/rev/4c80b55b7755 https://hg.mozilla.org/mozilla-central/rev/d72a3343d794 https://hg.mozilla.org/mozilla-central/rev/a71d68e6d81e https://hg.mozilla.org/mozilla-central/rev/ce90ba36975e https://hg.mozilla.org/mozilla-central/rev/b5c96a5d7b54 https://hg.mozilla.org/mozilla-central/rev/fbbffd53d2da https://hg.mozilla.org/mozilla-central/rev/966b3dcca292 https://hg.mozilla.org/mozilla-central/rev/b1eb321f7749
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 25•7 years ago
|
||
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)
Updated•7 years ago
|
Whiteboard: qe-verify-
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•