1064373 - Make the Timeline overview graph themable

Status: VERIFIED FIXED

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

Description

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

Similar to bug 1049820. The graph is drawn a canvas, so theming isn't supported yet.

Comment 1

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

Attachment: 1064373-timeline-dark-theme.patch

Need to wait for force reload graph feature to land before running tests here, but looking good so far.

Comment 2

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

Attachment: Screen Shot 2014-12-11 at 4.51.12 PM.png

Comment 3

[Tim Nguyen :ntim]

Nice! Can you make sure the firebug theme gets handled as well ? (Probably with light theme colors)

Comment 4

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

Hm, unfamiliar with Firebug themeing, any quick pointers to that? Isn't that in a different repo?

Comment 5

[Tim Nguyen :ntim]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> Hm, unfamiliar with Firebug themeing, any quick pointers to that? Isn't that
> in a different repo?

Instead of (theme === "light"), use (theme !== "dark"), so all themes created by add-ons will have the light colors. To test the firebug theme, you can try firebug.next (v3 alpha).

Comment 6

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

Attachment: 1064373-timeline-dark-theme.patch

Thoughts on this approach, and r? if it looks good. If it looks good, should add Mike to r? as well for the css-colors change.

Comment 7

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

Comment on attachment 8537594 [details] [diff] [review]
1064373-timeline-dark-theme.patch

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

::: browser/devtools/timeline/widgets/markers-overview.js
@@ +58,5 @@
>  
>  MarkersOverview.prototype = Heritage.extend(AbstractCanvasGraph.prototype, {
>    clipheadLineColor: OVERVIEW_CLIPHEAD_LINE_COLOR,
>    selectionLineColor: OVERVIEW_SELECTION_LINE_COLOR,
> +  fixedHeight: OVERVIEW_HEADER_HEIGHT + OVERVIEW_BODY_HEIGHT,

Bad rebase. There's a fixedHeight getter in here.

@@ +222,5 @@
>        return scaledStep;
>      }
> +  },
> +
> +  setTheme: function (theme) {

Comment pls.

::: browser/devtools/timeline/widgets/memory-overview.js
@@ +72,5 @@
>      this.dataOffsetX = interval.startTime;
>      LineGraphWidget.prototype.setData.call(this, memory);
> +  },
> +
> +  setTheme: function (theme) {

Ditto.

::: toolkit/devtools/css-color.js
@@ +467,5 @@
> +    alpha = 1;
> +  }
> +
> +  let { r, g, b } = color._getRGBATuple();
> +  return "rgba(" + r + ", " + g + ", " + b + ", " + alpha + ")";

Very nice.

Comment 8

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

Attachment: 1064373-timeline-dark-theme.patch

nits fixed
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=043b48e89f7d

Comment 9

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

Will work without bug 1087877, but won't update colors between renders. Probably should hold off on landing until that lands.

Comment 10

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

Attachment: 1064373-timeline-dark-theme.patch

Rebased against some timeline changes, try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a7c232e14f74

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6001258c480c

Comment 13

[Andrei Vaida [:avaida]]

Verified fixed on Aurora 37.0a2 (2015-01-19) and Nightly 38.0a1 (2015-01-18), using Ubuntu 12.04 32-bit, Mac OS X 10.9.5 and Windows 8 32-bit.

There's one potential inconsistency on Linux - the "Memory" checkbox is using the system theme's hover style instead of the dark one that's used across Firefox's interface. It's worth mentioning that this is a behavior occurring in the toolbox options as well, here's a screenshot [1].

I'm not sure if this is intended, so please let me know if I should file a separate bug for it.


[1] http://i.imgur.com/Tqnbn0I.png

Comment 14

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

Thanks Andrei;

That's not intentional, but the Timeline tool is not being promoted or discussed, as it will be moved in the new performance tool (combination of Timeline, and the current profiler/performance tool) (bug 1075567) for the next uplift (Fx38). The themeable aspect of the timeline tool and current profiler will be applied to the new performance tool in bug 1110550, where we won't want issues like this, as the new performance tool will be heavily promoted. I think this is fine for now, but we'll have to investigate why this is and see if it also occurs in the new performance tool

Comment 15

[David Lawrence [:dkl]]

Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl