1110550 - Enable theme toggling in new performance tool

Status: RESOLVED FIXED

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

Description

[Jordan Santell [:jsantell] [@jsantell]]

Need bug 1064373 for timeline themes toggling and bug 1049820 for category graph themes.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Flame graph themeing can be handled separately from this (and use it)

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1110550-dark-theme-perf.patch

Dark theme now toggles in the memory/markers view http://i.imgur.com/CYI8Oai.png

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=80d790a3d18f

Comment 4

[Victor Porof [:vporof][:vp]]

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.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1110550-dark-theme-perf.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b3509aef437

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/d7288ec2e107

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d7288ec2e107