Closed Bug 1295608 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
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

(6 files, 1 obsolete file)

Bug 1295608 - Migrate devtools/client/inspector to use l10n.js instead of Services.string;
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1295608 - Migrate devtools/client/inspector/test to use l10n.js;
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1295608 - Migrate devtools/client/shared to use l10n.js instead of Services.string;
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1295608 - Migrate devtools/client/sourceeditor to use l10n.js instead of Services.string;
58 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1295608 - Migrate devtools/client/definitions.js to use l10n.js instead of Services.string;
58 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1295608 - Migrate devtools/shared to use l10n.js instead of Services.string;
58 bytes, text/x-review-board-request
jdescottes
: review+
Details 
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.
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Priority: P2 → P1
Depends on: 1294220
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.
Attachment #8784438 - Flags: review?(ttromey) → review+
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.
Attachment #8784440 - Flags: review?(ttromey) → review+
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.
Attachment #8784439 - Flags: review?(ttromey) → review+
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.
Attachment #8782947 - Flags: review?(pbrosset) → review+
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
Attachment #8784436 - Flags: review?(pbrosset) → review+
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
Attachment #8784437 - Flags: review?(pbrosset) → review+
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
(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 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.
Pushed by jdescottes@mozilla.com:
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
Blocks: 1298012
Backout by archaeopteryx@coole-files.de:
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
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)
Attachment #8784440 - Attachment is obsolete: true
No longer blocks: 1298012
Depends on: 1298012
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.
Attachment #8785199 - Flags: review?(ttromey) → review+
No longer depends on: 1298012
Comment on attachment 8785199 [details]
Bug 1295608 - Migrate devtools/shared to use l10n.js instead of Services.string;

Carrying over r+
Attachment #8785199 - Flags: review?(ttromey) → review+
Pushed by jdescottes@mozilla.com:
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
Depends on: 1299070
Depends on: 1375471
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: