The start button in the profiler needs a label or at least a tooltip

RESOLVED FIXED in Firefox 26

Status

defect
P3
normal
RESOLVED FIXED
6 years ago
11 months ago

People

(Reporter: daniel.nr01, Assigned: srinath)

Tracking

25 Branch
Firefox 26
x86_64
Windows 7

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=anton@mozilla.com][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

6 years ago
Right now it's not obvious how to start a new profile. The start button should say "New profile" or something in addition to the icon.
Whiteboard: [good first bug][mentor=anton@mozilla.com][lang=js]
Tooltip is good. I experimented with adding labels and, since, the button is not technically for starting new profiles (its for starting recording) the labels were too long. Plus, in other language they might be even longer.
Assignee

Comment 2

6 years ago
Hi Anton,

I'm interested in working on this bug. Please let me know if you're available for mentoring.
I am! If you haven't built your version of Firefox yet you should start with this page: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → irtmail-bugzilla
Assignee

Comment 4

6 years ago
Hi Anton,

Yes. I've built firefox before. I've also submitted patches for a few bugs previously. 

In the mean time, I used mxr to identify where the string comes from. I found the following entry - http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/profiler.properties#61

I'm yet to identify how the %S in the string there is replaced by the Start button. I'm guessing that once I identify it, I will be able to add a 'tooltiptext' attribute to the button or create a new 'tooltip' and reference it in the button code.

Please let me know your thoughts on this.
(In reply to Srinath N [:srinath] from comment #4)
> 
> In the mean time, I used mxr to identify where the string comes from. I
> found the following entry -
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/devtools/profiler.properties#61

This string is a left-over from our previous UI and is obsolete now (filed bug 904212)
 
> I'm yet to identify how the %S in the string there is replaced by the Start
> button. I'm guessing that once I identify it, I will be able to add a
> 'tooltiptext' attribute to the button or create a new 'tooltip' and
> reference it in the button code.
> 
> Please let me know your thoughts on this.

I think you just need to add tooltip attribute (and add all the necessary localization strings) here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/profiler.xul#l23
Priority: -- → P3
Assignee

Comment 6

6 years ago
Posted patch tooltip-profile-start.patch (obsolete) — Splinter Review
Hi Anton, I've added the tooltip and it works for me. Please have a look and let me know your comments

Thanks,
Srinath
Comment on attachment 791696 [details] [diff] [review]
tooltip-profile-start.patch

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

Thanks! But you need to have two tooltips: one when we're not recording that says "Start profiling" and one when we're recording that says "Stop profiling". You will need to toggle that tooltip attribute in JavaScript when we start/stop recording.
Assignee

Comment 8

6 years ago
Posted patch tooltip-profile-start.patch (obsolete) — Splinter Review
Hi Anton,

I've made the necessary changes. When the profiler is running "Stop profiling" is displayed on the tooltip. When the profiler is not running (either COMPLETED or profiler hasn't been started yet), the tooltip will read "Start profiling".

I have only one concern with my approach. When the profiler dev tool is launched  and you hover over the Start button, the tooltip reads "Start profiling". This  string is fetched from profiler.dtd file. Once the start button is clicked, subsequently the tooltip text is obtained from profiler.properties file. 

So the string "Start profiling" is in two places and I'm not quite happy with it. Is there a way I can avoid the duplication?
Attachment #791696 - Attachment is obsolete: true
Comment on attachment 792610 [details] [diff] [review]
tooltip-profile-start.patch

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

Looks good. Can you add a test checking that tooltips are what we expect them to be? (You can also add to one of the existing tests) As for your questions, you *could* treat the "Start profiling" string as a default one and save its value in a variable when changing it to "Stop profiling". But I'm not sure it's worth the hassle.

(By the way, if you set the review flag to 'r? anton@mozilla.com' I will notice your request earlier)
Assignee

Comment 10

6 years ago
Attachment #792610 - Attachment is obsolete: true
Attachment #801070 - Flags: review?(anton)
Comment on attachment 801070 [details] [diff] [review]
Fixed the defect + added a test

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

I have two more smaller comments. They're not critical (hence the r+) but I'd like you to address them. Since I've already r+'ed this patch you can just set a flag [land-in-fx-team] after you upload an updated patch and someone (likely me) will land it for you. Thanks again for your contribution!

::: browser/devtools/profiler/sidebar.js
@@ +97,2 @@
>      let label = item.target.querySelector(".profiler-sidebar-item > hbox > span");
> +    let profilerStartStopToggleButton = doc.getElementById("profiler-start");

Nit: I generally use shorter local variable names in this file. `profilerStartStopToggleButton` doesn't have to be that descriptive: its function is rather short so you can always see its self-descriptive definition. `toggleButton` or even `toggle` will work just fine here.

@@ +110,5 @@
>          break;
>        case PROFILE_COMPLETED:
>          item.target.setAttribute("state", "completed");
>          label.textContent = L10N.getStr("profiler.stateCompleted");
> +        let startProfilingString = L10N.getStr("profiler.startProfilerString");

Nit: you can move startProfilingString/stopProfilingString variables into the global scope. You don't need to read it on every `setProfileState` call since it won't change between calls.
Attachment #801070 - Flags: review?(anton) → review+
Assignee

Comment 12

6 years ago
Fixed the defect + added a test + addressed Anton's nits + marking "[land-in-fx-team]" in whiteboard
Attachment #801070 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Whiteboard: [good first bug][mentor=anton@mozilla.com][lang=js] → [land-in-fx-team][good first bug][mentor=anton@mozilla.com][lang=js]
https://hg.mozilla.org/integration/fx-team/rev/944426b6b680
Whiteboard: [land-in-fx-team][good first bug][mentor=anton@mozilla.com][lang=js] → [fixed-in-fx-team][good first bug][mentor=anton@mozilla.com][lang=js]
https://hg.mozilla.org/mozilla-central/rev/944426b6b680
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][good first bug][mentor=anton@mozilla.com][lang=js] → [good first bug][mentor=anton@mozilla.com][lang=js]
Target Milestone: --- → Firefox 26

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.