Closed Bug 1294186 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: General, defect, P1)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox51 fix-optional, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Flags:
Tracking Status
firefox51 --- fix-optional
firefox52 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files, 2 obsolete files)

Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;
58 bytes, text/x-review-board-request
pbro
: review+
Details
bug1294186.patch
33.79 KB, patch
Details | Diff | Splinter Review 
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.
Whiteboard: [devtools-html] [triage]
Blocks: 1290947, devtools-html-phase2
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
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.
Attachment #8784777 - Flags: feedback?(bgrinstead)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Priority: P2 → P1
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 on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

See Comment 5
Attachment #8784777 - Flags: feedback?(bgrinstead)
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.
Attachment #8784778 - Flags: review?(pbrosset) → review+
Comment on attachment 8784776 [details]
Bug 1294186 - Add memoized getter for properties files in l10n.js;

https://reviewboard.mozilla.org/r/74102/#review73182
Attachment #8784776 - Flags: review?(bgrinstead) → review+
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
Attachment #8784777 - Flags: review?(bgrinstead)
Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

https://reviewboard.mozilla.org/r/74104/#review73394
Attachment #8784777 - Flags: review?(bgrinstead) → review+
Iteration: 51.2 - Aug 29 → 51.3 - Sep 12
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).
Try is green landing part 1 & 2.
Keywords: leave-open
Pushed by jdescottes@mozilla.com:
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
Depends on: 1295157
Attachment #8784776 - Attachment is obsolete: true
Attachment #8784777 - Attachment is obsolete: true
Depends on: 1302496
Attached patch bug1294186.patch — — Splinter Review 
I rebased the patch on top of changes introduced in bug 1260552
(I needed it for bug 1262443)

Please verify before landing.

Honza
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
Rebased and updated after 1302496 landed.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a90c8a62db
No longer depends on: 1295157
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
Keywords: leave-open
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/41f5b98f7b55
Migrate inspector.xul from DTDs to .properties files;r=pbro
https://hg.mozilla.org/mozilla-central/rev/41f5b98f7b55
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.
status-firefox51: affected → fix-optional
See Also: → 1308503
Product: Firefox → DevTools
See Also: → 1804425
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: