Closed Bug 1082695 Opened 5 years ago Closed 5 years ago

Audit localization strings for new performance tool

Categories

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

36 Branch
x86
macOS
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(3 files, 2 obsolete files)

No description provided.
Rename profiler.dtd/properties to performance.dtd/properties
No longer blocks: perf-tool-v2
This (and cleanup of old code in general) is going to be done after shipping. No reason to block now.
Blocks: perf-tool-v2
No longer blocks: enable-perf-tool
We'll have a different meta bug for code cleanup after finishing v2.
No longer blocks: perf-tool-v2
Check out escaping from bug 1077464 comment#35
(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
Priority: -- → P1
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attached patch 1082695-l10n.patch (obsolete) — Splinter Review
Strings for l10n.
Attachment #8603678 - Flags: review?(vporof)
Note this fixes the issue in comment #5.
Attached patch 1082695-simplifybuttons.patch (obsolete) — Splinter Review
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
Attachment #8603683 - Flags: review?(vporof)
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.
I originally had an ending just for a ".". I'll re add that. Thanks!
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".
Attachment #8603678 - Flags: review?(vporof) → review+
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?
Attachment #8603683 - Flags: review?(vporof) → review+
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 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.
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
Attachment #8603678 - Attachment is obsolete: true
Attachment #8603761 - Flags: review+
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()
Attachment #8603683 - Attachment is obsolete: true
Attachment #8603762 - Flags: review+
Duplicate of this bug: 1121086
Depends on: 1163384
https://hg.mozilla.org/mozilla-central/rev/ccc8f9f3aae1
https://hg.mozilla.org/mozilla-central/rev/6c0788228680
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
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?
(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).
(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
(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 ;-)
Great, added to the other comment bug for l10n as well which we'll get in for Fx40. Thanks!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.