917389 - Re-enable browser_css_color.js on Linux 32 bit tests

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Basically, a test was disabled due to bug 916544.

Setting rgba(255, 255, 255, 0.7) on an element and then using getComputedStyle to get the color results in rgba(255, 255, 255, 0.698).

255 * 0.7 = 178.5 which gets rounded down to 178 on 32 bit Linux.
This then gets divided by 255 which results in == 0.698.

Because of the performance impact of improving the accuracy we will simply use an alpha of 0.6 in browser_css_color.js so that it can be re-enabled on Linux builds.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: reenable_browser_css_color_js_917389.patch

Try containing these changes:
https://tbpl.mozilla.org/?tree=Try&rev=32fa435700ca

Re-enabled test on Linux and changed the offending alpha value to 0.6. I have also set the css-color.js to use values directly if the authored style is an rgb or rgba color.

Comment 2

[Heather Arthur [:harth]]

Comment on attachment 806089 [details] [diff] [review]
reenable_browser_css_color_js_917389.patch

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

One question, and one suggestion.

::: browser/devtools/shared/css-color.js
@@ +154,5 @@
>      if (!this.hasAlpha) {
> +      // Because of accuracy issues we use the authored values if this is a rgb
> +      // color.
> +      if (this.authored.startsWith("rgb(")) {
> +        let [, r, g, b] = /^\brgb\(([\d.]+),\s*([\d.]+),\s*([\d.]+)\)$/gi.exec(this.authored);

Put this regex at the top of the file. So it can be shared and easily tweaked.

But also wait, isn't this parsing the rgb string and then just putting it back together and returning the same thing?

@@ +173,5 @@
>      }
> +    // Because of accuracy issues we use the authored values if this is a rgba
> +    // color.
> +    if (this.authored.startsWith("rgba(")) {
> +      let [, r, g, b, a] = /^\brgba\(([\d.]+),\s*([\d.]+),\s*([\d.]+),\s*([\d.]+|0|1)\)$/gi.exec(this.authored);

same here.

@@ +272,5 @@
>      if (computed === "transparent") {
>        return "transparent";
>      }
>  
> +    let rgba = /^rgba\(([\d.]+),\s*([\d.]+),\s*([\d.]+),\s*([\d.]+|1|0)\)$/gi.exec(computed);

same here

@@ +278,5 @@
>      if (rgba) {
>        let [, r, g, b, a] = rgba;
>        return {r: r, g: g, b: b, a: a};
>      } else {
> +      let rgb = /^rgb\(([\d.]+),\s*([\d.]+),\s*([\d.]+)\)$/gi.exec(computed);

and sorry, same here.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: reenable_browser_css_color_js_917389.patch

(In reply to Heather Arthur [:harth] from comment #2)
> Comment on attachment 806089 [details] [diff] [review]
> reenable_browser_css_color_js_917389.patch
> 
> Review of attachment 806089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One question, and one suggestion.
> 
> ::: browser/devtools/shared/css-color.js
> @@ +154,5 @@
> >      if (!this.hasAlpha) {
> > +      // Because of accuracy issues we use the authored values if this is a rgb
> > +      // color.
> > +      if (this.authored.startsWith("rgb(")) {
> > +        let [, r, g, b] = /^\brgb\(([\d.]+),\s*([\d.]+),\s*([\d.]+)\)$/gi.exec(this.authored);
> 
> Put this regex at the top of the file. So it can be shared and easily
> tweaked.
> 

Done (with all regexes).

> But also wait, isn't this parsing the rgb string and then just putting it
> back together and returning the same thing?
> 

You are right, I wanted to ensure that we output the correct spacings etc. but if we know that we have a valid color and it starts with e.g. "rgb(" then we are safe to return the authored color.

Fixed in all color getters.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/19ca8d6ac38c