Closed Bug 1312041 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: General, defect, P1)

Product:
DevTools
Component:
General
Version:
49 Branch
Type:
defect

Tracking

(firefox52 fixed)

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

People

(Reporter: jryans, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files)

Bug 1312041 - make loader-plugin-raw.jsm eslint-clean;
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1312041 - remove requireRawId rewriting in favor of Loader paths;
58 bytes, text/x-review-board-request
jryans
: review+
Details
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: 8 years ago
status-firefox52: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: