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

Status: RESOLVED FIXED

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

Description

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

Like the original profiler, on recording and waiting and loaded states, we should display something other than the empty waterfall/calltree/flamegraph views.

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch v1

Needs rebasing on top of bug 1111004, and maybe some tests.

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch v2

Rebased and added test.

Comment 3

[Tim Nguyen :ntim]

Attachment: Patch v2.1

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.

Comment 4

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

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

Comment 5

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

Attachment: padding issue in OSX

Also looks like there's some padding issue on OSX for the clock button

Comment 6

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

Great stuff so far :) hit me with another r? once those issues are resolved

Comment 7

[Brian Grinstead [:bgrins]]

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.

Comment 8

[Brian Grinstead [:bgrins]]

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

Comment 9

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

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

Comment 10

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

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.

Comment 11

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

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

Comment 12

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

Attachment: 1120699-perf-panels.patch

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

Comment 13

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

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

Comment 14

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

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.

Comment 15

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

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

Comment 16

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

https://hg.mozilla.org/integration/fx-team/rev/e13315d46c4c

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e13315d46c4c
https://hg.mozilla.org/mozilla-central/rev/61e7df6a553e