Closed
Bug 1298012
Opened 8 years ago
Closed 8 years ago
Move l10n.js from devtools/client/shared to devtools/shared
Categories
(DevTools :: General, defect, P1)
DevTools
General
Tracking
(firefox51 fixed)
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
Details
(Whiteboard: [devtools-html])
Attachments
(4 files, 1 obsolete file)
Follow up to Bug 1295608. In order for classes in devtools/shared to use l10n.js, this library must be moved out side of devtools/client/shared to devtools/shared. Otherwise we get failures on Android tests [1]. I assume this occurs when client is not shipped together with the rest of devtools. Let's treat this as a separate issue, and work on it after all the current l10n.js migration bugs have landed. Will need to: - move l10n.js to devtools/shared - move the two libraries used by l10n.js to devtools/shared - update about:license (since it mentions the paths of the files) - replace all devtools/client/shared/l10n -> devtools/shared/l10n [1] https://treeherder.mozilla.org/logviewer.html#?job_id=2530886&repo=autoland#L3742
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
Blocks: devtools-html-phase2
Assignee | ||
Comment 1•8 years ago
|
||
Since Bug 1295609 is the only one on Autoland right now, I will wait until it gets merged to mc and will then attempt to land this one. After that, we will update 1295608, 1295610 & 1265887 with the new paths.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Updated•8 years ago
|
Iteration: --- → 51.2 - Aug 29
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Whiteboard: [devtools-html] [triage] → [devtools-html]
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8784858 [details] Bug 1298012 - move l10n.js from devtools/client/shared to devtools/shared; https://reviewboard.mozilla.org/r/74160/#review72040
Attachment #8784858 -
Flags: review?(bgrinstead) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8784859 [details] Bug 1298012 - update references to devtools/client/shared/l10n -> devtools/shared/l10n; https://reviewboard.mozilla.org/r/74162/#review72042
Attachment #8784859 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Updated•8 years ago
|
Summary: Migrate files in devtools/shared to use l10n.js instead of Services.string → Move l10n.js from devtools/client/shared to devtools/shared
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8784860 -
Attachment is obsolete: true
Attachment #8784860 -
Flags: review?(ttromey)
Assignee | ||
Comment 13•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11f856e66f13
Assignee | ||
Comment 14•8 years ago
|
||
As discussed in standup, I reviewed the current usage of localization in the devtools/shared folder. Rules for files in shared/: - a devtools/shared file should not depend on client files - a devtools/shared file should not depend on server files - a devtools/shared file should not use chrome specific APIs (in order to be used by client callers) I think these rules make sense, and given rule#3, it means that if shared files are using localization, they should do so using the LocalizationHelper, and given rule#1 this helper should not be in client. The other alternative is to forbid the usage of localization in this package. Shared files using localization: - css-logic, it only uses localization to localize "inline" but this is used by both client and server - security/prompt, used by both client and server and using localization for both. - fronts/ csscoverage, using it to display a notification on "state-changed". The callback could be moved to client (StyleEditor.jsm) - gcli/ all commands I honestly think it makes sense to keep the localization utility as something that can be used by both server and client. IMO this is the kind of helper that can and should be shared. However I'm curious about which localization files are actually available if we run in a context where only server & shared files are available.
Assignee | ||
Comment 15•8 years ago
|
||
Even after moving the l10n helper to devtools/shared, we still have one android test failure [1] ... > However I'm curious about which localization files are actually available if we run in a context > where only server & shared files are available. ... and it looks like it's because devtools/locale/shared.properties cannot be accessed in this context. When l10n.js is loaded, it will eagerly fetch the ellipsis character in the shared.properties file, and make it accessible at exports.ELLIPSIS. We can either : - use lazy loading for this property -> https://treeherder.mozilla.org/#/jobs?repo=try&revision=94da57fa837714e8e4c7fac58636fc2a8ae5c754 - move the ellipsis to a properties file of devtools/shared -> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a580c9968fada68ba32a27b97c82d54cbc315293 That being said, I'm not 100% sure if the solution will work out, as I don't really know what gets shipped with the server & shared files. I suppose that devtools/shared/locale should be available, but let's wait for the end of the try test here. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b916c3277c0c&selectedJob=26405121
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8785354 [details] Bug 1298012 - Use a shared locale file for ellipsis character; https://reviewboard.mozilla.org/r/74590/#review72426
Attachment #8785354 -
Flags: review?(bgrinstead) → review+
(In reply to Julian Descottes [:jdescottes] from comment #14) > However I'm curious about which localization files are actually available if > we run in a context where only server & shared files are available. IIRC, only the files in devtools/shared/locales are accessible in a server-only context.
Assignee | ||
Comment 22•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #21) > (In reply to Julian Descottes [:jdescottes] from comment #14) > > However I'm curious about which localization files are actually available if > > we run in a context where only server & shared files are available. > > IIRC, only the files in devtools/shared/locales are accessible in a > server-only context. Looks like it is, thanks for the confirmation! So, both try runs being successful, we will be moving the ellipsis character to a shared properties file here. Thanks for the review, Brian. I will proceed to land the first 3 changesets and leave-open for the license update so that I can unblock the other bugs.
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 23•8 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/bd4bde4a3649 move l10n.js from devtools/client/shared to devtools/shared;r=bgrins https://hg.mozilla.org/integration/fx-team/rev/21bfa8e5fa3b update references to devtools/client/shared/l10n -> devtools/shared/l10n;r=bgrins https://hg.mozilla.org/integration/fx-team/rev/9797d309cc74 Use a shared locale file for ellipsis character;r=bgrins
remote: https://hg.mozilla.org/mozilla-central/rev/bd4bde4a3649 remote: https://hg.mozilla.org/mozilla-central/rev/21bfa8e5fa3b remote: https://hg.mozilla.org/mozilla-central/rev/9797d309cc74
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Oops, missed the leave-open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 51 → ---
Updated•8 years ago
|
Status: REOPENED → ASSIGNED
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8784861 [details] Bug 1298012 - update libraries paths in about:license for node-properties and sprintfjs; https://reviewboard.mozilla.org/r/74166/#review72742 r=gerv. Gerv
Attachment #8784861 -
Flags: review?(gerv) → review+
Comment 27•8 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/d7e3cce68154 update libraries paths in about:license for node-properties and sprintfjs;r=gerv
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d7e3cce68154
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•