Closed
Bug 1431533
Opened 7 years ago
Closed 7 years ago
Add ChromeUtils helpers for lazy module import
Categories
(Core :: XPConnect, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [stockwell fixed:other])
Attachments
(8 files, 2 obsolete files)
59 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
52 bytes,
text/x-github-pull-request
|
Details | Review | |
52 bytes,
text/x-github-pull-request
|
Details | Review |
This would be considerably faster than our current JS-based solution. My initial testing shows about a 2-3% improvement on ts_paint.
Assignee | ||
Updated•7 years ago
|
Component: General → XPConnect
Product: Firefox → Core
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 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8943823 [details] Bug 1431533: Part 4 - Add ESLint support for ChromeUtils import methods. https://reviewboard.mozilla.org/r/214180/#review219960 Apologies for fly-by comments, however I believe Gijs is out today and this is overlapping with some of the work that he's done. ::: tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:44 (Diff revision 1) > "XPCOMUtils.defineLazyModuleGetters(this,", > "XPCOMUtils.defineLazyServiceGetters(this," > ]; > > const imports = [ > - /^(?:Cu|Components\.utils)\.import\(".*\/((.*?)\.jsm?)"(?:, this)?\)/ > + /^(?:Cu|Components\.utils|ChromeUtils)\.import\(".*\/((.*?)\.jsm?)"(?:, this)?\)/ Gijs has already added this in bug 1431069 ::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-chromeutils-import.js:2 (Diff revision 1) > +/** > + * @fileoverview Reject use of Cu.import and XPCOMUtils.defineLazyModuleGetter This file is going to conflict with bug 1431104. Maybe the rules could be combined into the same files? Also, this needs a test adding (I've also commented on that in bug 1431104), xref https://firefox-source-docs.mozilla.org/tools/lint/linters/eslint-plugin-mozilla.html#tests
Comment 8•7 years ago
|
||
Most of the Cu.import -> ChromeUtils.import replacements seem to be overlapping with a script that I believe Gijs was writing. I don't know how far he got (maybe try server stage). Also, why are we excluding devtools? We are now working on aligning the two configurations together (bug 1430596), so we should be doing that, unless there's a specific reason not to.
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #8) > Most of the Cu.import -> ChromeUtils.import replacements seem to be > overlapping with a script that I believe Gijs was writing. I don't know how > far he got (maybe try server stage). > > Also, why are we excluding devtools? We are now working on aligning the two > configurations together (bug 1430596), so we should be doing that, unless > there's a specific reason not to. Devtools is a special problem. Their modules don't have access to ChromeUtils by default, so they need more work to migrate. The Cu.import migration wasn't my main goal here. I wrote that part of the rule without really thinking about it, and since I knew Gijs was already working on it, I didn't want to waste duplicated effort on the non-trivial cases.
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 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8943820 [details] Bug 1431533: Part 1 - Cache module exports when a module is first loaded. https://reviewboard.mozilla.org/r/214174/#review220056 ::: js/xpconnect/loader/mozJSComponentLoader.h:68 (Diff revision 1) > static mozJSComponentLoader* Get() { return sSelf; } > > nsresult Import(const nsACString& aResourceURI, JS::HandleValue aTargetObj, > JSContext* aCx, uint8_t aArgc, JS::MutableHandleValue aRetval); > + > + nsresult Import(JSContext* aCx, const nsACString& aResourceURI, Maybe add a comment saying this version caches the module exports? Or give it a distinct name, like CachedImport? ::: js/xpconnect/loader/mozJSComponentLoader.cpp:274 (Diff revision 1) > nsCOMPtr<nsIURI> mResolvedURI; > }; > > +template <typename ...Args> > +static nsresult > +ReportOnCallerUTF8(JSCLContextHelper& helper, What's the goal of this change? Is it just code cleanup? It does seem unfortunate that we lose the MOZ_FORMAT_PRINTF, but I guess that's not a big deal for a function with such limited scope. ::: js/xpconnect/loader/mozJSComponentLoader.cpp:288 (Diff revision 1) > + if (!buf) { > + return NS_ERROR_OUT_OF_MEMORY; > + } > + > + helper.reportErrorAfterPop(Move(buf)); > + return NS_ERROR_INTERNAL_ERRORRESULT_EXCEPTION_ON_JSCONTEXT; Was this just a bug before that it returned NS_OK? ::: js/xpconnect/loader/mozJSComponentLoader.cpp:1109 (Diff revision 1) > vp.set(nullptr); > > + JS::RootedObject exports(cx); > + MOZ_TRY(Import(cx, aLocation, vp, &exports)); > + > + if (targetObj) { You should use an early return here. ::: js/xpconnect/loader/mozJSComponentLoader.cpp:1224 (Diff revision 1) > > MOZ_ASSERT(mod->obj, "Import table contains entry with no object"); > - vp.set(mod->obj); > + aModuleGlobal.set(mod->obj); > > - if (targetObj) { > + JS::RootedObject exports(aCx, mod->exports); > + if (!exports) { Just some food for thought, but it might be nice to spin out this gigantic block of code into a separate method at some point. ::: js/xpconnect/loader/mozJSComponentLoader.cpp:1326 (Diff revision 1) > #endif > } > + > + // Don't cache the exports object if any of its exported symbols are > + // missing. If the module hasn't finished loading yet, they may be > + // defined the next time we try to import it. So there are some modules that set properties that aren't done by the time it finishes executing the top level script?
Attachment #8943820 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943820 [details] Bug 1431533: Part 1 - Cache module exports when a module is first loaded. https://reviewboard.mozilla.org/r/214174/#review220056 > Maybe add a comment saying this version caches the module exports? Or give it a distinct name, like CachedImport? They both cache the module exports, at this point. The only real difference is that this version returns the exports object, but the other copies its properties. I make ImportInto() public and merge it with the other Import() method, though, and then change its callers. > What's the goal of this change? Is it just code cleanup? It does seem unfortunate that we lose the MOZ_FORMAT_PRINTF, but I guess that's not a big deal for a function with such limited scope. Yeah, sorry. I meant to put this cleanup into a separate patch, but I forgot to commit it before I added the other changes. I just couldn't follow what was going on in that code with the same 3-4 lines of boilerplate repeated every few lines. I agree that losing the MOZ_PRINTF_FORMAT is unfortunate. It seemed worth it for making the code so much easier to follow. The only other semi-reasonable solution I could think of was to add an infallible `GetLocation()` method. > Was this just a bug before that it returned NS_OK? Yeah, more or less. When it was called from XPConnect, the JSContext exception still got propagated to the caller even if we returned success. But I don't really want other callers of this function to have to check both the return value and the JSContext exception state after every call. > So there are some modules that set properties that aren't done by the time it finishes executing the top level script? Not that I know of, but it's a possibility with the current code, so it seemed best to play it safe for now.
![]() |
||
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8943820 [details] Bug 1431533: Part 1 - Cache module exports when a module is first loaded. https://reviewboard.mozilla.org/r/214174/#review220096 ::: js/xpconnect/loader/mozJSComponentLoader.h:69 (Diff revision 2) > + JS::MutableHandleObject aModuleGlobal, > + JS::MutableHandleObject aModuleExports, Could use docs, esp for what aModuleExports is.
![]() |
||
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8943821 [details] Bug 1431533: Part 2 - Add ChromeUtils.defineModuleGetter helper. https://reviewboard.mozilla.org/r/214176/#review220092 r=me ::: dom/base/ChromeUtils.cpp:434 (Diff revision 2) > + ExtractArgs(JSContext* aCx, JS::CallArgs& aArgs, > + JS::MutableHandle<JSObject*> aCallee, > + JS::MutableHandle<JSObject*> aThisObj, > + JS::MutableHandle<jsid> aId) > + { > + aCallee.set(&aArgs.calleev().toObject()); aCallee.set(&aArgs.callee()); ::: dom/base/ChromeUtils.cpp:445 (Diff revision 2) > + } > + > + aThisObj.set(&thisv.toObject()); > + > + JS::Rooted<JS::Value> id(aCx, js::GetFunctionNativeReserved(aCallee, SLOT_ID)); > + MOZ_ALWAYS_TRUE(JS_ValueToId(aCx, id, aId)); Why is this guaranteed to never fail? Atoms can get gced, and then you could OOM trying to atomize here. ::: dom/base/ChromeUtils.cpp:511 (Diff revision 2) > + JS::Rooted<jsid> id(aCx); > + if (!ExtractArgs(aCx, args, &callee, &thisObj, &id)) { > + return false; > + } > + > + JS::Rooted<JS::Value> value(aCx, args.get(0)); I don't think you need a Rooted temporary here. Just passing args.get(0), or storing in a `JS::Handle<JS::Value>` temporary would be fine. ::: dom/base/ChromeUtils.cpp:543 (Diff revision 2) > + if (!getter || !setter) { > + JS_ReportOutOfMemory(aCx); > + return false; > + } > + > + js::SetFunctionNativeReserved(getter, SLOT_ID, idValue); Better would be to store the actual id here. See js:IdToValue in jsfriendapi.h. Then you can actually assert that JS_ValueToId succeeds in ExtractArgs, since in the string case you really will have an atom. ::: dom/base/ChromeUtils.cpp:550 (Diff revision 2) > + > + js::SetFunctionNativeReserved(getter, SLOT_URI, uri); > + > + return JS_DefinePropertyById(aCx, aTarget, id, > + JS_DATA_TO_FUNC_PTR(JSNative, getter.get()), > + JS_DATA_TO_FUNC_PTR(JSNative, setter.get()), So this differs from defineLazyModuleGetter in terms of its setter behavior, right? ::: dom/base/ChromeUtils.cpp:553 (Diff revision 2) > + return JS_DefinePropertyById(aCx, aTarget, id, > + JS_DATA_TO_FUNC_PTR(JSNative, getter.get()), > + JS_DATA_TO_FUNC_PTR(JSNative, setter.get()), > + JSPROP_GETTER | JSPROP_SETTER | JSPROP_ENUMERATE); > + > + return true; This return statement is unreachable. ::: dom/webidl/ChromeUtils.webidl:285 (Diff revision 2) > + * - The getter property may be overwritten by simple assignment, but as > + * with imports, the new property value is always defined on the `this` > + * object that the setter is called with. > + * > + * - If the module import fails, the getter will throw an error, and the > + * property will not replaced. Each subsequent attempt to access the "will not be" ::: js/xpconnect/tests/unit/test_defineModuleGetter.js:55 (Diff revision 2) > + > + equal(obj.Services, temp.Services, "Getter works on object"); > + assertIsValue(obj, "Services", temp.Services); > + > + > + // Test overwriting via setter Should this set up a new "obj" and "child"?
Attachment #8943821 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 21•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943821 [details] Bug 1431533: Part 2 - Add ChromeUtils.defineModuleGetter helper. https://reviewboard.mozilla.org/r/214176/#review220092 > Why is this guaranteed to never fail? Atoms can get gced, and then you could OOM trying to atomize here. Oops. Good catch. Originally I stored the ID value here rather than the original string value. > So this differs from defineLazyModuleGetter in terms of its setter behavior, right? I guess it does. I thought our lazy getters also had setters, but apparently they don't. The other difference in behavior is that defineLazyModuleGetter always redefines the getter property on the object it was originally passed. This version defines it on the `this` object, because I already needed both reserved slots, and didn't want to allocate a holder object just for that. > Should this set up a new "obj" and "child"? Oh, I guess it should setup a new child. It shouldn't really need a new `obj`.
![]() |
||
Comment 22•7 years ago
|
||
> Originally I stored the ID value here Right, let's keep doing that. ;) I think having the setter is fine. > This version defines it on the `this` object Seems like it should be fine in all sane cases....
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8943822 [details] Bug 1431533: Part 3 - Define ChromeUtils on chrome-privileged Sandboxes that need it. https://reviewboard.mozilla.org/r/214178/#review220340
Attachment #8943822 -
Flags: review?(mixedpuppy) → review+
Comment 25•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5) > Note that this rewriting produces odd indenation in some cases for modules > which don't define indentation rules for function arguments. For modules with > appropriate indentation rules, the indentation is automatically adjusted > during rewriting. How much effort is it to set appropriate rules (even temporarily) to not break the indent (at least in browser/ and toolkit/)? If it's difficult, I'm willing to provide an xpcshell script doing the XPCOMUtils.defineLazyModuleGetter -> ChromeUtils.defineModuleGetter rewrite without breaking the indent.
Flags: needinfo?(kmaglione+bmo)
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8943824 [details] Bug 1431533: Part 4b - Run ESLint fix for ChromeUtils import rule. https://reviewboard.mozilla.org/r/214182/#review220608 This breaks the indent at least in browser/base/content and toolkit/components/jsdownloads. We can and should do better.
Attachment #8943824 -
Flags: review?(florian) → review-
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8943823 [details] Bug 1431533: Part 4 - Add ESLint support for ChromeUtils import methods. https://reviewboard.mozilla.org/r/214180/#review220592 Looks good to me. ::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-chromeutils-import.js:57 (Diff revision 2) > + > + // Is the expression starting with `Cu` or `Components.utils`? > + let isCu = ((!allowCu && isIdentifier(callee.object, "Cu")) || > + isMemberExpression(callee.object, "Components", "utils")); > + > + if (isCu && isIdentifier(callee.property, "import")) { nit: I would just inline the implementation of isCu here
Attachment #8943823 -
Flags: review?(florian) → review+
Assignee | ||
Comment 28•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #25) > How much effort is it to set appropriate rules (even temporarily) to not > break the indent (at least in browser/ and toolkit/)? > > If it's difficult, I'm willing to provide an xpcshell script doing the > XPCOMUtils.defineLazyModuleGetter -> ChromeUtils.defineModuleGetter rewrite > without breaking the indent. Setting up the rules is not difficult. The difficulty is that a lot of existing code would fail them, and wind up getting reformatted in the process. I'm OK with that, but I'd rather do it in a separate bug, which is why I didn't put more effort into it here. We can fix the indentation issues in the ESLint fixer, without an xpcshell script, but I think relying on the eslint indent rules should be preferable.
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8943825 [details] Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite. https://reviewboard.mozilla.org/r/214198/#review220604 Looks ok to me, although I don't really understand in which case ChromeUtils isn't usable, and would like to understand. ::: toolkit/modules/Promise-backend.js:58 (Diff revision 2) > var Components_ = this.require ? require("chrome").components : Components; > > // If Cu is defined, use it to lazily define the FinalizationWitnessService. > if (Cu) { > - ChromeUtils.import("resource://gre/modules/Services.jsm"); > - ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); > + /* eslint "mozilla/use-chromeutils-import": ["error", {allowCu: true}] */ > + Cu.import("resource://gre/modules/Services.jsm"); Why is ChromeUtils not usable here?
Attachment #8943825 -
Flags: review?(florian) → review+
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8944036 [details] Bug 1431533: Part 5 - Update non-ESLint files to use ChromeUtils.defineModuleGetter. https://reviewboard.mozilla.org/r/214362/#review220610 The scripts I wrote for bug 1421992 handle preserving the indent, and should be easy to adapt for this. They are available in https://bitbucket.org/fqueze/xpcshell-rewrites/commits/d3b509b6d80d24c8e7154e699c120126220c4acb
Attachment #8944036 -
Flags: review?(florian) → review-
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943825 [details] Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite. https://reviewboard.mozilla.org/r/214198/#review220604 > Why is ChromeUtils not usable here? Because this is used both as a JSM and as a devtools module. It's technicaly possible to use ChromeUtils from devtools modules, but it's more complicated, since it requires explicitly importing it. That's something we're going to need to handle eventually, since I want to remove support for Cu.import. But it's a much biggest bug, so I'd prefer to handle it separately.
Comment 32•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #31) > Comment on attachment 8943825 [details] > Bug 1431533: Part 4c - Fix ESLint errors left over after rewrite. > > https://reviewboard.mozilla.org/r/214198/#review220604 > > > Why is ChromeUtils not usable here? > > Because this is used both as a JSM and as a devtools module. It's technicaly > possible to use ChromeUtils from devtools modules, but it's more > complicated, since it requires explicitly importing it. > > That's something we're going to need to handle eventually, since I want to > remove support for Cu.import. But it's a much biggest bug, so I'd prefer to > handle it separately. Sounds like a good reason, but could do with a comment in the code to explain it. At this point it seems moving Promise-backend.js and Promise.jsm to devtools would be easy. But clearly outside the scope of this bug.
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #32) > Sounds like a good reason, but could do with a comment in the code to > explain it. At this point it seems moving Promise-backend.js and Promise.jsm > to devtools would be easy. But clearly outside the scope of this bug. Agreed on all three points. I'll file a follow-up for that.
Comment 34•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #28) > We can fix the indentation issues in the ESLint fixer, without an xpcshell > script, but I think relying on the eslint indent rules should be preferable. In some ways I'd love to do this, but I think there's still concerns around it for developers, and at the moment I'm thinking we should head towards something like prettier (run like clang-format will be). Hence, at the moment I'm avoiding the indent rule until the picture for prettier becomes clearer. The short route will probably be for fixing it with xpcshell. We can keep the eslint fixer that you've implemented for developers to use though.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8943824 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8944036 -
Attachment is obsolete: true
Comment 41•7 years ago
|
||
mozreview-review |
Comment on attachment 8943825 [details] Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite. https://reviewboard.mozilla.org/r/214198/#review220834 ::: toolkit/modules/Promise-backend.js:57 (Diff revision 3) > // not be available (see above), users of this must check it first. > var Components_ = this.require ? require("chrome").components : Components; > > // If Cu is defined, use it to lazily define the FinalizationWitnessService. > if (Cu) { > - ChromeUtils.import("resource://gre/modules/Services.jsm"); > + /* eslint "mozilla/use-chromeutils-import": ["error", {allowCu: true}] */ So are you adding a comment here saying that this is for devtools?
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8944959 [details] Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. https://reviewboard.mozilla.org/r/215100/#review220832 mozreview makes it very difficult to review patches of this size, so I applied it locally and looked at a sample of modified files (focusing mostly on files that have the largest number of lines changed). I found no issue caused by the script, so r=me. ::: browser/base/content/browser.js:1312 (Diff revision 1) > BrowserPageActions.init(); > gAccessibilityServiceIndicator.init(); > > if (window.matchMedia("(-moz-os-version: windows-win8)").matches && > window.matchMedia("(-moz-windows-default-theme)").matches) { > - let windowFrameColor = new Color(...Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {}) > + let windowFrameColor = new Color(...ChromeUtils.import("resource:///modules/Windows8WindowFrameColor.jsm", {}) This could do with being reformated by hand to: let windowFrameColor = new Color(...ChromeUtils.import(... .Windows8WindowFrameColor.get());
Attachment #8944959 -
Flags: review?(florian) → review+
Assignee | ||
Comment 43•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943825 [details] Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite. https://reviewboard.mozilla.org/r/214198/#review220834 > So are you adding a comment here saying that this is for devtools? Yup. I hadn't forgotten, but mccr8 also just landed a patch that touched this file, and I was waiting for that to merge before I touched it again.
Assignee | ||
Comment 44•7 years ago
|
||
Note that this push has some ESLint failures in devtools. Those errors weren't introduced by those patches. The fact that those files were touched just triggered the failures for existing issues.
Assignee | ||
Comment 45•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73b978e8f267a34716d19793229193288b7e3f8 Bug 1431533: Part 1 - Cache module exports when a module is first loaded. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/71891e3bf5b0ce4ff781e4f5538c3026e36d9a6d Bug 1431533: Part 2 - Add ChromeUtils.defineModuleGetter helper. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/ceba2147b1eaaef701405f426478261ed30ee9e2 Bug 1431533: Part 3 - Define ChromeUtils on chrome-privileged Sandboxes that need it. r=mixedpuppy https://hg.mozilla.org/integration/mozilla-inbound/rev/fbef09c3e8af1653f270db589f73f2786c23ddc2 Bug 1431533: Part 4 - Add ESLint support for ChromeUtils import methods. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a7c018420e408fbe0a13ffddd2861623fda5a7 Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/d48657228a0b84cf3a37a35227ca3b3d0bd2a017 Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite. r=florian
Assignee | ||
Comment 46•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb369804a51e7665c0b44d3778681ca132cb1c2c Bug 1431533: Follow-up: Fix no-single-arg-cu-import exemption to allow ChromeUtils. r=bustage CLOSED TREE
Assignee | ||
Comment 47•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc87ad81ff86109c7ea0187424fa9a8ed3b4af6c Backed out 3 changesets (bug 1431533) for Android mochitest bustage. CLOSED TREE
Assignee | ||
Comment 48•7 years ago
|
||
It looks like we're going to need to update the host-utils for Android with the first 3 parts applied before we can apply the rest. Otherwise the mochitest server fails when it tries to load modules that rely on ChromeUtils import methods.
Keywords: leave-open
![]() |
||
Comment 49•7 years ago
|
||
The first follow-up causes the eslint plugin mozilla test suite to permafail: Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bb369804a51e7665c0b44d3778681ca132cb1c2c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158382198&repo=mozilla-inbound > TEST-UNEXPECTED-FAIL | test-no-single-arg-cu-import.js | ChromeUtils.import('fake'); | Should have 1 error but had 0: []
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 50•7 years ago
|
||
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #49) > The first follow-up causes the eslint plugin mozilla test suite to permafail: Oh, sorry, I didn't realize there was a test for that behavior. I'll back out that follow-up and fix it when I re-land the ESLint rules.
Flags: needinfo?(kmaglione+bmo)
Comment 51•7 years ago
|
||
mozreview-review |
Comment on attachment 8943823 [details] Bug 1431533: Part 4 - Add ESLint support for ChromeUtils import methods. https://reviewboard.mozilla.org/r/214180/#review221444 This new rule should be documented in https://searchfox.org/mozilla-central/source/tools/lint/docs/linters/eslint-plugin-mozilla.rst ::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js:64 (Diff revision 3) > "use-default-preference-values": > require("../lib/rules/use-default-preference-values"), > "use-ownerGlobal": require("../lib/rules/use-ownerGlobal"), > "use-services": require("../lib/rules/use-services"), > - "var-only-at-top-level": require("../lib/rules/var-only-at-top-level") > + "var-only-at-top-level": require("../lib/rules/var-only-at-top-level"), > + "use-chromeutils-import": require("../lib/rules/use-chromeutils-import") nit: This list is sorted alphabetically. ::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js:90 (Diff revision 3) > "reject-some-requires": "off", > "use-default-preference-values": "off", > "use-ownerGlobal": "off", > "use-services": "off", > - "var-only-at-top-level": "off" > + "var-only-at-top-level": "off", > + "use-chromeutils-import": "off" This one too.
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 53•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4dcdb0f429003100e2727dad4108ab57087da2 Bug 1431533: Part 4 - Add ESLint support for ChromeUtils import methods. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/12fc4dee861c812fd2bd032c63ef17af61800c70 Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/6e56f4c8843ed134b2dba34fb007298fd55be966 Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite. r=florian
Comment 54•7 years ago
|
||
Backed out changeset 8e4dcdb0f429 (bug 1431533)(part 4) for ESlint failure on test-no-single-arg-cu-import.js Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6e56f4c8843ed134b2dba34fb007298fd55be966&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=159216038 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=159216038&repo=mozilla-inbound&lineNumber=208 /builds/worker/checkouts/gecko/mobile/android/tests/browser/robocop/robocop_head.js:701:13 | Please use ChromeUtils.import instead of Cu.import (mozilla/use-chromeutils-import) Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b467721963f8a5d3a2987199c2ee348ba59dff3
Flags: needinfo?(kmaglione+bmo)
Comment 55•7 years ago
|
||
Backed out 2 changesets (bug 1431533) (part 5a and 5b) for ESlint failures on ContentControl.jsm:74:5 Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6e56f4c8843ed134b2dba34fb007298fd55be966&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=159216038 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/505fc52bd6080c95bf27458a5753340b326f0fa1
Assignee | ||
Comment 56•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2674287e57fbf6835729f42b8491359c2c4bfd5 Bug 1431533: Part 4 - Add ESLint support for ChromeUtils import methods. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/34c999fa006bffe8705cf50c54708aa21a962e62 Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/a1eca62826a1341ca24d4d2a93d4884d4fc7ad51 Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite. r=florian
Comment 57•7 years ago
|
||
Backed out 3 changesets (bug 1431533) for Android mochitest failures on testEventDispatcher Treeherder link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a1eca62826a1341ca24d4d2a93d4884d4fc7ad51&filter-searchStr=1e9f4b44b8e31ad0ecb948ce8e1ac6f7520eea4a&selectedJob=159254840 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=159254840&repo=mozilla-inbound&lineNumber=1860 Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e0fe339cb1e7c0c029e68be5332e52f53ab6b7
Comment 58•7 years ago
|
||
But not just for Android mochitests, also for talos "crashes" (I don't think much of talos crashing) like https://treeherder.mozilla.org/logviewer.html#?job_id=159254709&repo=mozilla-inbound and timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=159254724&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=159254703&repo=mozilla-inbound
Assignee | ||
Comment 59•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5d35ff63dc96789fb3e7ae011d938ff6c08a9c Bug 1431533: Part 4 - Add ESLint support for ChromeUtils import methods. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a7b5e11ba856ee3535f76c6bcca17ea29e3d5f Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ed0cab6e2bb6f1a5d7b32e16f097f38183bf33 Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite. r=florian
![]() |
||
Comment 60•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb5d35ff63dc https://hg.mozilla.org/mozilla-central/rev/e6a7b5e11ba8 https://hg.mozilla.org/mozilla-central/rev/c6ed0cab6e2b
Comment 61•7 years ago
|
||
Comment 62•7 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/af27a98d8027 Part 5z - Use ChromeUtils import methods at positions highlighted by eslint after merge. a=eslint-fix
Comment hidden (Intermittent Failures Robot) |
Comment 64•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af27a98d8027
Comment 65•7 years ago
|
||
Given this has landed, should we close this? Separately, Kris, were you planning to post to fx-dev and m.d.platform about this? If not I can do it, if you don't mind - think it'll be good to give people a heads-up...
Assignee | ||
Comment 66•7 years ago
|
||
(In reply to :Gijs from comment #65) > Separately, Kris, were you planning to post to fx-dev and m.d.platform about > this? If not I can do it, if you don't mind - think it'll be good to give > people a heads-up... Yes, I was planning to send an email about this, the HTML sanitization changes, and probably also mention mccr8's Cc/Ci/... changes.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(kmaglione+bmo)
Keywords: leave-open
Resolution: --- → FIXED
Comment hidden (Intermittent Failures Robot) |
Comment 68•7 years ago
|
||
Comment 69•7 years ago
|
||
Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/d8d6583ce837214592cbd60a2429e1635c9fb00f chore(package): Upgrade babel to support ChromeUtils (for Bug 1431533) (#3967)
Comment 70•7 years ago
|
||
Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/be0d131b4d49b1a63e2ad24be2deec6f8e15cddc chore(mc): Port Bug 1431533 - Add ChromeUtils helpers for lazy module import (#3959) * chore(package): Update eslint-plugin-mozilla to 0.7.0 for ChromeUtils * Port Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian https://hg.mozilla.org/mozilla-central/rev/e6a7b5e11ba8 * Port Bug 1431533: Part 5z - Use ChromeUtils import methods at positions highlighted by eslint after merge. a=eslint-fix https://hg.mozilla.org/mozilla-central/rev/af27a98d8027 * chore(tests): Stub out ChromeUtils to get tests passing * chore(perf): Correctly check for ChromeUtils before importing Cleanup of unused / unnecessary Components aliasing to be done via porting bug 1432992.
status-firefox60:
--- → fixed
Target Milestone: --- → mozilla60
Updated•7 years ago
|
Whiteboard: [stockwell fixed:other]
Comment 71•7 years ago
|
||
It sure would be nice to get ChromeUtils documented in devmo... I'm looking at FF60 for the first time, and this is the first I've heard of it.
Keywords: dev-doc-needed
Assignee | ||
Comment 72•7 years ago
|
||
(In reply to Alex Vincent [:WeirdAl] from comment #71) > It sure would be nice to get ChromeUtils documented in devmo... I'm looking > at FF60 for the first time, and this is the first I've heard of it. We don't document browser internals on MDN anymore.
Comment 73•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #72) > (In reply to Alex Vincent [:WeirdAl] from comment #71) > > It sure would be nice to get ChromeUtils documented in devmo... I'm looking > > at FF60 for the first time, and this is the first I've heard of it. > > We don't document browser internals on MDN anymore. We are hopefully getting someone on board the team soon to handle Firefox docs (such as build tools and processes, devtools, webext extensions, etc.) I'll start recording a list of bugs like this one that are no longer relevant to MDN, but might be appropriate for them to handle in the future.
You need to log in
before you can comment on or make changes to this bug.
Description
•