Closed Bug 1312041 Opened 3 years ago Closed 3 years ago

Use loader paths for locales instead of appending chrome://

Categories

(DevTools :: General, defect, P1)

49 Branch
defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Status
firefox52 --- fixed

People

(Reporter: jryans, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files)

In bug 1294220, `requireRawId` was changed to prepend "chrome://" for properties files.

This complicates thinking about which files are accessible via the loader because it bypasses the list of paths we would normally use.

As far as I can tell, this could be removed and replaced with some kind of path mapping:

"devtools-shared/locale" -> "chrome://devtools-shared/locale"
"devtools/locale" -> "chrome://devtools/locale"
"toolkit/locale" -> "chrome://global/locale"

Such a mapping seems like it would work for both the DevTools loader and Webpack.

We could even go a bit further with module IDs, so they are closer to paths on disk, which is usually easier to understand overall:

"devtools/client/locales" -> "chrome://devtools/locale"
"devtools/shared/locales" -> "chrome://devtools-shared/locale"
"toolkit/locales" -> "chrome://global/locale"
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 7
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [devtools-html][triage] → [devtools-html]
Comment on attachment 8804560 [details]
Bug 1312041 - make loader-plugin-raw.jsm eslint-clean;

https://reviewboard.mozilla.org/r/88478/#review87780

Thanks for the cleanup!

::: devtools/shared/loader-plugin-raw.jsm:16
(Diff revision 1)
>   * A function that can be used as part of a require hook for a
>   * loader.js Loader.  This function only handles webpack-style "raw!"
>   * requires; other requires should not be passed to this.  See
>   * https://github.com/webpack/raw-loader.
>   */
> -function requireRawId(id, require) {
> +function requireRawId(id, require) { // eslint-disable-line no-unused-vars

You can also change to `this.requireRawId = function(id, require) { ... }` to avoid this linting note, if you like.  Either way is fine.
Attachment #8804560 - Flags: review?(jryans) → review+
Comment on attachment 8804561 [details]
Bug 1312041 - remove requireRawId rewriting in favor of Loader paths;

https://reviewboard.mozilla.org/r/88480/#review87786

Looks great, thanks for doing this!  I hope others find things more readable in this form as well.

Maybe good to note in whatever bug(s) are tweaking a webpack config that they will need to update after this lands.

::: devtools/shared/Loader.jsm:56
(Diff revision 1)
>        "source-map": "resource://devtools/shared/sourcemap/source-map.js",
>        // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠
>        // Allow access to xpcshell test items from the loader.
>        "xpcshell-test": "resource://test",
> +
> +      // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠

Ironically we've never actually discussed any change to this set on the mailing list since I added these warning comments...  and maybe they even scared people away from adding these l10n entries earlier...  Oh well. :)

In any case, these changes look reasonable to me.
Attachment #8804561 - Flags: review?(jryans) → review+
Now passing eslint.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bfbc8480d43
make loader-plugin-raw.jsm eslint-clean; r=jryans
https://hg.mozilla.org/integration/autoland/rev/c4f42d7d5453
remove requireRawId rewriting in favor of Loader paths; r=jryans
Flags: needinfo?(ttromey)
Working on it.  This patch is difficult to land because if a new use of the "wrong"
path is added, it will still apply correctly, but cause errors at runtime.
Flags: needinfo?(ttromey)
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6b8b95e97b1
make loader-plugin-raw.jsm eslint-clean; r=jryans
https://hg.mozilla.org/integration/autoland/rev/ea43bfeb850e
remove requireRawId rewriting in favor of Loader paths; r=jryans
https://hg.mozilla.org/mozilla-central/rev/d6b8b95e97b1
https://hg.mozilla.org/mozilla-central/rev/ea43bfeb850e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/954b0de8c064
Backed out changeset ea43bfeb850e 
https://hg.mozilla.org/mozilla-central/rev/a0419e0d7e28
Backed out changeset d6b8b95e97b1 for suspicion this cause bc test bustages on integration/m-c tree on a CLOSED TREE
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/18331b6089e5
make loader-plugin-raw.jsm eslint-clean because innocent of bustage on a CLOSED TREE ; r=jryans
https://hg.mozilla.org/mozilla-central/rev/e83e4fddceb4
remove requireRawId rewriting in favor of Loader paths because innocent of bustage on a CLOSED TREE; r=jryans
relanded, sorry for the backout
Flags: qe-verify? → qe-verify-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.