Migrate localization strings away from netmonitor.dtd

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: rickychien, Assigned: gasolin@mozilla.com)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [netmonitor])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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
Depends on: 1295157

Updated

2 years ago
Whiteboard: [devtools-html]

Updated

2 years ago
Whiteboard: [netmonitor]

Updated

2 years ago
Flags: qe-verify+
QA Contact: ciprian.georgiu
(Reporter)

Updated

2 years ago
Blocks: 1309191
(Assignee)

Comment 1

2 years ago
Work on it first since it block provide strings in react component
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
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
Flags: needinfo?(rchien)
(Reporter)

Comment 4

2 years ago
Thanks! I'll try that later.
Flags: needinfo?(rchien)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
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.

Updated

2 years ago
Priority: -- → P1

Comment 7

2 years ago
mozreview-review
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=...".
@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
Flags: needinfo?(gasolin)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
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.
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1308440
(Assignee)

Updated

2 years ago
Blocks: 1268444
Flags: needinfo?(gasolin)
(Assignee)

Comment 13

2 years ago
> 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.
(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 hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
> 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 hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
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

2 years ago
mozreview-review
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!
Attachment #8800979 - Flags: review?(jsnajdr) → review+
(Assignee)

Comment 20

2 years ago
treeherder green, thanks!
Keywords: checkin-needed

Comment 21

2 years ago
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6fc7fc30b5c5
Migrate localization strings away from netmonitor.dtd;r=jsnajdr
Keywords: checkin-needed

Updated

2 years ago
Iteration: --- → 52.3 - Nov 7

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6fc7fc30b5c5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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
Depends on: 1312352
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.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.