1163763 - Performance tools L10N cleanup

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Patrick Brosset <:pbro>]

In order to ship a part of bug 1146239 in time for dev-edition 40, we decided to use an existing l10n string from another tool in /browser/devtools/performance/views/recordings.js

This is bad because we shouldn't reuse strings from other contexts:
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Don%27t_reuse_strings_in_different_contexts

So in this bug, we should add a new dedicated string for when a record has finished and is being loaded, in performance.properties:

# LOCALIZATION NOTE (recordingsList.loadingRecordLabel):
# This string is displayed in the recordings list of the Profiler,
# for an item that has finished recording and that is being loaded in the tool
# (i.e. has not yet appeared).
recordingsList.loadingRecordLabel=Loading record…

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

We'll have some unlocalized strings for 40.1 -- this is a bug to keep track of unlocalized changes landed to readd and revisit for 41.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Revisit: recordingsList.saveDialogTitle=Save profile…
Changed in bug 1164593, both the save and import dialogs have been updated without l10n.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

profiler.bufferStatus no longer used via bug 1166124 due to string change

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

We've needed to add or change a handful of strings for the 40.1 release on June 2nd -- right now they're hardcoded and uplifting next week from nightly to aurora 40.1. Is there a better way to handle these string change uplifts? Its my understanding we cannot uplift any l10n changes and did what we could to preempt what strings we'd need, but still some changes are necessary. Luckily, I believe a good amount of them are unlocaluzable strings (using technical descriptions that reference known names and intrinsic JS things)

Any thoughts, recommendations, advice for this?

Comment 5

[Francesco Lodolo [:flod]]

Mozilla-aurora is string frozen in order to give localizers 6 weeks to work on localization. If necessary it's possible to land new strings in mozilla-aurora, approval is on release-drivers, but in general we try to avoid that as much as possible.

Also, devtools are actually localized only in a subset of locales, but landing strings make noise for all of them.

It would be a lot easier to give a suggestion with a clear indication of the amount of string changes requested (to see if we can work around them for fx40), and when these changes would eventually be ready for landing.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Still not sure how many changes are needed; should know by end of week. With bug 1167317 looming, I wonder if this would be duplicated effort

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #5)
> It would be a lot easier to give a suggestion with a clear indication of the
> amount of string changes requested (to see if we can work around them for
> fx40), and when these changes would eventually be ready for landing.

I have a very preliminary try push here: https://hg.mozilla.org/try/pushloghtml?changeset=ed5fcd147ed5 (from Bug 1167252).  Out of these, the only one that I see that currently needs string changes is Bug 1143004, which is coincidentally the only one I've requested uplift for.

Jordan, am I missing anything?  I know there was discussion about changing a couple of things soon (like Bug 1150761) but it seems like maybe there are just going to be a few strings that we are dealing with

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: commit-queue-outgoing.patch

And for reference, here is output of hg out -p with the current perf patches applied

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

To clarify, any strings after May 12th which should've been localized were just hardcoded strings (other than bug 1143004) -- those are listed in the comments at the beginning of this bug

Comment 10

[Francesco Lodolo [:flod]]

So, unless I'm missing some strings.

For profiler.properties:
Loading record…
Import recording…
Save recording…
Buffer ${percent}% full

For timeline.properties
Buffer ${percent}% full

@Pike
Any strong opinion about this? Even if not ideal, I think that we can live with some hard-coded strings in Firefox 40 for devtools, and let the proper fix ride the trains.

Side note: if we decide to go ahead and land these on mozilla-aurora, it would help to land them in one step. That's why I think it makes more sense to think about Fx40+devtools+strings globally.

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Details View Names in bug 1150761, hardcoded

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

I'd prefer to keep them hardcoded for the remaining 3 weeks in this cycle rather than more uplifting work (it's already a nightmare) and everything else that needs to get done is way more complicated with all the release changes than I'd like, unless there's strong feelings otherwise.

If that's the case, we should hardcode the timestamp text in bug 1143004 that bgrins mentioned for now

Comment 13

[Axel Hecht [:Pike]]

Fine with me.

Comment 14

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #13)
> Fine with me.

OK, let's go with hard-coded strings for Aurora and make sure that they're probably localizable on central for fx41.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #12)
> I'd prefer to keep them hardcoded for the remaining 3 weeks in this cycle
> rather than more uplifting work (it's already a nightmare) and everything
> else that needs to get done is way more complicated with all the release
> changes than I'd like, unless there's strong feelings otherwise.
> 
> If that's the case, we should hardcode the timestamp text in bug 1143004
> that bgrins mentioned for now

Sounds good Jordan, can you attach a patch with the hardcoded timestamp text here so I can track it for the uplift?

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #12)
> > I'd prefer to keep them hardcoded for the remaining 3 weeks in this cycle
> > rather than more uplifting work (it's already a nightmare) and everything
> > else that needs to get done is way more complicated with all the release
> > changes than I'd like, unless there's strong feelings otherwise.
> > 
> > If that's the case, we should hardcode the timestamp text in bug 1143004
> > that bgrins mentioned for now
> 
> Sounds good Jordan, can you attach a patch with the hardcoded timestamp text
> here so I can track it for the uplift?

Actually, I guess what we really need is a copy of the patch already in Bug 1143004 attached over there, but with the hardcoded string instead of the localized string.  We can then request uplift on that one separately since we're going to need to land a different version on Aurora than what landed on m-c.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The texts are still hardcoded (on central, aurora and beta). What's the plan to make them localizeable or use the translations?

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

We just need to update the localization for the hardcoded strings -- I can't imagine we'll want to uplift, due to low amount of l10n for devtools, as well as the internal-specific/technical text used frequently.

Comment 20

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1163763-l10n.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4884db2aee59

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

Outstanding ni? on GC/CC marker naming, emailed Nick but here's prob better:

Going through the L10N stuff that we didnt do for these markers (or anything in the last month or so), and I know we had a few discussions somewhere about these, but wanted to run them by you, as I don't really remember.

Currently:
"GC Event" and "GC Event (Non-incremental)" are the labels used for gc events, with "Cause" and "Non-incremental Cause" in non-inc cases for the properties.

"Cycle Collection" for CC event labels, with only a field for "Type" displaying Collect or ForgetSkippable.

--

What we have now, I'll probably add a "Type" field to GC's (displaying incremental or non-incremental, making it obvious), and wonder if we can have both of the gc/cc markers have similar labels -- maybe "CC Event" and "GC Event"? Chrome has "Minor GC", maybe we can just have "GC (major/minor)" and "CC (major/minor)", but I believe you said the major/minor explanation wasn't the best way to describe it -- although it might be a good-enough description that we can follow up with more information (like the "Type" field will explain Collect/ForgetSkippable, and could link to MDN for more info)

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Minor/major is whether it is a nursery collection or a full tenured heap collection. We shouldn't conflate existing definitions. Cycle collections don't have major/minor; we shouldn't try and force it on them and again conflate that with major/minor GC. Additionally, we only trace major gcs right now. Every major gc slice starts with a minor collection of the nursery before proceeding.

We also shouldn't conflate "GC Events" with actual DOM events, because they aren't DOM events. It's just confusing.

--------------8<----------------8<---------------8<-------------

Here are the marker names and properties I'd go with:

* "Minor GC" marker name for minor (aka nursery) collections (nice to have but not essential feature, not yet implemented)

* "Incremental GC" marker name for incremental major gc slices
  - reason: gc reason

* "Nonincremental GC" marker name for when we are forced into a nonincremental major gc (note: this is often very suboptimal)
  - reason "<the gc reason>"
  - nonincremental reason: "<the nonincremental reason>"

* "Cycle Collection" marker name for when we are actually doing cycle collection (ie not ForgetSkippable)

* "CC Graph Reduction" marker for ForgetSkippable. This is the best name I can think of, as the point of ForgetSkippable is to reduce the size of the CC graph considered so that it is faster to collect.

--------------8<----------------8<---------------8<-------------

"Cycle Collection" and "CC Graph Reduction" are two separate things often running at separate times and should really be treated as such.

Yes, we should have docs for these on MDN, as we should have for all the markers.

Comment 23

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1163763-l10n.patch

Added the label names for GC/CC markers as per nick's comment

Comment 24

[Francesco Lodolo [:flod]]

Comment on attachment 8634956 [details] [diff] [review]
1163763-l10n.patch

Review of attachment 8634956 [details] [diff] [review]:
-----------------------------------------------------------------

I stopped after markers.properties because I don't understand this patch at all.

You're adding strings to profiler.dtd that are already in the file. Is this patch against mozilla-aurora (Firefox 41)? 

You *can not* land a patch with strings in Firefox 41. We just need to fix the missing strings on trunk, and wait for the fix to ride the trains.

::: browser/locales/en-US/chrome/browser/devtools/markers.properties
@@ +41,5 @@
> +# The following labels should most likely not be translated, as they correspond
> +# to JS functions.
> +marker.label.javascript.setInterval=setInterval
> +marker.label.javascript.setTimeout=setTimeout
> +marker.label.javascript.requestAnimationFrame=requestAnimationFrame

I think you should completely remove these 3 strings. As you say they're function names, no need to make them localizable.

@@ +61,5 @@
> +# Field names for stack values
> +marker.field.stack=Stack:
> +marker.field.startStack=Stack at start:
> +marker.field.endStack=Stack at end:
> +marker.field.asyncStack=(Async: %S)

What is %S?

Comment 25

[Victor Porof [:vporof][:vp]]

Waiting on updated patch addressing flod's comments.

Comment 26

[Victor Porof [:vporof][:vp]]

Comment on attachment 8634956 [details] [diff] [review]
1163763-l10n.patch

See previous comment.

Comment 27

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8634956 [details] [diff] [review]
1163763-l10n.patch

Review of attachment 8634956 [details] [diff] [review]:
-----------------------------------------------------------------

This is for Fx42 -- this cleans up several localizations that were in several different files previously for the two separate tools (profiler, timeline) and organizes them logically according to the UI (performance) as well as the subcomponents (l10n file just for markers themselves, which could be used elsewhere in the future). I'm not sure what you mean by adding strings to profiler.dtd that are already in the file (as everything was moved to performance.dtd)

Comment 28

[Francesco Lodolo [:flod]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #27)
> I'm not sure what you mean by adding strings to profiler.dtd that are 
> already in the file (as everything was moved to performance.dtd)

It's been a while, and the only explanation is that I completely misread the patch (not sure how) :-\

Setting NI to check again with more attention. This is going to be pretty painful for localizers, with so many strings moving at the same time (tools should be able to cope with translation memory, not so much if you localize with text editors).

Comment 29

[Francesco Lodolo [:flod]]

Comment on attachment 8634956 [details] [diff] [review]
1163763-l10n.patch

Review of attachment 8634956 [details] [diff] [review]:
-----------------------------------------------------------------

A few nits together with the ones about markers.properties, but besides that I think strings should be OK. Sorry again for the confusion about file renaming.

::: browser/locales/en-US/chrome/browser/devtools/markers.properties
@@ +51,5 @@
> +# with the details. For examples: Paint (200x100), or console.time (FOO)
> +marker.fieldFormat=%1$S (%2$S)
> +
> +# LOCALIZATION NOTE (time.field.*):
> +# Strings used in the waterfall sidebar as property names.

There's a localization note but no time.field.* strings
Did you mean marker.field.*?

::: browser/locales/en-US/chrome/browser/devtools/performance.dtd
@@ +159,5 @@
> +
> +<!-- LOCALIZATION NOTE (performanceUI.console.stopCommandStart/stopCommandEnd):
> +  -  This string is displayed when a recording is selected that started via console.profile.
> +  -  Indicates how to stop the recording, wrapping the command, like
> +  -  "Stop recording by entering console.profilEnd("label") into the console." -->

Extra copy and paste?

::: browser/locales/en-US/chrome/browser/devtools/performance.properties
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# LOCALIZATION NOTE These strings are used inside the Performance Tools
> +

Nit: remove empty line

@@ +24,5 @@
> +# Used for the menuitem in the tool menu
> +performance.commandkey=VK_F5
> +performance.accesskey=P
> +
> +# LOCALIZATION NOTE (performance.tooltip3):

performance.tooltip

@@ +47,5 @@
> +
> +# LOCALIZATION NOTE (recordingsList.loadingLabel):
> +# This string is displayed in the recordings list of the Performance Tools,
> +# for an item that is finished and is loading.
> +recordingsList.loadingLabel=Loading\u2026

Any specific reason to use \u2026 instead of …?

@@ +91,5 @@
> +category.storage=Storage
> +category.events=Input & Events
> +category.tools=Tools
> +
> +# LOCALIZATION NOTE (graphs.ms):

table.ms

@@ +95,5 @@
> +# LOCALIZATION NOTE (graphs.ms):
> +# This string is displayed in the call tree after units of time in milliseconds.
> +table.ms=ms
> +
> +# LOCALIZATION NOTE (graphs.ms):

table.percentage

@@ +167,5 @@
> +# LOCALIZATION NOTE (profiler.bufferFull):
> +# This string is displayed when recording, indicating how much of the
> +# buffer is currently be used.
> +# %S is the percentage of the buffer used,
> +profiler.bufferFull=Buffer %S%% full

Please add in the note why there are two %.

Comment 30

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1163763-l10n.patch

Addressed all comments!

Comment 31

[Francesco Lodolo [:flod]]

Comment on attachment 8642201 [details] [diff] [review]
1163763-l10n.patch

Review of attachment 8642201 [details] [diff] [review]:
-----------------------------------------------------------------

Strings look good, thanks.

Comment 32

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8642201 [details] [diff] [review]
1163763-l10n.patch

Review of attachment 8642201 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks flod!

Comment 33

[Victor Porof [:vporof][:vp]]

Comment on attachment 8642201 [details] [diff] [review]
1163763-l10n.patch

Review of attachment 8642201 [details] [diff] [review]:
-----------------------------------------------------------------

This is so nice and clean now!

::: browser/devtools/performance/modules/logic/marker-utils.js
@@ +301,3 @@
>    "setInterval handler":       "setInterval",
>    "setTimeout handler":        "setTimeout",
>    "FrameRequestCallback":      "requestAnimationFrame",

Add some comments here about how these labels shouldn't be localized because they represent actual js API.

@@ +352,5 @@
>     */
>    JSFields: function (marker) {
>      if ("causeName" in marker && !JS_MARKER_MAP[marker.causeName]) {
> +      let cause = PREFS["show-platform-data"] ? marker.causeName : GECKO_SYMBOL;
> +      return { [L10N.getStr("marker.field.causeName")]: cause };

Nit: keep consistent formatting. Newline after { and before }

Are we sure it's safe to localize object properties? Are we relying on English property names anywhere? It's probably safe from an utf pov, but I'm worrying about assuming this property is always called "Reason" somewhere in the codebase.

@@ +384,5 @@
>      }
>    },
>  
>    CycleCollectionFields: function (marker) {
> +    return { [L10N.getStr("marker.field.type")]: marker.name.replace(/nsCycleCollector::/g, "") };

Nit: keep consistent formatting. Newline after { and before }

Comment 34

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8642201 [details] [diff] [review]
1163763-l10n.patch

Review of attachment 8642201 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/logic/marker-utils.js
@@ +352,5 @@
>     */
>    JSFields: function (marker) {
>      if ("causeName" in marker && !JS_MARKER_MAP[marker.causeName]) {
> +      let cause = PREFS["show-platform-data"] ? marker.causeName : GECKO_SYMBOL;
> +      return { [L10N.getStr("marker.field.causeName")]: cause };

I'm more worried about utf-8'ing property names, but if they're a valid JS string, it should be fine. There isn't any place in the code base that relies on this field being "Reason", as this shouldn't be any different than any other localized field, which should be handled just fine

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3301c85aa671

Comment 36

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1163763-2.patch

test fix

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/20ced115bd64

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3301c85aa671
https://hg.mozilla.org/mozilla-central/rev/20ced115bd64

Comment 39

[Ton [:Tonnes]]

>> <!ENTITY performanceUI.table.selfDuration.tooltip     "The amount of time spent only within this function.">
>> <!ENTITY performanceUI.recordButton.tooltip "Toggle the recording state of a performance recording.">
>> <!ENTITY performanceUI.options.gear.tooltiptext "Configure performance preferences.">
>> etc.

Once again, please make it a habit *not* to use trailing periods in tooltips, unless when instructing a user or using full sentences, which is very, very rare (such as performanceUI.bufferStatusTooltip.) Descriptions or actions should NOT use them, as they generally use infinitive mood, while adding them results in imperative mood, making users think they are ordered to do something, both in English and for any other locale where different verbs may be needed. As imperative and infinitive mood look similar in English, erroneously placed periods make the problem hard to recognize for non-English, but even more vital for English users for the reason above.

As a localizer I keep running into improper (and inconsistent) use of end stops in tooltips on en-US devtools files on almost any FF version. I’ve commented on this issue 3 times before and even had to file a bug once to have some corrected, while people keep making this mistake. Now I might be wrong, but think localizers shouldn’t need to worry about, nor spend time on finding out whether or not infinitive or imperative mood is meant in strings, nor remind developers about proper use more than once. Anyone working on devtools files should seriously take this into account just like any other source file where people do so. Tooltips just shouldn’t use periods, period.

If possible (yes, it probably is), please watch/fix this when entering/moving strings, as well as when doing QA, and always keep this in mind. Thanks.

Comment 40

[Jordan Santell [:jsantell] [@jsantell]]

Filed bug 1199515 for correcting the end stops in the performance tool.


(In reply to Ton from comment #39)
> As a localizer I keep running into improper (and inconsistent) use of end
> stops in tooltips on en-US devtools files on almost any FF version. I’ve
> commented on this issue 3 times before and even had to file a bug once to
> have some corrected, while people keep making this mistake.

We're a large team covering many tools, it's likely most (myself included) were not aware of the tooltip grammar rule.

>  Now I might be wrong, but think localizers shouldn’t need to worry about, nor spend time on
> finding out whether or not infinitive or imperative mood is meant in
> strings, nor remind developers about proper use more than once. Anyone
> working on devtools files should seriously take this into account just like
> any other source file where people do so. Tooltips just shouldn’t use
> periods, period.

We have many contributors, volunteers and those on the team, where english is not their native language, or may not know grammar rules of imperative and infinitive moods -- we try, but mistakes and lack of good grammar (speaking about myself here) will happen -- pinging someone from localization on any L10N changes/additions could be a solution for those less confident in string changes. I don't think it's unreasonable to remind an entire team more than once about this.

Comment 41

[chris hofmann]

(In reply to Ton from comment #39)
> >> <!ENTITY performanceUI.table.selfDuration.tooltip     "The amount of time spent only within this function.">
> >> <!ENTITY performanceUI.recordButton.tooltip "Toggle the recording state of a performance recording.">
> >> <!ENTITY performanceUI.options.gear.tooltiptext "Configure performance preferences.">
> >> etc.
> 
> Once again, please make it a habit *not* to use trailing periods in
> tooltips, unless when instructing a user or using full sentences, which is
> very, very rare (such as performanceUI.bufferStatusTooltip.) Descriptions or
> actions should NOT use them, as they generally use infinitive mood, while
> adding them results in imperative mood, making users think they are ordered
> to do something, both in English and for any other locale where different
> verbs may be needed. As imperative and infinitive mood look similar in
> English, erroneously placed periods make the problem hard to recognize for
> non-English, but even more vital for English users for the reason above.
> 
> As a localizer I keep running into improper (and inconsistent) use of end
> stops in tooltips on en-US devtools files on almost any FF version. I’ve
> commented on this issue 3 times before and even had to file a bug once to
> have some corrected, while people keep making this mistake. Now I might be
> wrong, but think localizers shouldn’t need to worry about, nor spend time on
> finding out whether or not infinitive or imperative mood is meant in
> strings, nor remind developers about proper use more than once. Anyone
> working on devtools files should seriously take this into account just like
> any other source file where people do so. Tooltips just shouldn’t use
> periods, period.
> 
> If possible (yes, it probably is), please watch/fix this when
> entering/moving strings, as well as when doing QA, and always keep this in
> mind. Thanks.


Ton, These guidelines you are suggesting are probably worth formalizing and adding to places like
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices
and https://wiki.mozilla.org/L10n/WorldReady

That we often point contributors at to help improve skills that will lead to better and easier localization.   Let's figure out how to get some of these items integrated there.

Maybe jbeatty can help with that.

Jordan,

Your probably right that we might always need reminders, but its also good to point contributors to good docs that train in advance of hacking on stuff.  Lets try and do more of that too.

-chofmann

Comment 42

[Jordan Santell [:jsantell] [@jsantell]]

Adding to the MDN page, with examples, would be great for both contributors and internally! Also was thinking about adding to the l10n tests checking strings named "tooltip" and the last character of the string, but sounds like its dependent on how its used, so not automatable.

Comment 43

[Francesco Lodolo [:flod]]

IMO that's not L10n, that's English copy guidelines for Firefox (each language should have its own guidelines), and they should be in a different document. And you want to involve Matej in that kind of discussion.

That page already says:

> If you have doubts about the quality of strings, ask a copywriter to do a copy 
> review of this text. Ideally, all strings landing in code should originate from 
> approved UX wireframes, and copy review should be part of the initial stage of 
> creating these wireframes.

And I still think that's the way things should go (maybe because I wrote that…). Developers are generally not a good pick when it comes to design things, or explain them in strings.