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)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Iteration:
51.2 - Aug 29
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [devtools-html])

Attachments

(4 files, 1 obsolete file)

Bug 1298012 - move l10n.js from devtools/client/shared to devtools/shared;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1298012 - update references to devtools/client/shared/l10n -> devtools/shared/l10n;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1298012 - update libraries paths in about:license for node-properties and sprintfjs;
58 bytes, text/x-review-board-request
gerv
: review+
Details
Bug 1298012 - Use a shared locale file for ellipsis character;
58 bytes, text/x-review-board-request
bgrins
: review+
Details 
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
Depends on: 1295608, 1295609, 1295610
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage]
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.
No longer depends on: 1295608, 1295610
Blocks: 1265887
Blocks: 1295608
Blocks: 1295610
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 51.2 - Aug 29
Whiteboard: [devtools-html] [triage] → [devtools-html]
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 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+
Summary: Migrate files in devtools/shared to use l10n.js instead of Services.string → Move l10n.js from devtools/client/shared to devtools/shared
Attachment #8784860 - Attachment is obsolete: true
Attachment #8784860 - Flags: review?(ttromey)
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.
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 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.
(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.
Keywords: leave-open
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
Oops, missed the leave-open.
Status: RESOLVED → REOPENED
status-firefox51: fixed → affected
Resolution: FIXED → ---
Target Milestone: Firefox 51 → ---
Status: REOPENED → ASSIGNED
No longer blocks: 1295608
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+
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
Thanks for the review, removing leave-open.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/d7e3cce68154
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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: