828038 - Better record/stop UI

Status: RESOLVED FIXED

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

Description

[Anton Kovalyov (:anton)]

Current record/stop UI is rather primitive and we can do better. AFAIK, there were no design decisions made yet, so feel free to chime in.

Comment 1

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ffa37079e829

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Er, that commit had the wrong bug number!

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Yeah, sorry, that commit should have been labeled as bug 828039. I've backed it out and re-landed it with the right bug number.

Comment 4

[Anton Kovalyov (:anton)]

Attachment: Change recording UI and behavior

So I was playing with better recording UI and accidentally refactored everything. In this patch:

1. New recording UI that is similar to other profiles. There's only one button to start/stop profiling. I removed the functionality to create profiles without starting them because a) it was kinda useless and b) it simplified code a lot (this patch deletes more code than it adds). You can still use console API to run multiple profiles in parallel but now they won't appear in the UI until they're completed.
2. Changed GCLI commands to reflect changes in (1).
3. Refactored a bunch of code to remove unnecessary complexity and removed some tests that became obsolete because of (1).
4. (!!!) Got rid of JSMs. All profiler files are now normal RequireJS files. It looks so much better.

Since I touched some code outside of the profiler I'm asking multiple people for reviews.

Joe: GCLI changes (2);
Dave: RequireJS transition (4);
Rob and/or Panos: everything else.

Thanks!

Comment 5

[Rob Campbell [:rc] (:robcee)]

needs more reviewers.

Comment 6

[Rob Campbell [:rc] (:robcee)]

adding 828038 to series file
applying 828038
patching file browser/devtools/profiler/ProfilerPanel.jsm
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/profiler/ProfilerPanel.jsm.rej
patching file browser/devtools/profiler/cmd-profiler.jsm
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/profiler/cmd-profiler.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 828038
renamed 828038 -> profiler-refactor.patch

needs a rebasin'.

Comment 7

[Rob Campbell [:rc] (:robcee)]

these rejects just look like hg removes that didn't apply somehow? Is that the case?

Comment 8

[Rob Campbell [:rc] (:robcee)]

[14:35:57.083] ReferenceError: MenuContainer is not defined @ resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/profiler/sidebar.js:24

Comment 9

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> [14:35:57.083] ReferenceError: MenuContainer is not defined @
> resource://gre/modules/commonjs/toolkit/loader.js ->
> resource:///modules/devtools/profiler/sidebar.js:24

I'm sorry. Bug 882054.

Comment 10

[Anton Kovalyov (:anton)]

Attachment: Change recording UI and behavior

Rebased.

Comment 11

[Anton Kovalyov (:anton)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=dcb57a60480f

Comment 12

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

Comment on attachment 766887 [details] [diff] [review]
Change recording UI and behavior

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

::: browser/devtools/profiler/helpers.js
@@ +11,5 @@
> +
> +/**
> + * Localization helper methods.
> + */
> +let L10N = {

ViewHelpers.L10N bro. Use it. IMHO this file should begone.

::: browser/devtools/profiler/sidebar.js
@@ +17,5 @@
> +  EventEmitter.decorate(this);
> +
> +  this.document = el.ownerDocument;
> +  this.widget = new SideMenuWidget(el);
> +  this.widget.notice = "Click the stopwatch button.";

l10n.

@@ +46,5 @@
> +    });
> +  },
> +
> +  getElementByProfile: function (profile) {
> +    return this.document.querySelector("#profile-" + profile.uid);

I think just profile.target would work.

Comment 13

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

(In reply to Victor Porof [:vp] from comment #12)

> I think just profile.target would work.

..where profile is the thing returned when you do this.push([box], ...). But I guess you're not really using that anywhere, so in this particular case it'd be more like this.getItemByProfile(profile).target, which is a mouthful. Meh, you decide what you like.

Comment 14

[Anton Kovalyov (:anton)]

Attachment: Change recording UI and behavior

Rebased, fixed a bug that was failing Try and incorporated Victor's feedback.

Comment 15

[Anton Kovalyov (:anton)]

Attachment: Change recording UI and behavior

Last rebase restored a ProfilerHelpers import which is invalid now. Fixed that. Try: https://tbpl.mozilla.org/?tree=Try&rev=bed9d3a9ee3d

Comment 16

[Stephen Horlander [:shorlander]]

Attachment: Icon with active state

Comment 17

[Dave Camp (:dcamp)]

Comment on attachment 768034 [details] [diff] [review]
Change recording UI and behavior

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

Require stuff looks fine, although I'm pretty surprised that require isn't working for you in mochitests, we already use it all over the place.

Comment 18

[Anton Kovalyov (:anton)]

Attachment: Change recording UI and behavior

Added new icon. Carrying over dcamp's r+. Gonna land this patch since try push was successful.

Comment 19

[Anton Kovalyov (:anton)]

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

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/addbd7cf1741