Closed Bug 1308500 Opened 8 years ago Closed 8 years ago

Migrate localization strings away from netmonitor.dtd

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: rickychien, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

Bug 1308500 - Migrate localization strings away from netmonitor.dtd;
58 bytes, text/x-review-board-request
jsnajdr
: review+
Details 
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
Whiteboard: [devtools-html]
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Blocks: 1309191
Work on it first since it block provide strings in react component
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
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)
Thanks! I'll try that later.
Flags: needinfo?(rchien)
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.
Priority: -- → P1
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)
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.
Blocks: 1308440
Blocks: 1268444
Flags: needinfo?(gasolin)
> 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.
> 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.
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 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+
treeherder green, thanks!
Keywords: checkin-needed
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
Iteration: --- → 52.3 - Nov 7
https://hg.mozilla.org/mozilla-central/rev/6fc7fc30b5c5
Status: ASSIGNED → RESOLVED
Closed: 8 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+
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: