1082695 - Audit localization strings for new performance tool

Status: RESOLVED FIXED

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

Comment 1

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

Rename profiler.dtd/properties to performance.dtd/properties

Comment 2

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

This (and cleanup of old code in general) is going to be done after shipping. No reason to block now.

Comment 3

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

We'll have a different meta bug for code cleanup after finishing v2.

Comment 4

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

Check out escaping from bug 1077464 comment#35

Comment 5

[Francesco Lodolo [:flod]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> Check out escaping from bug 1077464 comment#35

Also note that this string doesn't make our tools happy
https://l10n.mozilla.org/dashboard/compare?run=488157

Comment 6

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

Attachment: 1082695-l10n.patch

Strings for l10n.

Comment 7

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

Note this fixes the issue in comment #5.

Comment 8

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

Attachment: gif of new recording start/stop message.

Comment 9

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

Attachment: 1082695-simplifybuttons.patch

The rest of the changes. Please review the L10n stuff first, that has to land soon. There are notes to remove strings once this lands in the l10n file.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bfbb471b788

Comment 10

[Francesco Lodolo [:flod]]

Comment on attachment 8603678 [details] [diff] [review]
1082695-l10n.patch

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

::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
@@ +140,5 @@
> +
> +<!-- LOCALIZATION NOTE (profilerUI.console.recordingNotice):
> +  -  This string is displayed when a recording is selected that started via console.profile.
> +  -  Wraps the command used to start, like "Currently recording via console.profile("label")" -->
> +<!ENTITY profilerUI.console.recordingNotice "Currently recording via">

Not all locales might be able to concatenate the string like this. Let's say a locale needs "Currently console.profile is recording with". 

You need a "after" string, even if it remains empty for English.

Comment 11

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

I originally had an ending just for a ".". I'll re add that. Thanks!

Comment 12

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

Comment on attachment 8603678 [details] [diff] [review]
1082695-l10n.patch

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

::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
@@ +140,5 @@
> +
> +<!-- LOCALIZATION NOTE (profilerUI.console.recordingNotice):
> +  -  This string is displayed when a recording is selected that started via console.profile.
> +  -  Wraps the command used to start, like "Currently recording via console.profile("label")" -->
> +<!ENTITY profilerUI.console.recordingNotice "Currently recording via">

Currently recording console profile with label "%S".

Comment 13

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

Comment on attachment 8603683 [details] [diff] [review]
1082695-simplifybuttons.patch

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

::: browser/devtools/performance/performance.xul
@@ -76,5 @@
>                 class="devtools-toolbar">
>          <hbox id="recordings-controls"
>                class="devtools-toolbarbutton-group">
>            <toolbarbutton id="main-record-button"
> -                         class="devtools-toolbarbutton record-button devtools-thobber"

I just noticed the "thobber" now.

@@ +141,5 @@
>                pack="center"
>                flex="1">
> +          <hbox class="devtools-toolbarbutton-group">
> +            <toolbarbutton class="record-button"
> +              label="&profilerUI.startRecording;" />

Let's respect the formatting in this file. Align attributes to the left.

::: browser/themes/shared/devtools/performance.inc.css
@@ +92,5 @@
> +}
> +
> +.theme-light #performance-view toolbarbutton.record-button,
> +.theme-light .console-profile-command {
> +  background-color: #ddd;

Nit: Use rgba instead of solid grays, here and everywhere else.
Also, why can't we use vars here?

Comment 14

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

Comment on attachment 8603683 [details] [diff] [review]
1082695-simplifybuttons.patch

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

::: browser/themes/shared/devtools/performance.inc.css
@@ +92,5 @@
> +}
> +
> +.theme-light #performance-view toolbarbutton.record-button,
> +.theme-light .console-profile-command {
> +  background-color: #ddd;

I'll see if there's a grey var, but consistently using vars that look good in both light/dark is rare (so this'll still just be for light)

Comment 15

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

Comment on attachment 8603678 [details] [diff] [review]
1082695-l10n.patch

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

::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
@@ +140,5 @@
> +
> +<!-- LOCALIZATION NOTE (profilerUI.console.recordingNotice):
> +  -  This string is displayed when a recording is selected that started via console.profile.
> +  -  Wraps the command used to start, like "Currently recording via console.profile("label")" -->
> +<!ENTITY profilerUI.console.recordingNotice "Currently recording via">

That's what we had before -- no where else in the tool do we call it "console profile", and this would need another string in the event of unlabeled console profile (because technically it shouldn't be an empty string), so this just explains what started it, which could occur on some benchmark or page that uses console.profile, and the user may not know what "console profile" means.

This just clearly explains what started it, and how to stop it.

Comment 16

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

Attachment: 1082695-l10n.patch

Added an "end" string for localization of console profile recordings.

Keeping the console profile string I had before for reasons in comment #15, and feel pretty strongly about it

Comment 17

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

Attachment: 1082695-simplifybuttons.patch

Addressed all comments -- also removed the border/background color of the monospace `console.profile()` elements, as it matched the start/stop recording buttons, and these are not buttons (although maybe in the future they can be, and open console and enter console.profileEnd()

Comment 18

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/ccc8f9f3aae1
remote:   https://hg.mozilla.org/integration/fx-team/rev/6c0788228680

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ccc8f9f3aae1
https://hg.mozilla.org/mozilla-central/rev/6c0788228680

Comment 21

[Francesco Lodolo [:flod]]

I realize that I missed a confusing string

<!-- LOCALIZATION NOTE (profilerUI.recordButton2): This string is displayed
  -  on a button that starts a new profile. -->
<!ENTITY profilerUI.recordButton2.tooltip "Toggle the recording state of a performance recording.">

What exactly does it mean?

Comment 22

[Francesco Lodolo [:flod]]

(In reply to Victor Porof [:vporof][:vp] from comment #12)
> Comment on attachment 8603678 [details] [diff] [review]
> 1082695-l10n.patch
> 
> Review of attachment 8603678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
> @@ +140,5 @@
> > +
> > +<!-- LOCALIZATION NOTE (profilerUI.console.recordingNotice):
> > +  -  This string is displayed when a recording is selected that started via console.profile.
> > +  -  Wraps the command used to start, like "Currently recording via console.profile("label")" -->
> > +<!ENTITY profilerUI.console.recordingNotice "Currently recording via">
> 
> Currently recording console profile with label "%S".

Also, not sure if this has been included (I read it as a request to change the string).

Comment 23

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

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #21)
> I realize that I missed a confusing string
> 
> <!-- LOCALIZATION NOTE (profilerUI.recordButton2): This string is displayed
>   -  on a button that starts a new profile. -->
> <!ENTITY profilerUI.recordButton2.tooltip "Toggle the recording state of a
> performance recording.">
> 
> What exactly does it mean?

This button starts a recording, and then is "activated", and then can be used to then stop that recording.



(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #22)
> (In reply to Victor Porof [:vporof][:vp] from comment #12)
> > Comment on attachment 8603678 [details] [diff] [review]
> > 1082695-l10n.patch
> > 
> > Review of attachment 8603678 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
> > @@ +140,5 @@
> > > +
> > > +<!-- LOCALIZATION NOTE (profilerUI.console.recordingNotice):
> > > +  -  This string is displayed when a recording is selected that started via console.profile.
> > > +  -  Wraps the command used to start, like "Currently recording via console.profile("label")" -->
> > > +<!ENTITY profilerUI.console.recordingNotice "Currently recording via">
> > 
> > Currently recording console profile with label "%S".
> 
> Also, not sure if this has been included (I read it as a request to change
> the string).

This was not included due to reasons in comment #15 -- this does not use any new unfamiliar terms ("console profile"), will work for both labeled and unlabeled recordings, and explicitly indicates a command that was used to start recording a profile (because it may not have been the user looking at the recording, as it could've been included in the source code). If there is strong reasoning against having the string that was landed, we can change it though

Comment 24

[Francesco Lodolo [:flod]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #23)
> This button starts a recording, and then is "activated", and then can be
> used to then stop that recording.

I find the string a bit confusing. Maybe you can add this comment to the string? The current one seems like a copy and paste from another string.

> This was not included due to reasons in comment #15 -- this does not use any
> new unfamiliar terms ("console profile"), will work for both labeled and
> unlabeled recordings, and explicitly indicates a command that was used to
> start recording a profile (because it may not have been the user looking at
> the recording, as it could've been included in the source code). If there is
> strong reasoning against having the string that was landed, we can change it
> though

Thanks, I missed that comment. No need to change for me ;-)

Comment 25

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

Great, added to the other comment bug for l10n as well which we'll get in for Fx40. Thanks!