1562082 - Always enable profiler popup icon in local builds

Status: NEW

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

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Most devtools have shortcuts.

I propose we have one for the profiler popup icon, which would be useful when working on local builds that regularly use clean profiles.

The obvious choice would be Ctrl+Shift+1, if not used elsewhere. I guess we'd need to know if the legacy add-on is there, so we don't steal its shortcut.

Bonus points (possibly a separate bug) : Automatically enable the icon if profiler is already started (e.g., for startup profiling).

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

After discussing with the team:

• Yet-another shortcut is probably not good, especially as a very small proportion of our users will use the Profiler, and the rest may be confused with the strange icon appearing.
• A useful compromise would be to just enable the popup icon in local builds, and maybe also in Nightly builds.

In the meantime the icon can be programmatically enabled with the "devtools.performance.popup.feature-flag" pref, which can be set with ./mach run --setpref "devtools.performance.popup.enabled=true"

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Gerald Squelart [:gerald] from comment #1)

...
In the meantime the icon can be programmatically enabled with the "devtools.performance.popup.feature-flag" pref, which can be set with ./mach run --setpref "devtools.performance.popup.enabled=true"

This doesn't seem to work on new profiles (until I manually enable it once). 😪

Comment 3

[Julien Wajsberg [:julienw]]

Ah is it possible that this doesn't work on the prefs that aren't defined in all.js? This pref is defined in code only :/

Comment 4

[Julien Wajsberg [:julienw]]

Maybe Andrew would know about this.

Comment 5

[Andrew Halberstadt [:ahal]]

The --setpref option works by writing a user.js file in the profile. So all.js shouldn't affect anything on the tooling side, but maybe it has implications with Firefox that I'm not aware of. On first run (when the pref doesn't work), open the profile and inspect user.js to verify if the pref is in there or not.

Also reading the mach run source, looks like --setpref is only supported if you don't pass in -P/--profile.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Gerald Squelart [:gerald] from comment #1)

After discussing with the team:

• Yet-another shortcut is probably not good, especially as a very small proportion of our users will use the Profiler, and the rest may be confused with the strange icon appearing.
• A useful compromise would be to just enable the popup icon in local builds, and maybe also in Nightly builds.

We do have some 'local development only' shortcuts (like ctrl+alt+r for doing a restart plus session restore): https://searchfox.org/mozilla-central/source/browser/base/content/browser-development-helpers.js. I think it'd be totally fine to add global shortcut for the profile either directly in browser-development-helpers, or guarded behind !AppConstants.MOZILLA_OFFICIAL in the profile code somewhere, as per https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/browser/base/content/browser.xhtml#95-97.

Comment 7

[:Harald Kirschner :digitarald]

A useful compromise would be to just enable the popup icon in local builds, and maybe also in Nightly builds.

I like that solution as well. Nightly might also be fine, which is our prime performance contributor base (at least testing it out and gathering feedback).

Comment 8

[:Harald Kirschner :digitarald]

We touched on this a few times during planning and I didn't hear concerns. Lets enable this for local builds first and then see if devs would also like it for Nightly.

Comment 9

[:Harald Kirschner :digitarald]

Related, bug 1385452 landed the restart button for local builds.

Comment 10

[Greg Tatum [:gregtatum]]

Attachment: Bug 1562082 - Enable profiler menu button on local builds; r?canaltinova

This patch will turn on the profiler button for all local builds.

Depends on D66407

Comment 11

[:Harald Kirschner :digitarald]

I mentioned this work to mattn. He pointed out that adding icons for builds has to be done carefully to avoid changing the toolbar for when tests run. He recommended to tie it into mach run.

Comment 12

[Matthew N. [:MattN]]

Yeah, we've had test failures in the past due to toolbar overflow and UITour availableTargets. The toolbar item could also skew local Talos results.

browser-development-helpers.js is what I was thinking of but that also seems to use AppConstants.MOZILLA_OFFICIAL. It looks like the proposed patch also aligns with how we handle debugging prefs. Maybe we don't actually do any additional changes for mach run anymore? bgrins would know.

I guess as long as tests pass in automation and people don't complain too much about local test failures due to different screen resolutions causing toolbar overflow then you're probably fine.

Comment 13

[Greg Tatum [:gregtatum]]

Yeah, I may hook into mach run. I'm holding off on landing this patch for now.

Here's where that would happen:
https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/python/mozbuild/mozbuild/mach_commands.py#1002-1146