1308500 - Migrate localization strings away from netmonitor.dtd

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

There is a “localizeMarkup” utility in devtools/shared/l10n. Use it to convert the l10n for Netmonitor. The same task was done for Inspector in bug 1294186. Depends on creating a script for automated conversion of the *.dtd files in all locales. That’s bug 1295157.

Land before the next merge day. This could be one of the first steps.
Step 1: Move strings from netmonitor.dtd to netmonitor.properties.
Step 2: Use localizeMarkup to localize the static netmonitor.xul file.
Step 3: Convert netmonitor.xul step-by-step to React components. Don't worry about l10n at all, because everything is already converted to properties.
There should be netmonitor.properties

Comment 1

[[e-mail]]

Work on it first since it block provide strings in react component

Comment 2

[[e-mail]]

Attachment: Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

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

Comment 3

[[e-mail]]

The WIP patch has 
* migrated netmonitor.dtd strings to netmonitor.properties
* migrated toolbar related strings to netmonitor.properties in xul

You can base on it to handle toolbar related react component strings

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Thanks! I'll try that later.

Comment 5

[[e-mail]]

Comment on attachment 8800979 [details]
Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

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

Comment 6

[[e-mail]]

The patch convert netmonitor.dtd strings to netmonitor.properties

It use same panel.js modification in bug 1308503, will rebase when its landed.

This patch doesn't convert popupset&keyset strings so it still depends on netmonitor.dtd.
We could remove netmonitor.dtd once we converted those elements.

Comment 7

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8800979 [details]
Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

https://reviewboard.mozilla.org/r/85776/#review84422

Why don't you remove netmonitor.dtd at the same time? Am I missing something?

::: devtools/client/locales/en-US/netmonitor.properties:313
(Diff revision 2)
> +
> +# LOCALIZATION NOTE (netmonitorUI.perfNotice1/2/3): These are the labels displayed
> +# in the network table when empty to start performance analysis.
> +netmonitorUI.perfNotice1=• Click on the
> +netmonitorUI.perfNotice2=button to start performance analysis.
> +netmonitorUI.perfNotice3=Analyze

Consider replacing the "netmonitorUI" prefix with "netmonitor". This makes the naming convention in the netmonitor.properties file more consistent.

::: devtools/client/netmonitor/netmonitor.xul:106
(Diff revision 2)
>          <hbox id="requests-menu-filter-buttons">
>            <button id="requests-menu-filter-all-button"
>                    class="requests-menu-filter-button"
>                    checked="true"
>                    data-key="all"
> -                  label="&netmonitorUI.footer.filterAll;">
> +                  data-localization="content=netmonitorUI.footer.filterAll">

This looks wrong - l10n of attribute "label" is converted to "content", which means the textContent of the <button> element, doesn't it? It should be "label=...".

Comment 8

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

@Fred: just not to forget, one of the questions we had on our stand up is whether the netmonitor.dtd can be removed as part of the patch. Can we do that?

Honza

Comment 9

[[e-mail]]

Comment on attachment 8800979 [details]
Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

https://reviewboard.mozilla.org/r/85776/#review84422

As comment 6 This patch doesn't convert popupset&keyset (which are not in scope of <deck> element) strings so it still depends on netmonitor.dtd.

https://github.com/mozilla/gecko-dev/blob/master/devtools/client/netmonitor/netmonitor.xul#L24

I'll convert popupset&keyset soon then we can removed netmonitor.dtd from the codebase.

> Consider replacing the "netmonitorUI" prefix with "netmonitor". This makes the naming convention in the netmonitor.properties file more consistent.

ok I can change it

> This looks wrong - l10n of attribute "label" is converted to "content", which means the textContent of the <button> element, doesn't it? It should be "label=...".

Thanks for finding that. Inspector use data-localization="title=inspectorAddNode.label" since it's html's button but not XUL's button.

Though with `content=` I can't visually identify the diffence on UI, I'll change them to `data-localization="label=` with spirit of minimum change in refactoring.

Comment 10

[Jarda Snajdr [:jsnajdr]]

(In reply to Fred Lin [:gasolin] from comment #9)
> As comment 6 This patch doesn't convert popupset&keyset (which are not in
> scope of <deck> element) strings so it still depends on netmonitor.dtd.

The localizeMarkup(this.panelDoc) call gets the whole XUL document as an argument, so why does it matter that the popupset and keyset elements are outside of the <deck>? The call to root.querySelectorAll("[data-localization]") at [1] should return these elements, right? Or am I wrong about something? Did you try to convert the popupset&keyset elements? And it really didn't work?

> Though with `content=` I can't visually identify the diffence on UI, I'll
> change them to `data-localization="label=` with spirit of minimum change in
> refactoring.

Thanks. The <button> element label can be set by both "label" attribute and the textContent. Changing label to textContent could introduce subtle styling changes - some CSS styles can have a "[label]" selector, "crop" attribute can behave differently... No mochitest would catch that, only a careful visual inspection.

Comment 11

[[e-mail]]

Comment on attachment 8800979 [details]
Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

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

Comment 12

[[e-mail]]

Comment on attachment 8800979 [details]
Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

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

Comment 13

[[e-mail]]

> The localizeMarkup(this.panelDoc) call gets the whole XUL document as an
> argument, so why does it matter that the popupset and keyset elements are
> outside of the <deck>? The call to
> root.querySelectorAll("[data-localization]") at [1] should return these
> elements, right? Or am I wrong about something? Did you try to convert the
> popupset&keyset elements? And it really didn't work?

It's didn't work. I have not read l10n code but that might because
 
1. localizeMarkup only deal with child elements which the root element bind with `data-localization-bundle` attribute
2. L10n might not deal with menuitem element because we'll migrated to menu API


I think its fine to keep netmonitor.dtd for a while since we can convert bug 1308440 (menu API) and bug 1268444 (key shortcut) and remove netmonitor.dtd in a very short term.

Comment 14

[Jarda Snajdr [:jsnajdr]]

(In reply to Fred Lin [:gasolin] from comment #13)
> It's didn't work. I have not read l10n code but that might because
>  
> 1. localizeMarkup only deal with child elements which the root element bind
> with `data-localization-bundle` attribute

Yes, this seems to be a cause. Moving the attribute from <deck> up to <window> should make it work.

> I think its fine to keep netmonitor.dtd for a while since we can convert bug
> 1308440 (menu API) and bug 1268444 (key shortcut) and remove netmonitor.dtd
> in a very short term.

The point of this bug is to migrate everything from netmonitor.dtd to netmonitor.properties and then not have to worry about it again. All localizations from netmonitor.xul will be eventually moved away, either to menu API or to React components. I'd prefer to do the whole migration in one step.

Comment 15

[[e-mail]]

Comment on attachment 8800979 [details]
Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

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

Comment 16

[[e-mail]]

> The point of this bug is to migrate everything from netmonitor.dtd to netmonitor.properties 
> and then not have to worry about it again. All localizations from netmonitor.xul 
> will be eventually moved away, either to menu API or to React components. 
> I'd prefer to do the whole migration in one step.

That's a reasonable concern, I've update the patch to do the whole migration in one step. Please kindly review it.

Comment 17

[[e-mail]]

Comment on attachment 8800979 [details]
Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

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

Comment 18

[[e-mail]]

Note: I dup the `netmonitor.summary.editAndResend` to `netmonitor.context.editAndResend` to separate the string presention in the context menu and the summary tab.

Comment 19

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8800979 [details]
Bug 1308500 - Migrate localization strings away from netmonitor.dtd;

https://reviewboard.mozilla.org/r/85776/#review85082

Looks perfect, thanks for fixing all issues!

Comment 20

[[e-mail]]

treeherder green, thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fc7fc30b5c5
Migrate localization strings away from netmonitor.dtd;r=jsnajdr

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fc7fc30b5c5

Comment 23

[Julian Descottes [:jdescottes]]

FYI, I created a migration file for this bug in the scope of the devtools l10n migration script: Bug 1295157 and https://bugzilla.mozilla.org/attachment.cgi?id=8801671

Comment 24

[Ciprian Georgiu, Desktop QA]

I've managed to perform test around netmonitor migrate localization (l10n) strings. I can confim that nothing was broken and is working as expected.

Based on the above, this issue is verified fixed on latest Nightly 52.0a1 (2016-10-24), using the following OSes:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- macOS 10.12.1 Beta

I will mark here accordingly.