[webkit-reviews] review granted: [Bug 125244] Web Inspector: edited color should serialize back to original format when possible : [Attachment 218431] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 125244: Web Inspector: edited color should serialize back to original
format when possible
https://bugs.webkit.org/show_bug.cgi?id=125244

Attachment 218431: Patch
https://bugs.webkit.org/attachment.cgi?id=218431&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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.