replace inIDOMUtils.rgbToColorName

RESOLVED FIXED in Firefox 49

Status

P1
enhancement
RESOLVED FIXED
3 years ago
6 months ago

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
(Assignee)

Comment 6

3 years ago
Created attachment 8745578 [details]
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/
Attachment #8745578 - Flags: review?(pbrosset)
Attachment #8745579 - Flags: review?(pbrosset)
(Assignee)

Comment 7

3 years ago
Created attachment 8745579 [details]
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/
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)

Comment 15

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

Comment 16

3 years ago
Created attachment 8746579 [details]
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/
(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
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(Assignee)

Updated

3 years ago
Blocks: 1270139

Updated

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