Closed Bug 1431533 Opened 6 years ago Closed 6 years ago

Add ChromeUtils helpers for lazy module import

Categories

(Core :: XPConnect, enhancement, P2)

Product:
Core
Component:
XPConnect
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
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)

Bug 1431533: Part 1 - Cache module exports when a module is first loaded.
59 bytes, text/x-review-board-request
mccr8
: review+
Details
Bug 1431533: Part 2 - Add ChromeUtils.defineModuleGetter helper.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1431533: Part 3 - Define ChromeUtils on chrome-privileged Sandboxes that need it.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1431533: Part 4 - Add ESLint support for ChromeUtils import methods.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1431533: Part 5b - Fix ESLint errors left over after rewrite.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods.
59 bytes, text/x-review-board-request
florian
: review+
Details
Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3959
52 bytes, text/x-github-pull-request
Details | Review
Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3967
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.
Component: General → XPConnect
Product: Firefox → Core
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
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.
Priority: -- → P2
(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 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+
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 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 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+
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`.
> 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....
Blocks: 1425611
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+
(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 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 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+
(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 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+
Flags: needinfo?(kmaglione+bmo)
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-
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.
(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.
(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.
(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.
Attachment #8943824 - Attachment is obsolete: true
Attachment #8944036 - Attachment is obsolete: true
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 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+
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.
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.
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
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc87ad81ff86109c7ea0187424fa9a8ed3b4af6c
Backed out 3 changesets (bug 1431533) for Android mochitest bustage. CLOSED TREE
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
Depends on: 1433075
(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)
Depends on: 1433279
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.
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
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)
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
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
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
Depends on: 1434543
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...
(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: 6 years ago
Flags: needinfo?(kmaglione+bmo)
Keywords: leave-open
Resolution: --- → FIXED
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)
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.
Blocks: 1436961
status-firefox60: --- → fixed
Target Milestone: --- → mozilla60
Whiteboard: [stockwell fixed:other]
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
(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.
See Also: → 1446558
(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.
Blocks: 1502646
Blocks: 1762601
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: