Use the SideMenuWidget in the Profiler

RESOLVED FIXED in Firefox 24

Status

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: vporof, Assigned: anton)

Tracking

unspecified
Firefox 24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

We want to move away from splitview.jsm.
Depends on: 879008
Blocks: 879008
No longer depends on: 879008
Assignee: nobody → anton
Status: NEW → ASSIGNED
I'll make up a gist with how it should be used. If you want it to be at all useful, you'll need to wrap it, as explained in the SideMenuWidget constructor comment.
That's be great. I was asking Heather yesterday about it.
(In reply to Anton Kovalyov (:anton) from comment #2)
> That's be great. I was asking Heather yesterday about it.

https://gist.github.com/victorporof/5749386
Depends on: 882054
Posted patch Re-implement Profiler sidebar (obsolete) — Splinter Review
Re-implemented Profiler sidebar to use the new SideMenuWidget and changed its design a little bit. Working on fixing tests right now because old tests relied on the DOM structure but the code is ready for review.

Every item in the sidebar now has two lines (like in the style editor): name of the profile and its status (Idle, Running or Complete). Later we can put a little link there to export a profile or something else.

Oh and I also need to localize strings—forgot about that.
Attachment #763269 - Flags: ui-review?(shorlander)
Attachment #763269 - Flags: review?(vporof)
Posted image New sidebar design
Here's a screenshot of the new sidebar look.
Comment on attachment 763269 [details] [diff] [review]
Re-implement Profiler sidebar

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

Beautifully done.
How did you find the API? Was there anything confusing that you'd like to change?

::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +273,5 @@
> +  this.node = new SideMenuWidget(el);
> +  this.state = ProfilerView.IDLE;
> +}
> +
> +ViewHelpers.create({ constructor: ProfilerView, proto: MenuContainer.prototype }, {

I'll rebase bug 882054 on yours. Or we can do it the other way around. Let's race.

@@ +275,5 @@
> +}
> +
> +ViewHelpers.create({ constructor: ProfilerView, proto: MenuContainer.prototype }, {
> +  getItemByProfile: function (profile) {
> +    return this.orderedItems.filter((item) => item.attachment === profile.uid)[0];

You can also use the getItemForPredicate method after bug 823577.
Attachment #763269 - Flags: review?(vporof) → review+
> How did you find the API? Was there anything confusing that you'd like to change?

Overall it was pretty good. One thing that bugged me is why custom DOM nodes replace both label and value? I think it would be easier if label could be either a string or a DOM node. Then I can have my own presentation and still use stuff like getItemByValue.

Also, in your gist you should mention that not everything landed yet. :) I was trying to use getItemForPredicate before realizing that it's not there yet.
(In reply to Anton Kovalyov (:anton) from comment #7)
> > How did you find the API? Was there anything confusing that you'd like to change?
> 
> Overall it was pretty good. One thing that bugged me is why custom DOM nodes
> replace both label and value? I think it would be easier if label could be
> either a string or a DOM node. Then I can have my own presentation and still
> use stuff like getItemByValue.

I know, this has been bugging my brain as well for a while. I'll file a bug.

> Also, in your gist you should mention that not everything landed yet. :) I
> was trying to use getItemForPredicate before realizing that it's not there
> yet.

There's a mention on top // In effect after bug 882054.
(In reply to Victor Porof [:vp] from comment #8)
> (In reply to Anton Kovalyov (:anton) from comment #7)

Filed bug 884006.
All tests pass, all strings localized.
Attachment #763269 - Attachment is obsolete: true
Attachment #763269 - Flags: ui-review?(shorlander)
Attachment #763845 - Flags: review?(vporof)
Comment on attachment 763845 [details] [diff] [review]
New sidebar for the JavaScript Profiler

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

Yup. Few nits below if you care about them, but r+.

::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +395,3 @@
>      // Local profiling needs to make the target remote.
> +    let target = this.target;
> +    let promise = !target.isRemote ?  target.makeRemote() : Promise.resolve(target);

Opportunistic change, but okay.
Nit: there's an extra whitespace after "?"

::: browser/devtools/profiler/cmd-profiler.jsm
@@ +83,4 @@
>          throw gcli.lookup("profilerAlreadyFinished");
>        }
>  
> +      let item = panel.sidebar.getItemByProfile(profile);

Might want to keep the switchToProfile helper public API, since this looks like a bit of boilerplate, but you might not care.

@@ +87,5 @@
> +
> +      if (panel.sidebar.selectedItem === item) {
> +        profile.start();
> +      } else {
> +        panel.on("profileSwitched", () => profile.start());

These events are synchronous to the best of my knowledge, but it's ok to be safe.

::: browser/devtools/profiler/test/browser_profiler_console_api_named.js
@@ +49,3 @@
>  
> +  is(getSidebarItem(1, panel).attachment.state, PROFILE_IDLE);
> +  is(getSidebarItem(2, panel).attachment.name, "Second");

It looks like this test (and similar) would fail on non en-US builds, and I've seen some contributors being confused because of such things. Would it be hard to localize "Second" and "Third" here (assuming they're actually part of UI, but I may be mistaken).

::: browser/devtools/profiler/test/browser_profiler_profiles.js
@@ +51,5 @@
> +  let profile = gPanel.profiles.get(uid);
> +  let data = gPanel.sidebar.getItemByProfile(profile).attachment;
> +
> +  is(data.uid, uid, "UID is correct");
> +  is(data.name, "Custom Profile", "Name is correct on the label");

Can't wait for bug 884006.
Attachment #763845 - Flags: review?(vporof) → review+
Comment on attachment 763845 [details] [diff] [review]
New sidebar for the JavaScript Profiler

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

::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +395,3 @@
>      // Local profiling needs to make the target remote.
> +    let target = this.target;
> +    let promise = !target.isRemote ?  target.makeRemote() : Promise.resolve(target);

Oops.

::: browser/devtools/profiler/cmd-profiler.jsm
@@ +87,5 @@
> +
> +      if (panel.sidebar.selectedItem === item) {
> +        profile.start();
> +      } else {
> +        panel.on("profileSwitched", () => profile.start());

The problem is, if selectedItem === item, selectedItem = item won't do anything including emiting the profileSwitched event.

::: browser/devtools/profiler/test/browser_profiler_console_api_named.js
@@ +49,3 @@
>  
> +  is(getSidebarItem(1, panel).attachment.state, PROFILE_IDLE);
> +  is(getSidebarItem(2, panel).attachment.name, "Second");

Why would they fail on non en-US build if I explicitly set their custom names to "Second" and "Third" earlier in the test? There are cases where I check auto-generated names (like "Profile 1") but its not a new thing. So I think I'll file a follow-up bug to localize Profiler tests.
Fixed a whitespace typo. Carrying over r+.
Attachment #763845 - Attachment is obsolete: true
Attachment #764394 - Flags: review+
Follow up for tests: bug 884559.
(In reply to Anton Kovalyov (:anton) from comment #12)

> ::: browser/devtools/profiler/test/browser_profiler_console_api_named.js
> @@ +49,3 @@
> >  
> > +  is(getSidebarItem(1, panel).attachment.state, PROFILE_IDLE);
> > +  is(getSidebarItem(2, panel).attachment.name, "Second");
> 
> Why would they fail on non en-US build if I explicitly set their custom
> names to "Second" and "Third" earlier in the test? There are cases where I
> check auto-generated names (like "Profile 1") but its not a new thing. So I
> think I'll file a follow-up bug to localize Profiler tests.

Sorry, I misread the test. I though those strings ("First", "Second") were localized but hardcoded in tests. There's really no need to localize tests afaik, you can close WONTFIX bug 884559.
https://hg.mozilla.org/mozilla-central/rev/ce4e72421c35
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
No longer blocks: 879008
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.