Simplify handling of "devtools/shared/platform" in Loader.jsm

RESOLVED FIXED in Firefox 58

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

unspecified
Firefox 58
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

See http://searchfox.org/mozilla-central/source/devtools/shared/Loader.jsm#38 
This was introduced in Bug 1287910. 

I think the intent is to have non-chrome privileged versions of the files in  devtools/shared/platform/chrome. As far as I can tell the files in the devtools/shared/platform/content are only used by webpack/launchpad files, where a special alias rule is needed anyway since webpack doesn't use our loader.

I think we can get rid of the rule in Loader.jsm, move the real implementation files from devtools/shared/platform/chrome to devtools/shared/platform and finally move the content shims to a dedicated shim/webpack folder.
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
Tom: I only kept the chrome versions of our modules in devtools/shared/platform/ and moved the content files next to the other webpack shims, as I believe they are only used in this scenario.

One thing that is a bit disturbing is that the shims are tested using xpcshell & mochi tests, which means that we package the shims with Firefox. Not sure if there is an easy way to keep the tests without packaging the files anymore.
Assignee

Updated

2 years ago
Depends on: 1403175
(In reply to Julian Descottes [:jdescottes] from comment #4)
> Tom: I only kept the chrome versions of our modules in
> devtools/shared/platform/ and moved the content files next to the other
> webpack shims, as I believe they are only used in this scenario.
> 
> One thing that is a bit disturbing is that the shims are tested using
> xpcshell & mochi tests, which means that we package the shims with Firefox.
> Not sure if there is an easy way to keep the tests without packaging the
> files anymore.

I wonder if the shims should all just be moved to launchpad or elsewhere in devtools-core.
They don't really make much sense in M-C for the long term.
Assignee

Comment 6

2 years ago
(In reply to Tom Tromey :tromey from comment #5)
> (In reply to Julian Descottes [:jdescottes] from comment #4)
> > Tom: I only kept the chrome versions of our modules in
> > devtools/shared/platform/ and moved the content files next to the other
> > webpack shims, as I believe they are only used in this scenario.
> > 
> > One thing that is a bit disturbing is that the shims are tested using
> > xpcshell & mochi tests, which means that we package the shims with Firefox.
> > Not sure if there is an easy way to keep the tests without packaging the
> > files anymore.
> 
> I wonder if the shims should all just be moved to launchpad or elsewhere in
> devtools-core.
> They don't really make much sense in M-C for the long term.

Yes that's something that might make sense for these two. With shims I'm a bit reluctant to do it, as they usually only "mock" methods well enough to make their consumers work. So if the consumer changes the way it uses a given module, the shim can need updating.

Here though I feel like they could live in devtools-core just fine.

Comment 7

2 years ago
mozreview-review
Comment on attachment 8912633 [details]
Bug 1403489 - remove special loader rule for devtools/shared/platform;

https://reviewboard.mozilla.org/r/183962/#review189242

Thanks for the patch.

I think this is fine, but please see the note about the npm-shrinkwrap.json change.

::: npm-shrinkwrap.json:538
(Diff revision 2)
>        }
>      },
>      "glob": {
>        "version": "7.1.2",
>        "resolved": "https://registry.npmjs.org/glob/-/glob-7.1.2.tgz",
> -      "integrity": "sha512-MJTUg1kjuLeQCJ+ccE4Vpa6kKVXkPYJ2mOCQyUuKLcLQsdrMCpBPUi8qVE6+YuaJkozeA9NusTAw3hLr8Xe5EQ==",
> +      "integrity": "sha1-wZyd+aAocC1nhhI4SmVSQExjbRU=",

I don't know what this file is.  I sort of suspect this isn't supposed to be here.
Attachment #8912633 - Flags: review?(ttromey) → review+
Comment hidden (mozreview-request)

Comment 9

2 years ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e10a7083e7a7
remove special loader rule for devtools/shared/platform;r=tromey

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e10a7083e7a7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Comment 11

2 years ago
mozreview-review
Comment on attachment 8912633 [details]
Bug 1403489 - remove special loader rule for devtools/shared/platform;

https://reviewboard.mozilla.org/r/183962/#review189508

::: devtools/client/inspector/webpack.config.js:104
(Diff revision 3)
>          "devtools/shared/DevToolsUtils":
>            path.join(__dirname, "./webpack/devtools-utils-sham.js"),
>          "devtools/shared/locales": path.join(__dirname, "../../shared/locales/en-US"),
> -        "devtools/shared/platform": path.join(__dirname, "../../shared/platform/content"),
> +        "devtools/shared/platform/clipboard": path.join(__dirname,
> +          "../../client/shared/webpack/shims/platform-clipboard-stub"),
> +        "devtools/shared/platform/stack": path.join(__dirname,

Looks like this should be "platform-stack-stub" instead?
Assignee

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8912633 [details]
Bug 1403489 - remove special loader rule for devtools/shared/platform;

https://reviewboard.mozilla.org/r/183962/#review189508

> Looks like this should be "platform-stack-stub" instead?

Ah indeed! Will push a follow up. The inspector webpack workflow does not work at the moment so should not impact anyone.
Assignee

Updated

2 years ago
Depends on: 1403831

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.