Closed Bug 1436303 Opened 2 years ago Closed 2 years ago

Remove definitions of Ci, Cr, Cc, and Cu for devtools/

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: standard8, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

As has been done in bug 1432992 we should look to removing the definitions of Ci/Cr/CC and Cu for devtools.

Note that bug 1230369 is adding a rule for ESLint (though not all of devtools is covered at the moment) which will cover ensuring these definitions aren't re-added.
Priority: -- → P3
Is this about removing all the require("chrome") that are solely about importing: Cc, Ci, Cu, Cr, Cm and CC?
Or is this only about a subset of this list?

require("chrome") exposes all these symbols: Cc, Ci, Cu, Cr, Cm, CC, components and ChromeWorker.
  https://searchfox.org/mozilla-central/source/devtools/shared/base-loader.js#614-616
Flags: needinfo?(standard8)
So my current thinking is this might not apply to devtools in the same way.

Bug 767640 defined Cc/Ci/Cr/Cu in the chrome scope so that we don't need all the individual definitions. We removed all the definitions from non-devtools code, and created an ESLint rule for it.

The reason I now think this might not be applicable to all of devtools, is that you seem to have your own sandbox that you are loading files in, and therefore you do need to import these manually.

So maybe, base-loader.js, and any other devtools files directly loaded in chrome scope (e.g. tests), should have these definitions removed, but leave the rest.

Please correct me if I'm understanding wrongly for any of these.
Flags: needinfo?(standard8)
(In reply to Mark Banner (:standard8) from comment #2)
> So my current thinking is this might not apply to devtools in the same way.
> 
> Bug 767640 defined Cc/Ci/Cr/Cu in the chrome scope so that we don't need all
> the individual definitions. We removed all the definitions from non-devtools
> code, and created an ESLint rule for it.
> 
> The reason I now think this might not be applicable to all of devtools, is
> that you seem to have your own sandbox that you are loading files in, and
> therefore you do need to import these manually.

Exactly. Module sandboxes don't get direct access to these symbols.
But this decision is kind of historical. It comes from decisions taken in Jetpack/Addon-SDK!

> So maybe, base-loader.js, and any other devtools files directly loaded in
> chrome scope (e.g. tests), should have these definitions removed, but leave
> the rest.

It may be a quite tedious work to distinguish what is a module from what is not (JSM/xpcom/framescript/...).


It may be easier to expose these symbols to devtools scope.

The original reason for not exposing them was to allow to make a clear distinction between Addon modules that were privileged (i.e. using chrome via Cc, Ci, ...) versus the others that were not using such internals and instead only importing other modules. If an addon was not having a single module requiring the chrome module, it would mean that it is only using high level SDK modules and was subject to a more lightweight code review on Addons website.
But this was never really used and doesn't apply to DevTools.

Now, it may help DevTools on some other angle. The "De-chromization". In order to run our tools in launchpad, as a regular website we should stop using require("chrome")/Cc,Ci,... Keeping the explicit import help a bit identifying the modules that have to be refactored to not use any privileged API. Otherwise you have to grep for all of these symbols, which may be hard given their shortness.

Is it worth keeping require("chrome") pattern and be different from the rest of mozilla for that? (or some other reason?)

Julian, Jryans, any opinion about that?
Flags: needinfo?(jryans)
Flags: needinfo?(jdescottes)
(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Now, it may help DevTools on some other angle. The "De-chromization". In
> order to run our tools in launchpad, as a regular website we should stop
> using require("chrome")/Cc,Ci,... Keeping the explicit import help a bit
> identifying the modules that have to be refactored to not use any privileged
> API. Otherwise you have to grep for all of these symbols, which may be hard
> given their shortness.
> 
> Is it worth keeping require("chrome") pattern and be different from the rest
> of mozilla for that? (or some other reason?)

Because DevTools occasionally considers running our code in non-Firefox environments, I think it's easier to continue explicitly importing chrome usages in DevTools so that it can be rewritten / replaced by the loader if needed.  It also simplifies locating chrome usages like Alex mentioned.

However, that's just my observation, and I would be okay with going either way.
Flags: needinfo?(jryans)
I would be in favor of keeping require("chrome"), for the reason mentioned by :jryans and also because it is more consistent with  how DevTools modules load their dependencies. We are already different from the rest of mozilla-central because of our loader.
Flags: needinfo?(jdescottes)
Ok ok. Looks like we have a clear concensus on the fact that identifying privileged usages are still relevant for devtools.

I'm no eslint expert but we should have some kind of whitelist for all-but-modules (framescript,xpcom,jsm). A whitelist, because.we mostly have modules. Do we have such list? I imagine not... So should we put such list in /.eslintrc.js under use-cc-etc whitelist ?
An override in .eslintrc.js sounds good here. And for tests we have .eslintrc.mochitest.js and .eslintrc.xpcshell.js where we should be able to just enable the rule?
(In reply to Julian Descottes [:jdescottes][:julian] from comment #7)
> An override in .eslintrc.js sounds good here. And for tests we have
> .eslintrc.mochitest.js and .eslintrc.xpcshell.js where we should be able to
> just enable the rule?

It is probably just easier to do the turning off per file in one place, as otherwise it'll be turned off in one place, then on again and I could see some of that getting confusing and probably broken...

You can choose files to include or exclude in a single override:

https://eslint.org/docs/user-guide/configuring#example-configuration
(In reply to Mark Banner (:standard8) from comment #8)
> It is probably just easier to do the turning off per file in one place, as
> otherwise it'll be turned off in one place, then on again and I could see
> some of that getting confusing and probably broken...
> 
> You can choose files to include or exclude in a single override:
> 
> https://eslint.org/docs/user-guide/configuring#example-configuration

Ah good point, for some reason I thought tests were not impacted by the topmost eslintrc file. I was playing around with the rule and it seems it doesn't detect destructuring:

  const { utils: Cu } = Components;

will not raise any violation for instance.
As discussed on IRC, the rule to use is "mozilla/no-define-cc-etc" and not "mozilla/use-cc-etc". It is currently disabled for devtools at https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/.eslintrc.js#43-50
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8984460 [details]
Bug 1436303 - Enable mozilla/no-define-cc-etc for some DevTools files;

https://reviewboard.mozilla.org/r/250292/#review257082

Here is a couple of additional files we may want to whitelist:
* frame scripts:
devtools/server/startup/frame.js
devtools/server/startup/content-process.js
devtools/client/jsonview/converter-observer.js
devtools/server/actors/webconsole/content-process-forward.js
devtools/server/actors/worker/service-worker-process.js
devtools/client/responsive.html/browser/content.js
devtools/client/inspector/shared/test/doc_frame_script.js
devtools/client/inspector/animation/test/doc_frame_script.js
devtools/client/inspector/animation-old/test/doc_frame_script.js
devtools/client/inspector/rules/test/doc_frame_script.js
devtools/client/jsonview/test/doc_frame_script.js
devtools/client/debugger/test/mochitest/code_frame-script.js
* jsm without .jsm extension
devtools/shared/worker/worker.js
devtools/shared/worker/loader.js
* xpcoms
devtools/startup/devtools-startup.js
devtools/startup/aboutdevtoolstoolbox-registration.js
devtools/startup/aboutdevtools/aboutdevtools-registration.js
devtools/startup/aboutdebugging-registration.js

Otherwise, this first chunk looks good to me.
Attachment #8984460 - Flags: review?(poirot.alex) → review+
Product: Firefox → DevTools
Comment on attachment 8984460 [details]
Bug 1436303 - Enable mozilla/no-define-cc-etc for some DevTools files;

https://reviewboard.mozilla.org/r/250292/#review257194
Attachment #8984460 - Flags: review?(standard8) → review+
Thanks for the reviews, try https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce1bf23598019a854604295edf35ffe0344b04c

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Comment on attachment 8984460 [details]
> Bug 1436303 - Enable mozilla/no-define-cc-etc for some DevTools files;
> 
> https://reviewboard.mozilla.org/r/250292/#review257082
> 
> Here is a couple of additional files we may want to whitelist:

That's slightly more than a couple :) I just removed devtools/shared/worker/worker.js which we also load as a module.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd095fd7a7fa
Enable mozilla/no-define-cc-etc for some DevTools files;r=ochameau,standard8
https://hg.mozilla.org/mozilla-central/rev/bd095fd7a7fa
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.