1150112 - Timeline markers should respond to theme changes

Status: RESOLVED FIXED

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

Description

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

Right now timeline markers are only for "light theme" -- these should have a dark theme equivilent, and switch when toggling themes.

Comment 1

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

Attachment: 1150112-marker-colors.patch

Working on the colors in bug 947242.

Changed waterfall view and marker details to just use CSS. Markers overview now reacts to theme changes

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62091261a4ff

Comment 2

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

Comment on attachment 8587512 [details] [diff] [review]
1150112-marker-colors.patch

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

UUUUUGH these colors are horrible. My eyes are hurting.

I also expect this to break some tests. This comment: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/timeline/global.js#25

::: browser/devtools/shared/timeline/markers-overview.js
@@ +169,5 @@
>        let top = headerHeight + style.group * groupHeight + groupPadding / 2;
>        let height = groupHeight - groupPadding;
>  
> +      let color = getColor(style.colorName, this.theme);
> +

Nit: extra newline.

@@ +222,5 @@
>     * and updates the internal styling to match. Requires a redraw
>     * to see the effects.
>     */
>    setTheme: function (theme) {
> +    this.theme = theme = theme || "light";

= theme = theme = theme

::: browser/devtools/shared/timeline/waterfall.js
@@ +431,5 @@
>      sidebar.setAttribute("width", WATERFALL_SIDEBAR_WIDTH);
>      sidebar.setAttribute("align", "center");
>  
>      let bullet = this._document.createElement("hbox");
> +

Nit: extra newline.

::: browser/themes/shared/devtools/performance.inc.css
@@ +433,5 @@
> +.marker-details-bullet.highlight-pink,
> +.waterfall-marker-bar.highlight-pink,
> +.waterfall-marker-bullet.highlight-pink {
> +  background-color: var(--theme-highlight-pink);
> +  border-color: var(--theme-highlight-pink);

The background color shouldn't be the same as the border color. Otherwise, why even have a border?

Comment 3

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

Attachment: 1150112-marker-colors.patch

Looks like most of the timeline tests are still in /timeline -- made bug 1150696 for this.

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=341b66a172ea

Comment 4

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

Comment on attachment 8587655 [details] [diff] [review]
1150112-marker-colors.patch

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

::: browser/themes/shared/devtools/performance.inc.css
@@ -57,5 @@
>    width: 8px;
>    height: 8px;
>    margin: 0 8px;
> -  border: 1px solid;
> -  border-radius: 1px;

Should still keep the border-radius, no? Here and everywhere else.

Comment 5

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

Attachment: 1150112-marker-colors.patch

I like the squares, but wfm

Comment 6

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

Intermittents in try for bug 1134500, but looks like that got handled

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59e7246ffb37