1266842 - replace inIDOMUtils.rgbToColorName

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

We'll need to replace inIDOMUtils.rgbToColorName for the devtools
de-chrome-ification project.

Comment 1

[Patrick Brosset <:pbro>]

This is used in 2 places on the client-side:
devtools\client\eyedropper\eyedropper.js
devtools\shared\css-color.js

Named colors don't change very often. In gecko they are listed here: 
https://dxr.mozilla.org/mozilla-central/source/gfx/src/nsColorNameList.h
And the last real update to this file was 2 years ago to add rebeccapurple to the list.

So I think it would be ok to have a hard-coded list of colors in some JS module in the devtools UI code too.

Comment 2

[Tom Tromey :tromey]

It probably makes sense to handle bug 1266843 at the same time.

Comment 3

[Tom Tromey :tromey]

FWIW this is already done in devtools.html, it just needs a little work to pull it over.

Comment 5

[Tom Tromey :tromey]

Maybe css-color should be moved out of devtools/shared and into devtools/client/shared.
Nothing server-side seems to use it.

Comment 6

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1266842 - move css-color.js to devtools/client/shared; r?pbro

Review commit: https://reviewboard.mozilla.org/r/49049/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49049/

Comment 7

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r?pbro

Review commit: https://reviewboard.mozilla.org/r/49051/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49051/

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8745578 [details]
MozReview Request: Bug 1266842 - move css-color.js to devtools/client/shared; r?pbro

https://reviewboard.mozilla.org/r/49049/#review46009

Looks good. However, mozreview shows me the 2 moved files as deletions and additions. Did you move them with mercurial? If so, that's fine, it might only be mozreview not being able to show this information.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8745578 [details]
MozReview Request: Bug 1266842 - move css-color.js to devtools/client/shared; r?pbro

https://reviewboard.mozilla.org/r/49049/#review46011

Actually, after checking https://reviewboard-hg.mozilla.org/gecko/rev/95b20853a4e82cfbbe585b7c0fc1868501549188 I believe the files were deleted and created, which will break history. Could you use `hg mv` instead?

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8745579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r?pbro

https://reviewboard.mozilla.org/r/49051/#review46013

::: .eslintignore:105
(Diff revision 1)
>  devtools/client/responsivedesign/**
>  devtools/client/scratchpad/**
>  devtools/client/shadereditor/**
>  devtools/client/shared/**
>  !devtools/client/shared/css-color.js
> +!devtools/client/shared/css-color-db.js

Oh nice, I didn't know about ! in .eslintignore.

::: devtools/client/shared/css-color-db.js:26
(Diff revision 1)
> + * @param {Number} r, g, b  The color components.
> + * @return {String} the name of the color
> + */
> +function rgbToColorName(r, g, b) {
> +  if (!cssRGBMap) {
> +    cssRGBMap = new Map();

Why not just an object, all keys are strings anyway.

::: devtools/client/shared/css-color-db.js:62
(Diff revision 1)
> +  }
> +  return val;
> +}
> +
> +// Originally from dom/tests/mochitest/ajax/mochikit/MochiKit/Color.js.
> +function hslToRGB([hue, saturation, lightness]) {

We have some similar color helpers in css-color.js (like `rgbToHsl`). It would make sense to have these all in one place.

::: devtools/client/shared/css-color-db.js:91
(Diff revision 1)
> +
> +/**
> + * Convert a string representing a color to an array holding the
> + * color's components.  Any valid CSS color form can be passed in.
> + *
> + * @param {String} name the color

Missing @return comment

::: devtools/client/shared/css-color-db.js:93
(Diff revision 1)
> + * Convert a string representing a color to an array holding the
> + * color's components.  Any valid CSS color form can be passed in.
> + *
> + * @param {String} name the color
> + */
> +function colorToRGBA(name) {

My main concern with this new file, and especially this function here is that it may be hard for people to understand why we have css-color.js and css-color-db.js that seem to have functions that do similar things.
In practice, css-color.js contains the class `CssColor` which is meant to be a much higher level API to do many things with a color object, but it might not be obvious to people.
In fact, when I saw the name css-color-db, I was expecting to only see a map of color named -> rgb.
Turns out there are more color utils things, but we also have others in css-color.js. So maybe this file should really only contain the map, and then everything else should go in css-color.js ?

::: devtools/client/shared/css-color-db.js:98
(Diff revision 1)
> +    result = cssColors[name];
> +  } else if (name === "transparent") {
> +    result = [0, 0, 0, 0];
> +  } else if (name === "currentcolor") {
> +    result = [0, 0, 0, 1];

It would be easier to read if each of these condition bodies returned, so that the last case (the really long one, which uses the lexer) could be separated out a bit more, and un-indented:

```
// Early return for simple cases like named colors and keywords.
if (name in cssColors) {
  return cssColors[name]
} else if (name === "transparent") {
  return [0, 0, 0, 0];
} else if (name === "currentcolor") {
  return [0, 0, 0, 1];
}

// In all other cases, we need a more complex parsing
// of the color string.
...
```

::: devtools/client/shared/css-color-db.js:106
(Diff revision 1)
> +  } else if (name === "currentcolor") {
> +    result = [0, 0, 0, 1];
> +  } else {
> +    let lexer = DOMUtils.getCSSLexer(name);
> +
> +    let getToken = function() {

Arrow functions are nicer for these small, inline, helpers:

```
let getToken = () => {
  ...
};
```

Same below with requireComma:

```
let requireComma = token => {
  ...
};
```

::: devtools/client/shared/css-color-db.js:129
(Diff revision 1)
> +    if (!func || func.tokenType !== "function") {
> +      return null;
> +    }
> +    let alpha = false;
> +    if (func.text === "rgb" || func.text === "hsl") {
> +      // Nothing.

Instead of the `// nothing` comment, maybe do set the alpha to false, so the intent is clearer.

::: devtools/client/shared/css-color-db.js:133
(Diff revision 1)
> +    if (func.text === "rgb" || func.text === "hsl") {
> +      // Nothing.
> +    } else if (func.text === "rgba" || func.text === "hsla") {
> +      alpha = true;
> +    } else {
> +      return null;

I think the case where the function isn't expected would be better dealt in the previous if block above.
So this whole first part could be rewritten as:

```
let func = getToken();
const expectedFunctions = ["rgba", "rgba", "hsla", "hsl"];
if (!func || func.tokenType !== "function ||
    !expectedFunctions.includes(func.text)) {
  return null;
}

let alpha = func.text === "rgba" || func.text === "hsla";
```

::: devtools/client/shared/css-color-db.js:146
(Diff revision 1)
> +      if (num < 0) {
> +        num = 0;
> +      } else if (num > 255) {
> +        num = 255;
> +      }

Maybe move these 5 lines (and the 5 lines further below where you cap the alpha between 0 and 1) to a small helper arrow function you can use simply here like `let num = cap(token.number, 0, 255);`

::: devtools/client/shared/css-color-db.js:199
(Diff revision 1)
> + */
> +function isValidCSSColor(name) {
> +  return colorToRGBA(name) !== null;
> +}
> +
> +// Auto-generated from nsColorNameList.h.

I think this should be a bigger/more prominent comment, something like:

// /!\ Auto-generated from nsColorNameList.h
// Should be kept in sync with that file.

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8745579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r?pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49051/diff/1-2/

Comment 12

[Tom Tromey :tromey]

(In reply to Patrick Brosset <:pbro> from comment #9)
> Comment on attachment 8745578 [details]
> MozReview Request: Bug 1266842 - move css-color.js to
> devtools/client/shared; r?pbro
> 
> https://reviewboard.mozilla.org/r/49049/#review46011
> 
> Actually, after checking
> https://reviewboard-hg.mozilla.org/gecko/rev/
> 95b20853a4e82cfbbe585b7c0fc1868501549188 I believe the files were deleted
> and created, which will break history. Could you use `hg mv` instead?

I'll redo this once the eslint fix has landed, so I can more readily use hg.

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8745579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r?pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49051/diff/2-3/

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8745579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r?pbro

https://reviewboard.mozilla.org/r/49051/#review46265

Awesome, thanks!

Comment 15

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1266842 - move css-color.js to devtools/client/shared; r=pbro

Review commit: https://reviewboard.mozilla.org/r/49473/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49473/

Comment 16

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro

Review commit: https://reviewboard.mozilla.org/r/49475/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49475/

Comment 17

[Tom Tromey :tromey]

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

Comment 18

[Tom Tromey :tromey]

Comment on attachment 8746578 [details]
MozReview Request: Bug 1266842 - move css-color.js to devtools/client/shared; r=pbro

https://reviewboard.mozilla.org/r/49473/#review46305

Comment 19

[Tom Tromey :tromey]

Comment on attachment 8746579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro

https://reviewboard.mozilla.org/r/49475/#review46307

Comment 20

[Tom Tromey :tromey]

I missed a pretty baffling number of cases in the color parser.
This adds more parsing and more tests.

Comment 21

[Tom Tromey :tromey]

Comment on attachment 8746579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49475/diff/1-2/

Comment 22

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=366c32779832

Comment 23

[Tom Tromey :tromey]

Still some problems in try.
I don't know how to cancel a review in reviewboard, or I would.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Tom Tromey :tromey from comment #23)
> Still some problems in try.
> I don't know how to cancel a review in reviewboard, or I would.

You can click "Details" from the attachment in bugzilla and clear it there.  Not sure if that's the 'right' way to do it, but I've done it before.

Comment 25

[Tom Tromey :tromey]

Comment on attachment 8746579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49475/diff/2-3/

Comment 26

[Tom Tromey :tromey]

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

Comment 27

[Patrick Brosset <:pbro>]

Comment on attachment 8746578 [details]
MozReview Request: Bug 1266842 - move css-color.js to devtools/client/shared; r=pbro

Clearing review flag since this has been R+'d

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8746579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro

Clearing review flag since this has been R+'d

Comment 29

[Tom Tromey :tromey]

Comment on attachment 8746579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49475/diff/3-4/

Comment 30

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ddb7689383a

Comment 31

[Tom Tromey :tromey]

https://reviewboard.mozilla.org/r/49475/#review46493

Carrying over r+

Comment 32

[Carsten Book [:Tomcat]]

failed to apply, needs rebasing

Comment 33

[Patrick Brosset <:pbro>]

Comment on attachment 8746579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro

Clearing the R? flag since I think it was added by mistake (when uploading to mozreview).

Comment 34

[Tom Tromey :tromey]

Comment on attachment 8746578 [details]
MozReview Request: Bug 1266842 - move css-color.js to devtools/client/shared; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49473/diff/1-2/

Comment 35

[Tom Tromey :tromey]

Comment on attachment 8746579 [details]
MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49475/diff/4-5/

Comment 36

[Tom Tromey :tromey]

Comment on attachment 8746578 [details]
MozReview Request: Bug 1266842 - move css-color.js to devtools/client/shared; r=pbro

Rebased; carrying over r+

I would have thought that pushing to review with "r=pbro" would do this, but it didn't.

Comment 37

[Pulsebot]

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

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dcd6211724fd
https://hg.mozilla.org/mozilla-central/rev/f7d385fac19a