[webkit-reviews] review granted: [Bug 124354] Web Inspector: New color picker : [Attachment 216942] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 124354: Web Inspector: New color picker
https://bugs.webkit.org/show_bug.cgi?id=124354

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

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=216942&action=review


> Source/WebInspectorUI/UserInterface/Color.js:843
> +    var min = Math.min(Math.min(r, g), b),
> +	   max = Math.max(Math.max(r, g), b),
> +	   delta = max - min;

We prefer multiple var declarations and not stringing them.

> Source/WebInspectorUI/UserInterface/Color.js:847
> +    var v = max,
> +	   s,
> +	   h;

Ditto.

> Source/WebInspectorUI/UserInterface/Color.js:849
> +    if (max == min)

===

> Source/WebInspectorUI/UserInterface/Color.js:851
> +    else if (max == r)

===

> Source/WebInspectorUI/UserInterface/Color.js:853
> +    else if (max == g)

===

> Source/WebInspectorUI/UserInterface/Color.js:855
> +    else if (max == b)

===

> Source/WebInspectorUI/UserInterface/Color.js:862
> +    if (max == 0)

===

> Source/WebInspectorUI/UserInterface/Color.js:873
> +	   return [v,v,v];

Spaces after the commas.

> Source/WebInspectorUI/UserInterface/ColorPicker.js:49
> +    this._opacityPattern = 'url("data:image/svg+xml;base64,' + btoa('<svg
xmlns="http://www.w3.org/2000/svg" width="6" height="6" fill="rgb(204, 204,
204)"><rect width="3" height="3" /><rect x="3" y="3" width="3"
height="3"/></svg>') + '")';

Neat! It would be nice it this was declared in CSS or at least in an SVG file.
Then we could use it for the swatch background too.

> Source/WebInspectorUI/UserInterface/ColorPicker.js:101
> +	   this._opacitySlider.value = color.alpha || 1;

This will cause a color that has alpha === 0 to turn into alpha === 1. Is that
intended?

> Source/WebInspectorUI/UserInterface/ColorPicker.js:139
> +	   this._opacitySlider.element.style.backgroundImage =
"linear-gradient(90deg, " + transparent + ", " + tintedColor + "), " +
this._opacityPattern;
> +
> +	   this._brightnessSlider.element.style.backgroundImage =
"linear-gradient(90deg, black, " + rawColor + ")";

Grouping these two lines would read better, with the blank line before both.

> Source/WebInspectorUI/UserInterface/ColorWheel.js:58
> +	   this._radius = dimension / 2 - 2;

A comment on why it is 2px smaller would be good.

> Source/WebInspectorUI/UserInterface/Slider.js:125
> +	   return window.webkitConvertPointFromPageToNode(this._element, new
WebKitPoint(event.pageX, event.pageY));

Add a comment why this is needed (since this class allows transforms).

> Source/WebInspectorUI/UserInterface/Slider.js:131
> +	       this.__maxX = this._element.offsetWidth -
WebInspector.Slider.KnobWidth / 2 - 1;

Comment on why it is 1 smaller.

> Source/WebInspectorUI/WebInspectorUI.vcxproj/WebInspectorUI.vcxproj.filters:1

> -<?xml version="1.0" encoding="utf-8"?>
> +<?xml version="1.0" encoding="utf-8"?>

Might want to revert the BOM change.