Closed
Bug 1436303
Opened 6 years ago
Closed 6 years ago
Remove definitions of Ci, Cr, Cc, and Cu for devtools/
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
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.
Updated•6 years ago
|
Priority: -- → P3
Comment 1•6 years ago
|
||
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)
Reporter | ||
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
(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)
Assignee | ||
Comment 5•6 years ago
|
||
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)
Comment 6•6 years ago
|
||
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 ?
Assignee | ||
Comment 7•6 years ago
|
||
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?
Reporter | ||
Comment 8•6 years ago
|
||
(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
Assignee | ||
Comment 9•6 years ago
|
||
(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.
Assignee | ||
Comment 10•6 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
try https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9789b9392d3fb382cdb9abc384cc03289cc00e7
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment 13•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Product: Firefox → DevTools
Reporter | ||
Comment 14•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•6 years ago
|
||
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.
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bd095fd7a7fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in
before you can comment on or make changes to this bug.
Description
•