Closed Bug 1164281 Opened 5 years ago Closed 5 years ago

Have an "experiments" flag in about:config that enable UI options in performance tools

Categories

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

37 Branch
defect
Not set

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(2 files, 1 obsolete file)

(prompted by digitalrald/djvj's discussion)

Right now, we have two things "hidden" in about:config flags.

devtools.performance.ui.enable-memory
devtools.performance.ui.show-jit-optimizations

Memory recording is rather unstable and needs a lot more polish work done, and we want to hold off for the initial release on June 2nd for this. For the JIT Opts stuff, we also want to do more work there in the next release. That being said, these are still useful for Gecko/SM hackers and good dogfooding as well. These prefs can be set to true, but if you don't always want these options on, it's a bit annoying turning them on/off in about:config.

Would it be desirable to have a flag `devtools.performance.ui.experiments` or something like that, which would display the ability to easily turn on/off things like memory recording and JIT opts via the normal options menu? We still don't want unstable things like memory recording being enabled and causing a negative experience, but still want the option for more internal users. Thoughts?
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attached patch 1164281-experimental.patch (obsolete) — Splinter Review
Enabled on nightly by default, displays things like record memory, jit opts, etc.
Attachment #8604992 - Flags: review?(vporof)
Most likely the retro stuff will get removed shortly, too.
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0)
...
> Would it be desirable to have a flag `devtools.performance.ui.experiments`
> or something like that, which would display the ability to easily turn
> on/off things like memory recording and JIT opts via the normal options
> menu? We still don't want unstable things like memory recording being
> enabled and causing a negative experience, but still want the option for
> more internal users. Thoughts?

If that solves your immediate problem, sure. On my list for Whistler is to talk about shipping experimental features and providing settings ui to toggle them. IMO the bar is too high to ship right now, because we don't have a way to expose users to experimental things.
Totally, I think this is something all tools can benefit from, but don't want things like this making its way up releases unless explicitly opting in, even by hiding via UI
Comment on attachment 8604992 [details] [diff] [review]
1164281-experimental.patch

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

many comment

::: browser/devtools/performance/performance.xul
@@ +53,5 @@
>                  data-pref="invert-call-tree"
>                  label="&profilerUI.invertTree;"
>                  tooltiptext="&profilerUI.invertTree.tooltiptext;"/>
>        <menuitem id="option-invert-flame-graph"
> +                class="retro-option"

The name `retro-option` sounds misleading to me. It's something for *not* retro, so the opposite of what you'd expect.

Since we're removing this later anyway, I don't care that much, but "unavailable-in-retro-mode" or something similar would be infinitely better.

::: browser/devtools/performance/views/toolbar.js
@@ +23,3 @@
>      });
>  
> +    this._toggleExperimentalUI();

Nit: please add a comment here describing what this experimental UI is about and why we need it.

Nit2: missing param.

@@ +23,5 @@
>      });
>  
> +    this._toggleExperimentalUI();
> +    // TODO bug 1160313 get rid of this option completely and make these always on.
> +    this._toggleRetroUI();

Nit: missing param.

@@ +87,5 @@
> +   * Fired when `devtools.performance.ui.experimental` is changed, or
> +   * during init. Toggles the visibility of experimental performance tool options
> +   * in the UI options.
> +   *
> +   * Sets or removes "experimental-enabled" on the menu and main elements, 

Nit: ws

@@ +96,5 @@
> +   *
> +   * @param {boolean} show
> +   */
> +  _toggleExperimentalUI: function (show) {
> +    let isEnabled = show == null ? PerformanceController.getOption("experimental") : show;

Why do these two functions even take a param? We expect to have one already all the time, right, before calling them?

So, nit: always have the `show` param.

@@ +112,5 @@
> +   * during init. Toggles the visibility of "retro-mode" options, which disable
> +   * all new options in Fx40+ to simulate the previous profiler.
> +   * in the UI options.
> +   *
> +   * Sets or removes "retro-mode-enabled" on the menu and main elements, 

Nit: ws

@@ +120,5 @@
> +   *
> +   * @param {boolean} show
> +   */
> +  _toggleRetroUI: function (show) {
> +    let isEnabled = show == null ? PerformanceController.getOption("retro-mode") : show;

Same here, make the `show` param mandatory.

::: browser/themes/shared/devtools/performance.inc.css
@@ +702,5 @@
> +.experimental-enabled .experimental-option {
> +  display: block;
> +}
> +.experimental-enabled menuitem.experimental-option::before {
> +  display: block;

Instead of having display: block both here and everywhere else, along with a display: none, it's easier to just do this:

#performance-options-menupopup:not(.experimental-enabled) .experimental-option
#performance-options-menupopup:not(.experimental-enabled) .experimental-option::before {
  display: none;
}

This makes the intent much clearer. Also, you probably know that display: block may not be the default display mode.
Attachment #8604992 - Flags: review?(vporof)
Comment on attachment 8604992 [details] [diff] [review]
1164281-experimental.patch

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

Looks like we're getting rid of retro mode moving forward, so will just wrap up bug 1160313 first for these.
Depends on: 1160313
All the retro mode stuff has been removed, comments addressed; 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc1df403495
Attachment #8604992 - Attachment is obsolete: true
Attachment #8606558 - Flags: review?(vporof)
Comment on attachment 8606558 [details] [diff] [review]
1164281-experimental.patch

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

Very nice.
Attachment #8606558 - Flags: review?(vporof) → review+
https://hg.mozilla.org/mozilla-central/rev/43efa0312b65
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Attached an example of some nicer styling for the experimental options:

Code from my themes:
.experimental-option{
	font-style:italic;
	background-image:repeating-linear-gradient(-45deg,transparent,transparent 10px,rgba(128,128,128,.2) 10px,rgba(128,128,128,.2) 20px)}
.experimental-option::after{
	content:"";
	display:inline-block;
	width:16px;
	height:16px;
	background:url(chrome://global/skin/icons/warning-16.png) center no-repeat}
Flags: qe-verify-
Comment on attachment 8606558 [details] [diff] [review]
1164281-experimental.patch


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8606558 - Flags: approval-mozilla-aurora?
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8606558 [details] [diff] [review]
1164281-experimental.patch

Change approved to skip one train as part of the spring campaign.
Attachment #8606558 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.