Bug 125244 – Web Inspector: edited color should serialize back to original format when possible

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Antoine Quint]

When we moved to the new color picker (see https://bugs.webkit.org/show_bug.cgi?id=124354) we lost the ability to have colors serialize back to their original format when possible. We should bring this back.

[Antoine Quint]

Created attachment 218431 [details]
Patch

[Antoine Quint]

Created attachment 218433 [details]
Screen recording showing color editing in a CSS resource in action

[Antoine Quint]

Created attachment 218441 [details]
New Color.js for easier review

[Joseph Pecoraro]

Comment on attachment 218431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218431&action=review

r=me, nice improvements!

> Source/WebInspectorUI/ChangeLog:9
> +        seriliazing the color to the various supported formats.

Typo: seriliazing => serializing

> Source/WebInspectorUI/ChangeLog:33
> +        Keep track of the original color format so that we can use it as the prefered format

Typo: prefered => preferred

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:431
> +                    var color = WebInspector.Color.fromString(match[0]);
> +                    if (!color) {

Awesome!

> Source/WebInspectorUI/UserInterface/Color.js:54
> +    if (transparentNicknames.indexOf(value) !== -1) {

Array.prototype.contains? In Utilities.js

> Source/WebInspectorUI/UserInterface/Color.js:76
> +                    parseInt(hex.substring(0,1), 16) * 255,

Bug! substring(0, 2) not 0,1.

> Source/WebInspectorUI/UserInterface/Color.js:78
> +                    parseInt(hex.substring(2,4), 16) * 255,
> +                    parseInt(hex.substring(4,6), 16) * 255,

Style: All of these should have spaces in the substring arguments. "(0, 2)" not "(0,2)".

> Source/WebInspectorUI/UserInterface/Color.js:89
> +        var rgb = match[2].split(/\s*,\s*/);
> +            return new WebInspector.Color(WebInspector.Color.Format.RGB, [
> +                parseInt(rgb[0]),
> +                parseInt(rgb[1]),
> +                parseInt(rgb[2]),
> +                1
> +            ]);

Style: missing indentation.

> Source/WebInspectorUI/UserInterface/Color.js:104
> +                parseInt(hsl[0]),
> +                parseInt(hsl[1]),
> +                parseInt(hsl[2]),

I wonder if we should use parseFloat here?

Inside WebCore we start with doubles and eventually truncate to ints:

    1. WebCore::CSSParser::parseHSLParameters (CSSParser.cpp)
      -> colorArray is a bunch of doubles
      -> colorArray[0] = (((static_cast<int>(parsedDouble(v, ReleaseParsedCalcValue)) % 360) + 360) % 360) / 360.0;
      -> colorArray[i] = std::max<double>(0, std::min<double>(100, parsedDouble(v, ReleaseParsedCalcValue))) / 100.0; // needs to be value between 0 and 1.0
      -> caller of parseHSLParameters then does c = makeRGBAFromHSLA(colorValues[0], colorValues[1], colorValues[2], 1.0);

    2. WebCore::makeRGBAFromHSLA (Color.cpp)
        RGBA32 makeRGBAFromHSLA(double hue, double saturation, double lightness, double alpha)
        {
            ...
            return makeRGBA(static_cast<int>(calcHue(temp1, temp2, hue + 1.0 / 3.0) * scaleFactor), 
                            static_cast<int>(calcHue(temp1, temp2, hue) * scaleFactor),
                            static_cast<int>(calcHue(temp1, temp2, hue - 1.0 / 3.0) * scaleFactor),
                            static_cast<int>(alpha * scaleFactor));
        }

I think because they get truncated to ints we are fine just doing parseInt.

The spec doesn't say what the behavior should be for floats (round / floor / parse error).

> Source/WebInspectorUI/UserInterface/Color.js:188
> +        var rgb = this.rgba.concat();

Per IRC we normally use .slice() to dup an array.

> Source/WebInspectorUI/UserInterface/Color.js:221
> +        switch (format) {
> +            case WebInspector.Color.Format.Original:
> +                return this._toOriginalString();

You can fix the indentation while we are here.

> Source/WebInspectorUI/UserInterface/Color.js:348
>      /**

Right beneath here is "toProtocolRGBA". We don't actually use this anywhere, maybe you can remove it. At the least, it essentially does an "if (this.rgba)" which will always be true now. So that can be updated.

> Source/WebInspectorUI/UserInterface/Color.js:369
> -    /**
>       * @param {number|string} rgbValue
>       * @return {number}
>       */

Style: You can drop these comments all over the file, unless you find them useful.

[Antoine Quint]

Created attachment 218453 [details]
Patch for landing

[Antoine Quint]

http://trac.webkit.org/changeset/160132