1589474 - The new performance panel icon in devtools should be highlighted in green when the profiler is running

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P3)

Description

[Michael Hoffmann]

In the new performance panel in devtools (devtools.performance.new-panel-enabled set to true), the profiler icon (chrome://devtools/skin/images/tool-profiler.svg) in the tab bar is not highlighted in green when the profiler is running.

The old performance panel tab and the accessibility tab are both highlighted in green when they are running, so the new tab should probably have the same behavior.

Comment 1

[Jacob Stenson]

Hi there, I would like to take on this bug. Could you give me some pointers as to how I would go about fixing it?

Comment 2

[Julien Wajsberg [:julienw]]

If I take the example of the Memory panel (it goes green when we check the option "record the call stacks"), this happens here: https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/devtools/client/framework/components/ToolboxTab.js#64
This means that to make the icon green, we need to add the id performance in this Set.
This is done by calling toolbox.highlightTool("performance"). See how the old panel is doing this too. I see it's done here: https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/devtools/client/performance/panel.js#108-114

In the new performance panel, the toolbox is accessible using the global gToolbox.

Now we need to see where we can call this. My suggestion is to create a react component, maybe called ToolboxHighlightController (suggestion). This component would get 2 prop:

• the recordingState,
• the toolbox
  It would render null, but in componentDidUpdate it would call toolbox.highlightTool or unhighlightTool depending on the value of the recordingState, and also comparing it with the previous value of the props (we have it as prevProps in the first argument of componentDidUpdate. Basically, if it's "recording" and the prevProps wasn't, we call highlightTool, and if it's not "recording" but prevProps was, we call unhighlightTool.

It would be rendered from Perf.js.

Now we need to take care also to one thing: depending on the context (depending whether the interface runs in the devtools' toolbox, or in the popup), the toolbox can be available, or absent. That's why I suggest to pass it as a prop to the component, so that the component doesn't have to check this. But this can be checked in Perf.js when the component is inserted, something like this:

gToolbox ? ToolboxHighlightController({ toolbox: gToolbox, recordingState }) : null

I hope it makes sense!
Please tell me if you need anymore information :)

Comment 3

[Jacob Stenson]

Thanks for your reply. I just have a few questions before I start work on this.
Where would I create the toolboxHighlightController react component?
Also where is the Perf.js file? And where would I call that code towards the end of your comment in that file?
One final question: If i create this new component and pass in data through props and put the necessary code inside said component while rendering nothing, how will that change the the profiler icon in the tab bar?

Apologies for the mass of questions. Currently I am unsure how to complete this.

Comment 4

[Julien Wajsberg [:julienw]]

All the code for this panel is in devtools/client/performance-new. There's a subdirectory components that contains the React components for this panel. This is where you can create the new component, and where you'll find the file Perf.js.

One final question: If i create this new component and pass in data through props and put the necessary code inside said component while rendering nothing, how will that change the the profiler icon in the tab bar?

This is done because highlightTool will add the panel name to the Set that's used in the code that deals with the icon (see links to these bits of code in my previous comment).

Just to be extra clear: this bug isn't about the icon in the tab bar, but the icon in the devtools' tab bar.

Please tell me if you need anything more! I understand that this bug is a bit involved, so if you can't make it, it's fine, but please say it now :-) But I think you can do it!

Comment 5

[Jacob Stenson]

Hi there, I believed I have solved the bug, but in a different way to how you have described.
I have added the code "gToolbox.highlightTool("performance")" inside the "handleProfilerStarting" function in the Perf.js file. This highlights the icon in the devtools' bar as green when the user clicks "Start recording". I have also added the code "gToolbox.unhighlightTool("performance")" in the "handleProfilerStopping" function so that when the user clicks "Stop and grab the recording" the icon is unhighlighted. Does this sound okay to you? From what I gather this is what you were aiming for, and requires less code.

Comment 6

[Julien Wajsberg [:julienw]]

Your approach works, but the problem with it is that if for some reason the recording state is changed in another location it won't get the result.
My proposal is more code but it's more correct and more future proof. Also it's more easily testable: small components with one responsibility makes it easier :-)
Hope that makes sense!

Comment 7

[Jacob Stenson]

Hi again. I am currently working on implementing ToolboxHighlightController.js, although I am running into an issue when I go to require the ToolboxHighlightController.js file from the Perf.js file. I am running into an error that is telling me the module is not found? I have double checked I am requiring from the correct path and checked file names yet am still getting the error. The file also does not appear to show up when I call the command 'hg diff', so I was wondering if you could give me any idea with regards to what I'm doing wrong? Thanks

Comment 8

[Julien Wajsberg [:julienw]]

About hg diff, I believe that's because it's a new file, and you need to "add" it first with the command hg add.

I think you need to give me the exact lines and where the files are located :) Otherwise it's difficult to help you. Attaching a diff as an attachment to the bug would help too.

Comment 9

[Jacob Stenson]

Attachment: unknown.png

Hi again, as can be seen from my attachment. Inside the Perf.js file I am requiring the ToolBoxHighlightController.js from its location in the files (as can be seen on the right). Yet I am getting an 'is not found' error when running the browser and showing the devtools window. Any ideas?

Comment 10

[Jacob Stenson]

Attachment: hgdiffOfIssue.txt

See attached an hg diff containing my changes as well.

Comment 11

[Julien Wajsberg [:julienw]]

Ah, I believe you need to add the filename to the file moz.build in the components directory.
I think this is how our build system decides what to package inside Firefox.

Comment 12

[Jacob Stenson]

The file is showing up and is not giving me errors now. Thank you. I'll start work on this now!

Comment 13

[Jacob Stenson]

Attachment: Bug 1589474 - The new performance panel icon in devtools should be highlighted in green when the profiler is running, r=julienw

Comment 14

[Julien Wajsberg [:julienw]]

Hey Jacob, I'm not sure you've seen my message on your patch. I wanted to let you know that the planned refactoring has been done and that you can apply your good changes on top of the latest code that's currently in the repository mozilla-central.
Please tell me if you don't intend to move forward with this so that I can propose it to somebody else :-)
Thanks a lot !

Comment 15

[Jacob Stenson]

Hi Julien, I dont intend to move forward with this.

Comment 16

[Julien Wajsberg [:julienw]]

Thanks.
Then this is now up for grabs!
Basically the existing patch from Jacob works fine but needs to be applied on the current code.

Comment 17

[nightwarriorftw]

Hi Julien,
I am new to this project and want to work on this issue. I have read the whole comment thread and have some few questions. I forked mozilla-central but don't have Perf.Js in the dir. So, I guess I have to make new. RIght ? Or I have to make a new ToolboxHighlighterController.js

Comment 18

[Julien Wajsberg [:julienw]]

Hey Aman, thanks for looking at this!
That's because Perf.js has been split in 2 components: ProfilerEventHandling.js and DevToolsAndPopup.js. The changes will need to be applied to these components instead. I can't look more precisely now but I think you should be able to find it yourself. Thanks!

Comment 19

[nightwarriorftw]

Attachment: Bug 1589474 - React component for new performance icon, r=julienw

Comment 20

[nightwarriorftw]

Attachment: Bug 1589474 - The new performance panel icon in devtools should be highlighted in green when the profiler is running, r=julienw

Depends on D59919

Comment 21

[nightwarriorftw]

Attachment: Bug 1589474 - The new performance panel icon in devtools should be highlighted in green when the profiler is running, r=julienw

Depends on D59951

Comment 22

[nightwarriorftw]

Attachment: Bug 1589474 - Removed log statement, r=julienw

Depends on D60300

Comment 23

[nightwarriorftw]

Attachment: Bug 1589474 - Fixed eslint errors, r=julienw

Depends on D60303

Comment 24

[nightwarriorftw]

Attachment: Bug 1589474 - The new performance panel icon in devtools should be highlighted in green when the profiler is running, r=julienw

---

---

---

---

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:julienw, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 26

[Julien Wajsberg [:julienw]]

It would still be good to do this.

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 28

[Abhijeet Chawla [:ff2400t]]

hello, i would like to work on this can someone Assign this to me

Comment 29

[Julien Wajsberg [:julienw]]

@Abhijeet Chawla: I see you're already assigned and provided a patch to bug 1731635, do you still want to look at this one? I believe you'll have to start from scratch, I can give some updated instructions if needed. Please tell me and I can assign it to you!

Comment 30

[Abhijeet Chawla [:ff2400t]]

Attachment: Bug 1589474 The new performance panel icon in devtools should be highlighted in green when the profiler is running r=julienw

Added the ToolboxHighlightController.js to Highlighing when the Profiler is On

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c940bd02fe7
The new performance panel icon in devtools should be highlighted in green when the profiler is running r=julienw

Comment 32

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/5c940bd02fe7