1294186 - localization: migrate inspector.xul to use properties file instead of dtd

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Julian Descottes [:jdescottes]]

For devtools.html, XHTML files will no longer be able to load entities from external DTD files. All XHTML/XUL files relying on DTDs for localization should move all the DTD entities to properties files.

This bug is about migrating all localization strings defined in inspector.xul.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294186 - Add memoized getter for properties files in l10n.js;

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

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294186 - Create localization helper for markup using data-localization attributes;

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

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

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

Some of our XUL/XHTML files have a lot of localized strings coming from DTDs. I feel a better approach for migrating those is to use something similar to what l20n.js is doing: add specific attributes in the markup and use those to localize in JS.

I added a simple helper in l10n.js which does just that. It's not intended to be a long term / bulletproof solution, but I think it can bridge the gap until we wait for l20n.js, without having to add too much code in all panels. 

Let me know what you think (didn't add unit tests for this yet because I'd prefer to get your feedback first). 

FWIW, performance-wise localizing the inspector takes ~7ms on my machine if the properties files are not loaded yet. If they are this goes down to < 3 ms.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

https://reviewboard.mozilla.org/r/74104/#review72062

General thoughts:
* I'm OK with building a solution like this that lets us keep things defined in markup
* Maybe use '=' instead of ':' to delimit attribute names / values
* I think I'd rather have content be less special cased, and look something like: `<foo data-localization="content:showAllFontsUsed;title:showAllFonts"></foo>` instead of `<foo data-localization="content;title:showAllFonts">{showAllFontsUsed}</foo>`.  I realize this will require some rearranging in the case of `<html:p class="font-css">&usedAs; "<html:span class="font-css-name"></html:span>"</html:p>`, but we'll need to do the same ultimately for l20n: https://github.com/l20n/l20n.js/blob/master/docs/html.md
* Apparently annotating attributes in l20n will be quite different and require the attributes to be stored by the strings: http://l20n.org/learn/html-attributes.  Don't think we should try to duplicate that logic though

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

See Comment 5

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8784776 [details]
Bug 1294186 - Add memoized getter for properties files in l10n.js;

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

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

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8784778 [details]
Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

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

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8784778 [details]
Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

https://reviewboard.mozilla.org/r/74106/#review73150

::: devtools/client/locales/en-US/layoutview.properties:6
(Diff revision 2)
> +# The Layout View is the panel accessible at the bottom of the Inspector
> +# sidebar.

You might want to change this comment. First of all, why "at the bottom", and secondly, now that the layout-view is inside the computed-view, this comment should reflect this.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8784776 [details]
Bug 1294186 - Add memoized getter for properties files in l10n.js;

https://reviewboard.mozilla.org/r/74102/#review73182

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

https://reviewboard.mozilla.org/r/74104/#review73184

::: devtools/shared/l10n.js:164
(Diff revision 2)
> +
> +    let attributes = element.getAttribute("data-localization").split(";");
> +    for (let attribute of attributes) {
> +      let [name, value] = attribute.trim().split("=");
> +      if (name === "content") {
> +        element.innerHTML = properties[value];

As discussed, let's use textContent instead

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:10
(Diff revision 2)
> +
> +// Tests that the markup localization works properly.
> +
> +const { localizeMarkup, LocalizationHelper } = require("devtools/shared/l10n");
> +
> +function test() {

Use add_task instead

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:55
(Diff revision 2)
> +  is(div3.getAttribute("title"), str2, "The title of #d3 is localized");
> +  is(div4.innerHTML, "Some content", "The content of #d4 is not replaced");
> +  is(div4.getAttribute("aria-label"), str1, "The aria-label of #d4 is localized");
> +  is(div5.innerHTML, str3, "The content of #d5 is localized with another bundle");
> +
> +  finish();

No need for finish() with add_task

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8784776 [details]
Bug 1294186 - Add memoized getter for properties files in l10n.js;

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

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

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

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8784778 [details]
Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

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

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

https://reviewboard.mozilla.org/r/74104/#review73394

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8784776 [details]
Bug 1294186 - Add memoized getter for properties files in l10n.js;

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

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

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

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8784778 [details]
Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

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

Comment 20

[Julian Descottes [:jdescottes]]

Rebased and pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0974881a027c

Will probably land parts 1 & 2 if try is green and leave-open. Would like to have some progress on the dtd->properties migration script before landing part 3 (Bug 1295157).

Comment 21

[Julian Descottes [:jdescottes]]

Try is green landing part 1 & 2.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/8afcd614f50e
Add memoized getter for properties files in l10n.js;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/cba87d584348
Create localization helper for markup using data-localization attributes;r=bgrins

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8afcd614f50e
https://hg.mozilla.org/mozilla-central/rev/cba87d584348

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8784778 [details]
Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

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

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1294186.patch

I rebased the patch on top of changes introduced in bug 1260552
(I needed it for bug 1262443)

Please verify before landing.

Honza

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8784778 [details]
Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

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

Comment 27

[Julian Descottes [:jdescottes]]

Rebased and updated after 1302496 landed.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a90c8a62db

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8784778 [details]
Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

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

Comment 29

[Julian Descottes [:jdescottes]]

Got a permafail on linux32debug dt6, but I have the same on fx-team [1], so not related to this bug. Landing.

[1] https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=675331e308773c07e5b39cafd92ec1f4f591d152&selectedJob=11677272

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/41f5b98f7b55
Migrate inspector.xul from DTDs to .properties files;r=pbro

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41f5b98f7b55

Comment 32

[Gerry Chang [:gchang]]

Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.