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

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

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 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1403489 - remove special loader rule for devtools/shared/platform;

Review commit: https://reviewboard.mozilla.org/r/183962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183962/

Comment 2

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d4caa19662bbc0e1e716d46d205ba4990bfd22a

Comment 3

[Julian Descottes [:jdescottes]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183962/diff/1-2/

Comment 4

[Julian Descottes [:jdescottes]]

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.

Comment 5

[Tom Tromey :tromey]

(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.

Comment 6

[Julian Descottes [:jdescottes]]

(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

[Tom Tromey :tromey]

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.

Comment 8

[Julian Descottes [:jdescottes]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183962/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e10a7083e7a7
remove special loader rule for devtools/shared/platform;r=tromey

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e10a7083e7a7

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 12

[Julian Descottes [:jdescottes]]

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.