1294220 - devtools.html localization: remove dependencies on Services.strings in l10n.js

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Julian Descottes [:jdescottes]]

For devtools.html localization, we need to be able to synchronously load properties files.

Comment 1

[Julian Descottes [:jdescottes]]

Probably needs to wait for Bug 1248830, to be discussed in triage.

Comment 2

[Julian Descottes [:jdescottes]]

Repurposing this bug. It seems that purely loading raw files is already taken care of by the loader. See Bug 1290947 comment 8 

(In reply to Tom Tromey :tromey from comment #8)
> > 2.1: create a loader to read raw/properties files
> 
> You can use require("raw!devtools/path/to/whatever") to get a file's
> contents.
> E.g.,
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/theme.
> js#15

Consequently, we do not need to do anything specific for properties files here.
The next step is to come up with a new version of shared/l10n.js that does not rely on Services.string.

Comment 3

[Julian Descottes [:jdescottes]]

My initial approach here is to use libraries in order to:
- parse the properties files (using a modified version of https://github.com/gagle/node-properties)
- format the strings relying on %S and similar (using a modified version of https://github.com/alexei/sprintf.js)

This seems to work pretty well on the few modules I tested so far. 

I tried to look for similar helpers in our current codebase but could not find any. 
ni? to Tom and Brian in case I missed an existing helper I should use instead?

Another question: should such external libraries go in devtools/client/shared/vendor (knowing they both need to be modified before using them)?

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294220 - part1: Move ellipsis character to a localized string in properties file;

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

Comment 5

[Tom Tromey :tromey]

(In reply to Julian Descottes [:jdescottes] from comment #3)
> My initial approach here is to use libraries in order to:
> - parse the properties files (using a modified version of
> https://github.com/gagle/node-properties)
> - format the strings relying on %S and similar (using a modified version of
> https://github.com/alexei/sprintf.js)

I think importing external code needs some kind of license review.
I don't know how to do this, but I think :jryans does.
Don't let this dissuade you from using external modules, supposedly the
process isn't difficult.

> I tried to look for similar helpers in our current codebase but could not
> find any. 
> ni? to Tom and Brian in case I missed an existing helper I should use
> instead?

I'm not aware of any (but I'm not sure I would be).

> Another question: should such external libraries go in
> devtools/client/shared/vendor (knowing they both need to be modified before
> using them)?

I think that's quite reasonable.  It would be good to have a large comment or
README explaining where the files came from and how they were modified (perhaps
with an upstream commit hash so we could reliably diff against the baseline revision).

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8782397 [details]
Bug 1294220 - part1: Move ellipsis character to a localized string in properties file;

https://reviewboard.mozilla.org/r/72582/#review70278

I made a few notes.

::: devtools/client/shared/l10n.js:20
(Diff revision 1)
>   *        The desired string bundle's name.
>   */
>  function LocalizationHelper(stringBundleName) {
> -  loader.lazyGetter(this, "stringBundle", () =>
> -    Services.strings.createBundle(stringBundleName));
> +  this.stringBundleName = stringBundleName;
> +  loader.lazyGetter(this, "properties", () => {
> +    let file = require("raw!" + this.stringBundleName);

This seems reasonable.  The dynamic lookup here means we'll have to be a bit more careful in our webpack config.  Also I wonder what URLs are being passed in here -- if chrome: URLs then those won't work in content...

::: devtools/client/shared/l10n.js:25
(Diff revision 1)
> +    let file = require("raw!" + this.stringBundleName);
> +    return parsePropertiesFile(file);
> +  });
> +
>    loader.lazyGetter(this, "ellipsis", () =>
> -    Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString)
> +    Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString).data);

We want to avoid Ci and require("chrome") in content.  It's fine I think to defer that to another patch but it's worth considering now what that will look like.

Also, we already have a content-only Services shim, so if it's convenient we could move some of the logic there.

::: devtools/client/shared/vendor/moz.build:10
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  modules = []
>  modules += [
>      'immutable.js',
> -    'jsol.js'
> +    'jsol.js',
> +    'parse.js'

I try to always use a trailing "," in spots like this so that a patch doesn't have to also touch the previous line.  This makes history navigation a bit easier sometimes.

Comment 7

[Julian Descottes [:jdescottes]]

Thanks for having a look !

(In reply to Tom Tromey :tromey from comment #6)
> Comment on attachment 8782397 [details]
> Bug 1294220 - devtools shared l10n helper: remove dependency on
> Services.string
> 
> https://reviewboard.mozilla.org/r/72582/#review70278
> 
> I made a few notes.
> 
> ::: devtools/client/shared/l10n.js:20
> (Diff revision 1)
> >   *        The desired string bundle's name.
> >   */
> >  function LocalizationHelper(stringBundleName) {
> > -  loader.lazyGetter(this, "stringBundle", () =>
> > -    Services.strings.createBundle(stringBundleName));
> > +  this.stringBundleName = stringBundleName;
> > +  loader.lazyGetter(this, "properties", () => {
> > +    let file = require("raw!" + this.stringBundleName);
> 
> This seems reasonable.  The dynamic lookup here means we'll have to be a bit
> more careful in our webpack config.  Also I wonder what URLs are being
> passed in here -- if chrome: URLs then those won't work in content...
> 

I plan to make another patch to make sure we don't pass chrome:// urls here.
I started thinking about the impact that localization can have on our webpack strategy, I need to spend more time on this.
(I don't think there's a way to avoid the dynamic lookup here?) 

> ::: devtools/client/shared/l10n.js:25
> (Diff revision 1)
> > +    let file = require("raw!" + this.stringBundleName);
> > +    return parsePropertiesFile(file);
> > +  });
> > +
> >    loader.lazyGetter(this, "ellipsis", () =>
> > -    Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString)
> > +    Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString).data);
> 
> We want to avoid Ci and require("chrome") in content.  It's fine I think to
> defer that to another patch but it's worth considering now what that will
> look like.
> 
> Also, we already have a content-only Services shim, so if it's convenient we
> could move some of the logic there.
> 

I would like to also fix this here. I think I can modify the preferences actor to return complex values and use this here in order to get the ellipsis. This way l10n.js will be free of any chrome privileged API.

> ::: devtools/client/shared/vendor/moz.build:10
> (Diff revision 1)
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  modules = []
> >  modules += [
> >      'immutable.js',
> > -    'jsol.js'
> > +    'jsol.js',
> > +    'parse.js'
> 
> I try to always use a trailing "," in spots like this so that a patch
> doesn't have to also touch the previous line.  This makes history navigation
> a bit easier sometimes.

Good point!

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Tom Tromey :tromey from comment #5)
> (In reply to Julian Descottes [:jdescottes] from comment #3)
> > My initial approach here is to use libraries in order to:
> > - parse the properties files (using a modified version of
> > https://github.com/gagle/node-properties)
> > - format the strings relying on %S and similar (using a modified version of
> > https://github.com/alexei/sprintf.js)
> 
> I think importing external code needs some kind of license review.
> I don't know how to do this, but I think :jryans does.
> Don't let this dissuade you from using external modules, supposedly the
> process isn't difficult.
> 

jryans: do you know the process to include external libraries such as the ones mentioned above?

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #7)
> > ::: devtools/client/shared/l10n.js:25
> > (Diff revision 1)
> > > +    let file = require("raw!" + this.stringBundleName);
> > > +    return parsePropertiesFile(file);
> > > +  });
> > > +
> > >    loader.lazyGetter(this, "ellipsis", () =>
> > > -    Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString)
> > > +    Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString).data);
> > 
> > We want to avoid Ci and require("chrome") in content.  It's fine I think to
> > defer that to another patch but it's worth considering now what that will
> > look like.
> > 
> > Also, we already have a content-only Services shim, so if it's convenient we
> > could move some of the logic there.
> > 
> 
> I would like to also fix this here. I think I can modify the preferences
> actor to return complex values and use this here in order to get the
> ellipsis. This way l10n.js will be free of any chrome privileged API.
> 
I don't think we want the preferences actor to be involved here - that would be only if we wanted to fetch prefs on the target being debugged.

This also reminds me of an issue here which is we aren't directly setting intl.ellipsis value in devtools.js so when we load the file in content the pref probably won't exist.  We could move that out into another bug, though.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #8)
> (In reply to Tom Tromey :tromey from comment #5)
> > (In reply to Julian Descottes [:jdescottes] from comment #3)
> > > My initial approach here is to use libraries in order to:
> > > - parse the properties files (using a modified version of
> > > https://github.com/gagle/node-properties)
> > > - format the strings relying on %S and similar (using a modified version of
> > > https://github.com/alexei/sprintf.js)
> > 
> > I think importing external code needs some kind of license review.
> > I don't know how to do this, but I think :jryans does.
> > Don't let this dissuade you from using external modules, supposedly the
> > process isn't difficult.
> > 
> 
> jryans: do you know the process to include external libraries such as the
> ones mentioned above?

Tangential to the license review: for things like codemirror, there's an update process that includes applying a patch to produce a modified version of the library: https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/codemirror/README#47.

I'm not sure what modifications were needed for these libs, but we should document the update process - including the current version number, link to the lib, license, and any patches needed for updates

Comment 11

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

(In reply to Julian Descottes [:jdescottes] from comment #8)
> jryans: do you know the process to include external libraries such as the
> ones mentioned above?

For license review, look over the license policy page[1].  Basically, file a licensing team bug[2] to get a clear answer from them on how to proceed.

Depending on the license, you may need to add the license to the about:license page.

[1]: https://www.mozilla.org/en-US/MPL/license-policy/
[2]: https://bugzilla.mozilla.org/enter_bug.cgi?bug_severity=normal&bug_status=NEW&component=Licensing&op_sys=All&product=mozilla.org&rep_platform=All

Comment 12

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #9)

> I don't think we want the preferences actor to be involved here - that would
> be only if we wanted to fetch prefs on the target being debugged.

We definitely don't want the prefs actor.
We have a prefs implementation in our content Services shim.

> This also reminds me of an issue here which is we aren't directly setting
> intl.ellipsis value in devtools.js so when we load the file in content the
> pref probably won't exist.  We could move that out into another bug, though.

:bgrins noted in the other bug that we could put the intl.ellipsis value into
a properties file instead and just treat it like other localized text.

Comment 13

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #9)

> This also reminds me of an issue here which is we aren't directly setting
> intl.ellipsis value in devtools.js so when we load the file in content the
> pref probably won't exist.  We could move that out into another bug, though.

We have to read at least part of all.js anyway, because not all devtools
prefs are in devtools.js.  I meant to mention this in bug 1272098 but I see
I forgot to do so.

Comment 14

[Tom Tromey :tromey]

Oops, sorry, comment #13 got delayed by a mid-air collision and so might
only make sense a few comments earlier.

Comment 15

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #7)
> 
> (In reply to Tom Tromey :tromey from comment #6)
> > 
> > This seems reasonable.  The dynamic lookup here means we'll have to be a bit
> > more careful in our webpack config.  Also I wonder what URLs are being
> > passed in here -- if chrome: URLs then those won't work in content...
> > 
> 
> I plan to make another patch to make sure we don't pass chrome:// urls here.

As discussed with Tom on IRC yesterday, the properties files are currently only accessible via chrome:// URLs. We quickly discussed two options here:

1- Use require("raw!devtools/locale/inspector.properties") and add some logic in the raw loader that will prefix *.properties URLs with chrome://. This means we effectively keep using chrome URLs when loaded in chrome and we will need to figure out a way to make webpack find the properties files.

2- Change the way we install the *.properties so that we can access them from a resource:// URL. If we do so using the DevtoolsModules() helper in moz.build, this means we will end up having to require("raw!devtools/client/locales/en-US/inspector.properties"). We can use navigator.language to get the locale of the browser so we should be able to build the path here. However this means adding a moz.build file in the devtools/client/locales/en-US folder, which means the same would need to be done on all l10n repos, if that's even possible.

Maybe I'm missing something, but solution 2 does not seem like the way to go. Unless someone has another option, can I go ahead with the first solution for now?

Regarding the issue with webpack, maybe the LocalizationHelper could check if it's running in chrome. If yes it would prefix the paths with "chrome://", if not it would rewrite the paths to target devtools/client/locales/en-US ?

(Not sure what our plans are for localizations of the webpack version of devtools, but I guess in the beginning it will be en-US only?)

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294220 - Escape % characters in localized strings

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

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294220 - fix tests relying on L10N.stringBundle

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

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8782397 [details]
Bug 1294220 - part1: Move ellipsis character to a localized string in properties file;

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

Comment 19

[Julian Descottes [:jdescottes]]

Cleaned up the current implementation, and pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=a129cd6b0aa5de0c0553c8e2c1f4fbcead169136, which is green after fixing a few tests depending directly on the internals of the LocalizationHelper.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> (In reply to Julian Descottes [:jdescottes] from comment #8)
> > jryans: do you know the process to include external libraries such as the
> > ones mentioned above?
> 
> For license review, look over the license policy page[1].  Basically, file a
> licensing team bug[2] to get a clear answer from them on how to proceed.
> 
> Depending on the license, you may need to add the license to the
> about:license page.
>

Thanks for the info! Opened Bug 1296600.

Comment 20

[Tom Tromey :tromey]

(In reply to Julian Descottes [:jdescottes] from comment #15)

> Regarding the issue with webpack, maybe the LocalizationHelper could check
> if it's running in chrome.

I think it would be better to try pretty hard to isolate any chrome-vs-content
dependencies elsewhere.  It can be done in the loader or in more extreme situations
by writing two modules, one for chrome and one for content, in devtools/shared/platform/.

In this particular case the change can be made in loader-plugin-raw.jsm, I think.
This isn't used in content; the solution for the content case is to document the
new requirement(s) in the webpack bug (bug 1291049) so that the webpack config
can be updated.

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8782397 [details]
Bug 1294220 - part1: Move ellipsis character to a localized string in properties file;

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

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8782397 [details]
Bug 1294220 - part1: Move ellipsis character to a localized string in properties file;

https://reviewboard.mozilla.org/r/72582/#review70796

::: devtools/client/shared/l10n.js:7
(Diff revision 3)
>  
> -const { Ci } = require("chrome");
> -const Services = require("Services");
> -
>  const { parsePropertiesFile } = require("devtools/client/shared/vendor/node-properties");
>  const { vsprintf } = require("devtools/client/shared/vendor/sprintf");

Weird that these vendor libs are showing up in mozreview, but I see it's not part of the patch so no problem

Comment 23

[Brian Grinstead [:bgrins]]

I think the ni? has been answered - let me know if not

Comment 24

[Julian Descottes [:jdescottes]]

Thanks for the review! 

I cleaned up the current patches and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55188394d0a1
Will push them all for review if try is green.

(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Julian Descottes [:jdescottes] from comment #8)
> 
> I'm not sure what modifications were needed for these libs, but we should
> document the update process - including the current version number, link to
> the lib, license, and any patches needed for updates

About this, I added UPGRADE text instructions. To be honest, these are small libraries, I don't really expect we will need to synchronize with the upstream versions. So I didn't take the time to create actual scripts to reapply automatically the changes I did here. If one of them becomes a pain to maintain, I guess we should take the time to rewrite it ourselves.

Comment 25

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294220 - part2: Devtools shared l10n helper: remove dependency on Services.string;

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

Comment 26

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294220 - part3: Escape % characters in localized strings;

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

Comment 27

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294220 - part6: use standard getter instead of loader.lazyGetter in l10n.js;

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

Comment 28

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294220 - part4: Add chrome:// protocol when loading raw properties files;

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

Comment 29

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294220 - part5: Remove the "chrome://" protocol when using LocalizationHelper;

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

Comment 30

[Julian Descottes [:jdescottes]]

Comment on attachment 8782397 [details]
Bug 1294220 - part1: Move ellipsis character to a localized string in properties file;

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

Comment 31

[Tom Tromey :tromey]

Comment on attachment 8783434 [details]
Bug 1294220 - part4: Add chrome:// protocol when loading raw properties files;

https://reviewboard.mozilla.org/r/73248/#review71032

Thank you.  This looks good.

Comment 32

[Tom Tromey :tromey]

Comment on attachment 8783431 [details]
Bug 1294220 - part2: Devtools shared l10n helper: remove dependency on Services.string;

https://reviewboard.mozilla.org/r/73242/#review71036

::: devtools/client/shared/l10n.js:16
(Diff revision 1)
>   *
>   * @param string stringBundleName
>   *        The desired string bundle's name.
>   */
>  function LocalizationHelper(stringBundleName) {
> -  loader.lazyGetter(this, "stringBundle", () =>
> +  loader.lazyGetter(this, "properties", () => {

We also need to get rid of loader.lazyGetter uses.  It seems like this one could be easily done as a getter on LocalizationHelper itself.

Comment 33

[Tom Tromey :tromey]

Comment on attachment 8783435 [details]
Bug 1294220 - part5: Remove the "chrome://" protocol when using LocalizationHelper;

https://reviewboard.mozilla.org/r/73250/#review71038

Thank you.  This patch is fine.

I'm curious, though, how the locale-finding will actually work.  LocalizationHelper doesn't currently seem to do this.  What I mean is, the source refers to a file by one name, like "devtools/clientinspector.properties", but the file is actually "devtools/client/locales/en-US/inspector.properties".  For the webpack case we can handle this by rewriting the paths, assuming perhaps that we only plan to support the one locale.  But I don't understand how it works for the chrome case.

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8783433 [details]
Bug 1294220 - part6: use standard getter instead of loader.lazyGetter in l10n.js;

https://reviewboard.mozilla.org/r/73246/#review71170

r=me but this should be pulled into part 2 so that change is more self-contained

Comment 35

[Brian Grinstead [:bgrins]]

Comment on attachment 8783432 [details]
Bug 1294220 - part3: Escape % characters in localized strings;

https://reviewboard.mozilla.org/r/73244/#review71172

Interesting, I guess the `%%` syntax is valid, but not required, for producing a single `%` with Services.strings.  r=me once you bump the key names to it will get re-localized

::: devtools/client/locales/en-US/memory.properties:215
(Diff revision 1)
>  # that represents the root of the tree when inverted.
>  tree-item.root=(root)
>  
>  # LOCALIZATION NOTE (tree-item.percent): A percent of bytes or count displayed in the tree view.
> -tree-item.percent=%S%
> +# there are two "%" after %S to escape and display "%"
> +tree-item.percent=%S%%

These key names need to be bumped (tree-item.percent2 and table.percentage3)

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8783431 [details]
Bug 1294220 - part2: Devtools shared l10n helper: remove dependency on Services.string;

https://reviewboard.mozilla.org/r/73242/#review71178

Comment 37

[Brian Grinstead [:bgrins]]

Comment on attachment 8783431 [details]
Bug 1294220 - part2: Devtools shared l10n helper: remove dependency on Services.string;

https://reviewboard.mozilla.org/r/73242/#review71180

Comment 38

[Julian Descottes [:jdescottes]]

Comment on attachment 8782397 [details]
Bug 1294220 - part1: Move ellipsis character to a localized string in properties file;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72582/diff/4-5/

Comment 39

[Julian Descottes [:jdescottes]]

Comment on attachment 8783431 [details]
Bug 1294220 - part2: Devtools shared l10n helper: remove dependency on Services.string;

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

Comment 40

[Julian Descottes [:jdescottes]]

Comment on attachment 8783432 [details]
Bug 1294220 - part3: Escape % characters in localized strings;

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

Comment 41

[Julian Descottes [:jdescottes]]

Comment on attachment 8783434 [details]
Bug 1294220 - part4: Add chrome:// protocol when loading raw properties files;

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

Comment 42

[Julian Descottes [:jdescottes]]

Comment on attachment 8783435 [details]
Bug 1294220 - part5: Remove the "chrome://" protocol when using LocalizationHelper;

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

Comment 43

[Julian Descottes [:jdescottes]]

Comment on attachment 8783433 [details]
Bug 1294220 - part6: use standard getter instead of loader.lazyGetter in l10n.js;

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

Comment 44

[Julian Descottes [:jdescottes]]

Comment on attachment 8783432 [details]
Bug 1294220 - part3: Escape % characters in localized strings;

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

Comment 45

[Julian Descottes [:jdescottes]]

Comment on attachment 8783434 [details]
Bug 1294220 - part4: Add chrome:// protocol when loading raw properties files;

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

Comment 46

[Julian Descottes [:jdescottes]]

Comment on attachment 8783435 [details]
Bug 1294220 - part5: Remove the "chrome://" protocol when using LocalizationHelper;

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

Comment 47

[Julian Descottes [:jdescottes]]

Comment on attachment 8783433 [details]
Bug 1294220 - part6: use standard getter instead of loader.lazyGetter in l10n.js;

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

Comment 48

[Julian Descottes [:jdescottes]]

Thanks for the reviews Tom & Brian!

(In reply to Brian Grinstead [:bgrins] from comment #35)
> Comment on attachment 8783432 [details]
> Bug 1294220 - part3: Escape % characters in localized strings;
> 
> https://reviewboard.mozilla.org/r/73244/#review71172
> 
> Interesting, I guess the `%%` syntax is valid, but not required, for
> producing a single `%` with Services.strings.  r=me once you bump the key
> names to it will get re-localized
> 
> ::: devtools/client/locales/en-US/memory.properties:215
> (Diff revision 1)
> >  # that represents the root of the tree when inverted.
> >  tree-item.root=(root)
> >  
> >  # LOCALIZATION NOTE (tree-item.percent): A percent of bytes or count displayed in the tree view.
> > -tree-item.percent=%S%
> > +# there are two "%" after %S to escape and display "%"
> > +tree-item.percent=%S%%
> 
> These key names need to be bumped (tree-item.percent2 and table.percentage3)

Good catch, thanks ! And yes, with Services.string, the % doesn't have to be escaped if it's the last character of the string. When in the middle of the string, we already had to escape it (for instance "profiler.bufferFull" in performance.properties).

(In reply to Tom Tromey :tromey from comment #32)
> Comment on attachment 8783431 [details]
> 
> We also need to get rid of loader.lazyGetter uses.  It seems like this one
> could be easily done as a getter on LocalizationHelper itself.

Sure, pushed a new patch for this :)


(In reply to Tom Tromey :tromey from comment #33)
> 
> I'm curious, though, how the locale-finding will actually work. 
> LocalizationHelper doesn't currently seem to do this.  What I mean is, the
> source refers to a file by one name, like
> "devtools/clientinspector.properties", but the file is actually
> "devtools/client/locales/en-US/inspector.properties".  For the webpack case
> we can handle this by rewriting the paths, assuming perhaps that we only
> plan to support the one locale.  But I don't understand how it works for the
> chrome case.

As discussed on IRC, using a chrome url should guarantee that the correct locale package is targeted, and this should be transparent for the "chrome" use case. For the "content" use case, we are fine with only having the en-US localization for now so we just have to configure webpack to map "devtools/locale/*.properties" to "devtools/client/locales/en-US/*.properties" (and "devtools-shared/locale/*.properties" to "devtools/shared/locales/en-US/*.properties")

Comment 49

[Brian Grinstead [:bgrins]]

Comment on attachment 8783432 [details]
Bug 1294220 - part3: Escape % characters in localized strings;

https://reviewboard.mozilla.org/r/73244/#review71236

Comment 50

[Julian Descottes [:jdescottes]]

Pushed the last version to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4bdff5407a1, all green so far.

Comment 51

[Tom Tromey :tromey]

Comment on attachment 8783433 [details]
Bug 1294220 - part6: use standard getter instead of loader.lazyGetter in l10n.js;

https://reviewboard.mozilla.org/r/73246/#review71402

Thank you very much!

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b79bba6f2091
part1: Move ellipsis character to a localized string in properties file;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/fc125c233fe0
part2: Devtools shared l10n helper: remove dependency on Services.string;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/f92cdcfe5d46
part3: Escape % characters in localized strings;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/fca9b2cc3f8f
part4: Add chrome:// protocol when loading raw properties files;r=tromey
https://hg.mozilla.org/integration/autoland/rev/1cccb1ca9c92
part5: Remove the "chrome://" protocol when using LocalizationHelper;r=tromey
https://hg.mozilla.org/integration/autoland/rev/4d0ea69c57f6
part6: use standard getter instead of loader.lazyGetter in l10n.js;r=bgrins,tromey

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b79bba6f2091
https://hg.mozilla.org/mozilla-central/rev/fc125c233fe0
https://hg.mozilla.org/mozilla-central/rev/f92cdcfe5d46
https://hg.mozilla.org/mozilla-central/rev/fca9b2cc3f8f
https://hg.mozilla.org/mozilla-central/rev/1cccb1ca9c92
https://hg.mozilla.org/mozilla-central/rev/4d0ea69c57f6