Closed
Bug 1164281
Opened 9 years ago
Closed 9 years ago
Have an "experiments" flag in about:config that enable UI options in performance tools
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox40 fixed, firefox41 fixed)
RESOLVED
FIXED
Firefox 41
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(2 files, 1 obsolete file)
14.28 KB,
patch
|
vporof
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
9.76 KB,
image/jpeg
|
Details |
(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 | ||
Updated•9 years ago
|
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
Enabled on nightly by default, displays things like record memory, jit opts, etc.
Attachment #8604992 -
Flags: review?(vporof)
Assignee | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a95c791202f
Assignee | ||
Comment 3•9 years ago
|
||
Most likely the retro stuff will get removed shortly, too.
Comment 4•9 years ago
|
||
(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.
Assignee | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Blocks: perf-tool-v2
Assignee | ||
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/43efa0312b65
Whiteboard: [fixed-in-fx-team]
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/43efa0312b65
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Comment 12•9 years ago
|
||
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}
Updated•9 years ago
|
Blocks: perf-40-uplifts
Updated•9 years ago
|
Flags: qe-verify-
Comment 13•9 years ago
|
||
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?
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/874dfde6ee0b
status-firefox40:
--- → fixed
Comment 15•9 years ago
|
||
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 16•9 years ago
|
||
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+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•