replace inIDOMUtils.rgbToColorName

RESOLVED FIXED in Firefox 49

Status

enhancement
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 49
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

3 years ago
We'll need to replace inIDOMUtils.rgbToColorName for the devtools
de-chrome-ification project.
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.
Assignee

Comment 2

3 years ago
It probably makes sense to handle bug 1266843 at the same time.
Assignee

Comment 3

3 years ago
FWIW this is already done in devtools.html, it just needs a little work to pull it over.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Duplicate of this bug: 1266843
Assignee

Comment 5

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

Updated

3 years ago
Depends on: 1267378
Attachment #8745578 - Flags: review?(pbrosset) → review+
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 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?
Attachment #8745578 - Flags: review+
Attachment #8745579 - Flags: review?(pbrosset)
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.
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
Assignee

Comment 11

3 years ago
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/
Attachment #8745579 - Flags: review?(pbrosset)
Assignee

Updated

3 years ago
Attachment #8745578 - Attachment is obsolete: true
Assignee

Comment 12

3 years ago
(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.
Assignee

Comment 13

3 years ago
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 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!
Attachment #8745579 - Flags: review?(pbrosset) → review+
Assignee

Updated

3 years ago
Attachment #8745579 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8746578 - Flags: review+
Assignee

Comment 18

3 years ago
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
Assignee

Updated

3 years ago
Attachment #8746579 - Flags: review+
Assignee

Comment 19

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

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

Comment 20

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

Comment 21

3 years ago
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/
Attachment #8746579 - Attachment description: MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro → MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r?pbro
Assignee

Comment 23

3 years ago
Still some problems in try.
I don't know how to cancel a review in reviewboard, or I would.
(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.
Assignee

Comment 25

3 years ago
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 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
Attachment #8746578 - Flags: review?(pbrosset)
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
Attachment #8746579 - Flags: review?(pbrosset)
Assignee

Comment 29

3 years ago
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/
Attachment #8746579 - Attachment description: MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r?pbro → MozReview Request: Bug 1266842 - replace rgbToColorName, colorToRGBA, isValidCSSColor in devtools; r=pbro
Attachment #8746579 - Flags: review?(pbrosset)
Assignee

Updated

3 years ago
Keywords: checkin-needed
failed to apply, needs rebasing
Flags: needinfo?(ttromey)
Keywords: checkin-needed
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).
Attachment #8746579 - Flags: review?(pbrosset)
Assignee

Comment 34

3 years ago
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/
Attachment #8746578 - Flags: review?(pbrosset)
Attachment #8746579 - Flags: review?(pbrosset)
Assignee

Comment 35

3 years ago
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/
Assignee

Comment 36

3 years ago
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.
Flags: needinfo?(ttromey)
Attachment #8746578 - Flags: review?(pbrosset) → review+
Assignee

Updated

3 years ago
Attachment #8746579 - Flags: review?(pbrosset) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 38

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dcd6211724fd
https://hg.mozilla.org/mozilla-central/rev/f7d385fac19a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee

Updated

3 years ago
Blocks: 1270139

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.