Closed Bug 1120699 Opened 7 years ago Closed 7 years ago

New Performance tool should toggle between empty/recording/content views

Categories

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

37 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(2 files, 3 obsolete files)

Like the original profiler, on recording and waiting and loaded states, we should display something other than the empty waterfall/calltree/flamegraph views.
Depends on: 1121086
Attached patch Patch v1 (obsolete) — Splinter Review
Needs rebasing on top of bug 1111004, and maybe some tests.
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8548381 - Flags: review?(jsantell)
Depends on: 1111004
Attached patch Patch v2 (obsolete) — Splinter Review
Rebased and added test.
Attachment #8548381 - Attachment is obsolete: true
Attachment #8548381 - Flags: review?(jsantell)
Attachment #8548434 - Flags: review?(jsantell)
Attached patch Patch v2.1 (obsolete) — Splinter Review
Fixed issue where the stopwatch icon was white in light theme (for the notice).

f?bgrins for the changes in toolbars.inc.css. Note that I don't use an id for the button, to avoid the duplicated id problem we had with the (old) profiler and the timeline.
Attachment #8548434 - Attachment is obsolete: true
Attachment #8548434 - Flags: review?(jsantell)
Attachment #8548447 - Flags: review?(jsantell)
Attachment #8548447 - Flags: feedback?
Attachment #8548447 - Flags: feedback? → feedback?(bgrinstead)
Comment on attachment 8548447 [details] [diff] [review]
Patch v2.1

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

::: browser/devtools/performance/performance-view.js
@@ +71,5 @@
>    _onRecordButtonClick: function (e) {
>      if (this._recordButton.hasAttribute("checked")) {
>        this._recordButton.removeAttribute("checked");
>        this._lockRecordButton();
> +      $("#profile-pane").selectedPanel = $("#details-pane");

So I don't think this'll work here. For example, if we start/stop one profile, and then start a second one, click back to the first, the "recording notice" is still displayed. We should display the details view for the currently selected profile, if it is done, or if its recording, and NOT from the console (which we don't even have built in yet, the console.profile hooks), then we should display the recording notice.

I think it might be easier/make more sense if we hooked into the EVENTS.RECORDING_SELETED event from the performance controller, which passes in a RecordingModel[0] object, which can tell you whether or not the model is currently recording. Then will probably need something when RECORDING_STOPPED is called as well to check again. Let me know if you have any questions about this!

[0] https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/performance/modules/recording-model.js
Attachment #8548447 - Flags: review?(jsantell)
Attached image padding issue in OSX
Also looks like there's some padding issue on OSX for the clock button
Great stuff so far :) hit me with another r? once those issues are resolved
Comment on attachment 8548447 [details] [diff] [review]
Patch v2.1

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

Changes to toolbars.inc.css look good

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +892,5 @@
>  .theme-light .command-button-invertable[checked=true]:not(:active) > image,
>  .theme-light .devtools-tab[icon-invertable][selected] > image,
>  .theme-light .devtools-tab[icon-invertable][highlighted] > image,
>  .theme-light #record-snapshot[checked] > image,
> +.theme-light #profiler-start[checked] > image,

I'm not sure what element this is referring to, but a quick search turns up nothing: https://dxr.mozilla.org/mozilla-central/search?q=profiler-start&redirect=true.  Maybe this selector should be removed while you are in here.
Attachment #8548447 - Flags: feedback?(bgrinstead) → feedback+
(In reply to Brian Grinstead [:bgrins] from comment #7)
> Comment on attachment 8548447 [details] [diff] [review]
> Patch v2.1
> 
> Review of attachment 8548447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes to toolbars.inc.css look good
> 
> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ +892,5 @@
> >  .theme-light .command-button-invertable[checked=true]:not(:active) > image,
> >  .theme-light .devtools-tab[icon-invertable][selected] > image,
> >  .theme-light .devtools-tab[icon-invertable][highlighted] > image,
> >  .theme-light #record-snapshot[checked] > image,
> > +.theme-light #profiler-start[checked] > image,
> 
> I'm not sure what element this is referring to, but a quick search turns up
> nothing:
> https://dxr.mozilla.org/mozilla-central/search?q=profiler-
> start&redirect=true.  Maybe this selector should be removed while you are in
> here.

Victor - is there such a thing as the #profiler-start element anymore?
Flags: needinfo?(vporof)
(In reply to Brian Grinstead [:bgrins] from comment #8)
> 
> Victor - is there such a thing as the #profiler-start element anymore?

Don't think so.
Flags: needinfo?(vporof)
Comment on attachment 8548447 [details] [diff] [review]
Patch v2.1

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

::: browser/devtools/performance/performance.xul
@@ +88,5 @@
> +                  checked="true"
> +                  oncommand="PerformanceView._onRecordButtonClick()"/>
> +          <label value="&profilerUI.stopNotice2;"/>
> +        </hbox>
> +        <deck id="details-pane" flex="1">

You know you're having fun when building decks inside of decks.

Ignore this comment.
Updated the label bug (bug 1121086) with these for now, can change before we launch if we need. Check out comment #4 in that bug
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
Blocks: 1121086
No longer depends on: 1121086
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
good to go -- still seeing that padding issue in OSX, though.

Moved the labels patch to depend on this instead, as other things are dependent on this, can nail down what the text should be in that other patch which will just be locale changes
Attachment #8548447 - Attachment is obsolete: true
Attachment #8558258 - Flags: review?(vporof)
Comment on attachment 8558258 [details] [diff] [review]
1120699-perf-panels.patch

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

::: browser/devtools/performance/performance-view.js
@@ +11,5 @@
> +  _state: null,
> +
> +  // Mapping of state to selectors for different panes
> +  // of the main profiler view. Used in `PerformanceView.setState()`
> +  states: {

Wow

@@ +124,5 @@
> +   */
> +  _onRecordingStopped: function (_, recording) {
> +    this._unlockRecordButton();
> +
> +    // If this recording stopped is the current recording, set the

s/recording stopped/stopped recording/

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

Are we having both #record-button and .record-button? That's kinda' horrible.
Attachment #8558258 - Flags: review?(vporof) → review+
Comment on attachment 8558258 [details] [diff] [review]
1120699-perf-panels.patch

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

::: browser/devtools/performance/performance-view.js
@@ +11,5 @@
> +  _state: null,
> +
> +  // Mapping of state to selectors for different panes
> +  // of the main profiler view. Used in `PerformanceView.setState()`
> +  states: {

very state. also, we'll need to add another state for a console recording in progress

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

#record-button for the main button that persists for recording, and .record-button for anything that triggers the recording state. We could change #record-button to something like #main-recording-button, but I'm indifferent
https://hg.mozilla.org/mozilla-central/rev/e13315d46c4c
https://hg.mozilla.org/mozilla-central/rev/61e7df6a553e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Duplicate of this bug: 1101146
Depends on: 1129982
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.