Closed
Bug 1110550
Opened 10 years ago
Closed 10 years ago
Enable theme toggling in new performance tool
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
Need bug 1064373 for timeline themes toggling and bug 1049820 for category graph themes.
Assignee | ||
Updated•10 years ago
|
Blocks: perf-tool-v2
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Updated•10 years ago
|
Blocks: enable-perf-tool
Updated•10 years ago
|
No longer blocks: enable-perf-tool
Updated•10 years ago
|
Assignee | ||
Comment 2•10 years ago
|
||
Flame graph themeing can be handled separately from this (and use it)
No longer depends on: 1121180
Assignee | ||
Comment 3•10 years ago
|
||
Dark theme now toggles in the memory/markers view http://i.imgur.com/CYI8Oai.png https://treeherder.mozilla.org/#/jobs?repo=try&revision=80d790a3d18f
Attachment #8584872 -
Flags: review?(vporof)
Comment 4•10 years ago
|
||
Comment on attachment 8584872 [details] [diff] [review] 1110550-dark-theme-perf.patch Review of attachment 8584872 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/test/browser.ini @@ +80,5 @@ > [browser_perf-recording-selected-01.js] > [browser_perf-recording-selected-02.js] > [browser_perf-recording-selected-03.js] > [browser_perf-recording-selected-04.js] > +[browser_perf-theme-toggle-01.js] I like how you add -01 to file names because hg and renaming. ::: browser/devtools/performance/views/overview.js @@ +103,5 @@ > + > + if (this.markersOverview) { > + this.markersOverview.setTheme(theme); > + if (options.redraw) { > + this.markersOverview.refresh({ force: true }); Why not just call foo.refresh({ force: options.redraw }). There's logic in the widgets that prevent an unnecessary redraw anyway, so no need for the extra conditionals here. @@ +110,5 @@ > + > + if (this.memoryOverview) { > + this.memoryOverview.setTheme(theme); > + if (options.redraw) { > + this.memoryOverview.refresh({ force: true }); Ditto. ::: browser/themes/shared/devtools/widgets.inc.css @@ +908,5 @@ > } > > .line-graph-widget-tooltip { > position: absolute; > + background: rgba(255,255,255,0.9); Should use a theme color for this. The tooltips should be dark on the dark theme, with white text. Since this is just CSS, changing it is trivial I think.
Attachment #8584872 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8584872 [details] [diff] [review] 1110550-dark-theme-perf.patch Review of attachment 8584872 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/test/browser.ini @@ +80,5 @@ > [browser_perf-recording-selected-01.js] > [browser_perf-recording-selected-02.js] > [browser_perf-recording-selected-03.js] > [browser_perf-recording-selected-04.js] > +[browser_perf-theme-toggle-01.js] we have fun ::: browser/devtools/performance/views/overview.js @@ +103,5 @@ > + > + if (this.markersOverview) { > + this.markersOverview.setTheme(theme); > + if (options.redraw) { > + this.markersOverview.refresh({ force: true }); This was to prevent rendering on instantiation, but if there's no data, then yeah i guess it shouldn't render at all ::: browser/themes/shared/devtools/widgets.inc.css @@ +908,5 @@ > } > > .line-graph-widget-tooltip { > position: absolute; > + background: rgba(255,255,255,0.9); Yeah, I still don't like how it looks, but is definitely more readable with this change. But all the tooltips for this should change to the light-text-on-dark-bg. Opening another bug for that
Assignee | ||
Comment 6•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b3509aef437
Attachment #8584872 -
Attachment is obsolete: true
Attachment #8586205 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Blocks: perf-polish
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d7288ec2e107
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d7288ec2e107
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Updated•9 years ago
|
Flags: qe-verify+
Updated•9 years ago
|
Flags: qe-verify+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•