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

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

We'll have a different meta bug for code cleanup after finishing 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

Strings for l10n.

Note this fixes the issue in comment #5.

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 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".

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 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

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()

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

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

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!