1295608 - localization: migrate inspector to use l10n.js instead of Services.string

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Julian Descottes [:jdescottes]]

In the scope of devtools html, the LocalizationHelper in l10n.js will be migrated to no longer depend on Services.string. 

Migrate all existing inspector classes to use this helper instead of directly relying on Services.string.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of Services.string;

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

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1295608 - Migrate devtools/client/shared to use l10n.js instead of Services.string;

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

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1295608 - Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;

I explicitly did not migrate the localizations of the external properties file brand.properties.
This should be handled as part of Bug 1297733.

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

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

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

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

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

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8782947 [details]
Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of Services.string;

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

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8784438 [details]
Bug 1295608 - Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;

https://reviewboard.mozilla.org/r/73838/#review71746

Thank you, looks good.

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8784440 [details]
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

https://reviewboard.mozilla.org/r/73876/#review71750

Thanks for adding this one on.

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8784439 [details]
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

https://reviewboard.mozilla.org/r/73874/#review71748

Thank you.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8782947 [details]
Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of Services.string;

https://reviewboard.mozilla.org/r/72952/#review71934

::: devtools/client/inspector/computed/computed.js:219
(Diff revision 2)
>    try {
> -    return CssComputedView._strings.GetStringFromName(name);
> +    return styleInspectorL10N.getStr(name);
>    } catch (ex) {
>      console.log("Error reading '" + name + "'");
>      throw new Error("l10n error with " + name);
>    }

Does the new LocalizationHelper also throw for inexistant strings? I'm guessing yes because hunder the hood it still uses Services.strings.

I wonder why we have this small helper/wrapper for l10n in this file. There doesn't seem to be cases where the string doesn't exist, or at least I think there shouldn't. Maybe we can get rid of this whole thing?
I'll R+ anyway, so I'll let you choose.

::: devtools/client/inspector/inspector-panel.js:35
(Diff revision 2)
> -  return Services.strings.createBundle("chrome://devtools/locale/inspector.properties");
> -});
> +const inspectorL10N = new LocalizationHelper("devtools/locale/inspector.properties");
> +const toolboxL10N = new LocalizationHelper("devtools/locale/toolbox.properties");

In the other files you've created const for the properties file URIs, maybe do the same here for consistency.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8784436 [details]
Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;

https://reviewboard.mozilla.org/r/73114/#review71936

::: devtools/client/animationinspector/test/browser_animation_empty_on_invalid_nodes.js:10
(Diff revision 1)
>  const STRINGS_URI = "devtools/locale/animationinspector.properties";
>  const L10N = new LocalizationHelper(STRINGS_URI);

I see you've created const in head.js files for other test dirs like:
STYLEINSPECTOR_L10N = ...
so that tests don't even have to instantiate a new LocalizationHelper.
Maybe we should do this consistently and therefore also have it in the head.js of the animationinspector tests rather than here.

::: devtools/client/inspector/markup/test/browser_markup_links_04.js:12
(Diff revision 1)
> -const STRINGS = Services.strings
> -  .createBundle("chrome://devtools/locale/inspector.properties");
> -const TOOLBOX_STRINGS = Services.strings
> -  .createBundle("chrome://devtools/locale/toolbox.properties");
>  
> +const inspectorL10N = new LocalizationHelper("devtools/locale/inspector.properties");

markup's tests already inherit from the inspector's head.js right? At least I think that's what happens. If yes, then you should already have access to INSPECTOR_L10N here.

::: devtools/client/inspector/test/head.js:37
(Diff revision 1)
>  // Import helpers for the inspector that are also shared with others
>  Services.scriptloader.loadSubScript(
>    "chrome://mochitests/content/browser/devtools/client/inspector/test/shared-head.js",
>    this);
>  
> +const {LocalizationHelper} = require("devtools/client/shared/l10n");

Might even make sense to have this in our global shared-head.js

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8784437 [details]
Bug 1295608 - Migrate devtools/client/shared to use l10n.js instead of Services.string;

https://reviewboard.mozilla.org/r/73116/#review71940

Comment 14

[Julian Descottes [:jdescottes]]

Thanks for the reviews! As a general disclaimer, I tried to keep the changes minimal for the migration here, which explains some of the issues you've raised.

(In reply to Patrick Brosset <:pbro> from comment #11)
> Comment on attachment 8782947 [details]
> Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of
> Services.string;
> 
> https://reviewboard.mozilla.org/r/72952/#review71934
> 
> ::: devtools/client/inspector/computed/computed.js:219
> (Diff revision 2)
> >    try {
> > -    return CssComputedView._strings.GetStringFromName(name);
> > +    return styleInspectorL10N.getStr(name);
> >    } catch (ex) {
> >      console.log("Error reading '" + name + "'");
> >      throw new Error("l10n error with " + name);
> >    }
> 
> Does the new LocalizationHelper also throw for inexistant strings? I'm
> guessing yes because hunder the hood it still uses Services.strings.
> 

The new Localization does not rely on Services.strings (it's on mc but probably not on fx-team :( ), but it explicitly throws when a string is unavailable for obvious backward compatibility reasons.

> I wonder why we have this small helper/wrapper for l10n in this file. There
> doesn't seem to be cases where the string doesn't exist, or at least I think
> there shouldn't. Maybe we can get rid of this whole thing?
> I'll R+ anyway, so I'll let you choose.
> 

We have so many of those helpers in the whole codebase. I decided for now to keep them if they add anything on top of the basic LocalizationHelper. In this case, it's the console.log. I would prefer to deal with this in a follow up.

> ::: devtools/client/inspector/inspector-panel.js:35
> (Diff revision 2)
> > -  return Services.strings.createBundle("chrome://devtools/locale/inspector.properties");
> > -});
> > +const inspectorL10N = new LocalizationHelper("devtools/locale/inspector.properties");
> > +const toolboxL10N = new LocalizationHelper("devtools/locale/toolbox.properties");
> 
> In the other files you've created const for the properties file URIs, maybe
> do the same here for consistency.

Good point, I think I only did it when I had to otherwise split the line to remain < 90chars, but I guess for consistency I'd be best to always do the same. I also think I haven't been consistent with the naming of the const. I'll clean that up

Comment 15

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset <:pbro> from comment #12)
> Comment on attachment 8784436 [details]
> Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;
> 
> https://reviewboard.mozilla.org/r/73114/#review71936
> 
> :::
> devtools/client/animationinspector/test/
> browser_animation_empty_on_invalid_nodes.js:10
> (Diff revision 1)
> >  const STRINGS_URI = "devtools/locale/animationinspector.properties";
> >  const L10N = new LocalizationHelper(STRINGS_URI);
> 
> I see you've created const in head.js files for other test dirs like:
> STYLEINSPECTOR_L10N = ...
> so that tests don't even have to instantiate a new LocalizationHelper.
> Maybe we should do this consistently and therefore also have it in the
> head.js of the animationinspector tests rather than here.
> 

Thanks, will do.

> ::: devtools/client/inspector/markup/test/browser_markup_links_04.js:12
> (Diff revision 1)
> > -const STRINGS = Services.strings
> > -  .createBundle("chrome://devtools/locale/inspector.properties");
> > -const TOOLBOX_STRINGS = Services.strings
> > -  .createBundle("chrome://devtools/locale/toolbox.properties");
> >  
> > +const inspectorL10N = new LocalizationHelper("devtools/locale/inspector.properties");
> 
> markup's tests already inherit from the inspector's head.js right? At least
> I think that's what happens. If yes, then you should already have access to
> INSPECTOR_L10N here.
> 

Good catch, will change it.

> ::: devtools/client/inspector/test/head.js:37
> (Diff revision 1)
> >  // Import helpers for the inspector that are also shared with others
> >  Services.scriptloader.loadSubScript(
> >    "chrome://mochitests/content/browser/devtools/client/inspector/test/shared-head.js",
> >    this);
> >  
> > +const {LocalizationHelper} = require("devtools/client/shared/l10n");
> 
> Might even make sense to have this in our global shared-head.js

Sounds good.

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8782947 [details]
Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of Services.string;

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

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8784436 [details]
Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;

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

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8784437 [details]
Bug 1295608 - Migrate devtools/client/shared to use l10n.js instead of Services.string;

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

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8784438 [details]
Bug 1295608 - Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;

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

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8784439 [details]
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

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

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8784440 [details]
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

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

Comment 22

[Julian Descottes [:jdescottes]]

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64c5a8454f15

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8784436 [details]
Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;

https://reviewboard.mozilla.org/r/73114/#review71936

> Might even make sense to have this in our global shared-head.js

I decided against this in the end, in order to avoid having to deal with the "redeclaration of const LocalizationHelper" errors.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ad552de5fa6
Migrate devtools/client/inspector to use l10n.js instead of Services.string;r=pbro
https://hg.mozilla.org/integration/autoland/rev/3c716a5a7906
Migrate devtools/client/inspector/test to use l10n.js;r=pbro
https://hg.mozilla.org/integration/autoland/rev/51bd96b9f93d
Migrate devtools/client/shared to use l10n.js instead of Services.string;r=pbro
https://hg.mozilla.org/integration/autoland/rev/85fabbd4395a
Migrate devtools/shared to use l10n.js instead of Services.string;r=tromey
https://hg.mozilla.org/integration/autoland/rev/69d2ac816444
Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;r=tromey
https://hg.mozilla.org/integration/autoland/rev/20f2ba4d1247
Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;r=tromey

Comment 25

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6269947a09a
Backed out changeset 20f2ba4d1247 
https://hg.mozilla.org/integration/autoland/rev/6978c896432c
Backed out changeset 69d2ac816444 
https://hg.mozilla.org/integration/autoland/rev/a88191007b5e
Backed out changeset 85fabbd4395a 
https://hg.mozilla.org/integration/autoland/rev/72490e03bad9
Backed out changeset 51bd96b9f93d 
https://hg.mozilla.org/integration/autoland/rev/a3eb6c47f988
Backed out changeset 3c716a5a7906 
https://hg.mozilla.org/integration/autoland/rev/3f11b63f72a9
Backed out changeset 8ad552de5fa6 for not finding devtools/client/shared/l10n in mochitest-chrome. r=backout

Comment 26

[Julian Descottes [:jdescottes]]

I explained the issue in the summary of Bug 1298012. I think only "Migrate devtools/shared to use l10n.js instead of Services.string" was at fault here. 

Here is a try push without this commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20560d2b3a5c (hopefully it will run the same test suites as the ones that failed for https://treeherder.mozilla.org/logviewer.html#?job_id=2530886&repo=autoland#L3742)

Comment 27

[Julian Descottes [:jdescottes]]

Comment on attachment 8782947 [details]
Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of Services.string;

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

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8784436 [details]
Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;

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

Comment 29

[Julian Descottes [:jdescottes]]

Comment on attachment 8784437 [details]
Bug 1295608 - Migrate devtools/client/shared to use l10n.js instead of Services.string;

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

Comment 30

[Julian Descottes [:jdescottes]]

Comment on attachment 8784438 [details]
Bug 1295608 - Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;

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

Comment 31

[Julian Descottes [:jdescottes]]

Comment on attachment 8784439 [details]
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

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

Comment 32

[Julian Descottes [:jdescottes]]

Attachment: Bug 1295608 - Migrate devtools/shared to use l10n.js instead of Services.string;

I explicitly did not migrate the localizations of the external properties file brand.properties.
This should be handled as part of Bug 1297733.

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

Comment 33

[Julian Descottes [:jdescottes]]

Comment on attachment 8782947 [details]
Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of Services.string;

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

Comment 34

[Julian Descottes [:jdescottes]]

Comment on attachment 8784436 [details]
Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;

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

Comment 35

[Julian Descottes [:jdescottes]]

Comment on attachment 8784437 [details]
Bug 1295608 - Migrate devtools/client/shared to use l10n.js instead of Services.string;

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

Comment 36

[Julian Descottes [:jdescottes]]

Comment on attachment 8784438 [details]
Bug 1295608 - Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;

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

Comment 37

[Julian Descottes [:jdescottes]]

Comment on attachment 8784439 [details]
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

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

Comment 38

[Julian Descottes [:jdescottes]]

Comment on attachment 8785199 [details]
Bug 1295608 - Migrate devtools/shared to use l10n.js instead of Services.string;

This was already reviewed by Tom. I then moved the changeset to 1298012, and moved it back here. 

Carrying over the r+ here.

Comment 39

[Julian Descottes [:jdescottes]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b916c3277c0c

Comment 40

[Julian Descottes [:jdescottes]]

Comment on attachment 8782947 [details]
Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of Services.string;

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

Comment 41

[Julian Descottes [:jdescottes]]

Comment on attachment 8784436 [details]
Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;

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

Comment 42

[Julian Descottes [:jdescottes]]

Comment on attachment 8784437 [details]
Bug 1295608 - Migrate devtools/client/shared to use l10n.js instead of Services.string;

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

Comment 43

[Julian Descottes [:jdescottes]]

Comment on attachment 8785199 [details]
Bug 1295608 - Migrate devtools/shared to use l10n.js instead of Services.string;

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

Comment 44

[Julian Descottes [:jdescottes]]

Comment on attachment 8784438 [details]
Bug 1295608 - Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;

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

Comment 45

[Julian Descottes [:jdescottes]]

Comment on attachment 8784439 [details]
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

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

Comment 46

[Julian Descottes [:jdescottes]]

Comment on attachment 8784439 [details]
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;

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

Comment 47

[Julian Descottes [:jdescottes]]

Comment on attachment 8785199 [details]
Bug 1295608 - Migrate devtools/shared to use l10n.js instead of Services.string;

Carrying over r+

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/d8068a6e48b2
Migrate devtools/client/inspector to use l10n.js instead of Services.string;r=pbro
https://hg.mozilla.org/integration/fx-team/rev/fd75ac7a5800
Migrate devtools/client/inspector/test to use l10n.js;r=pbro
https://hg.mozilla.org/integration/fx-team/rev/8df49208d18a
Migrate devtools/client/shared to use l10n.js instead of Services.string;r=pbro
https://hg.mozilla.org/integration/fx-team/rev/90688aa9ab4d
Migrate devtools/shared to use l10n.js instead of Services.string;r=tromey
https://hg.mozilla.org/integration/fx-team/rev/cc58035f8302
Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;r=tromey
https://hg.mozilla.org/integration/fx-team/rev/294c959e2368
Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;r=tromey

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8068a6e48b2
https://hg.mozilla.org/mozilla-central/rev/fd75ac7a5800
https://hg.mozilla.org/mozilla-central/rev/8df49208d18a
https://hg.mozilla.org/mozilla-central/rev/90688aa9ab4d
https://hg.mozilla.org/mozilla-central/rev/cc58035f8302
https://hg.mozilla.org/mozilla-central/rev/294c959e2368