[Webkit-unassigned] [Bug 71262] Web Inspector: Add colorpicker functionality to color swatches in Styles Sidebar

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=71262





--- Comment #34 from Pavel Feldman <pfeldman at chromium.org>  2012-01-04 03:46:53 PST ---
(From update of attachment 120904)
View in context: https://bugs.webkit.org/attachment.cgi?id=120904&action=review

This looks mostly Ok. Let me apply it to see if it behaves consistently with the rest of the app.

> Source/WebCore/inspector/front-end/Spectrum.js:31
> +    this._popover = new WebInspector.Popover(null, true);

First parameter is marked as optional, not nullable, so you should pass "undefined" instead.

> Source/WebCore/inspector/front-end/Spectrum.js:99
> +WebInspector.Spectrum.hsvToRGB = function(h, s, v, a) 

WebInspector.Spectrum.hsvaToRGBA ?

> Source/WebCore/inspector/front-end/Spectrum.js:133
> +WebInspector.Spectrum.rgbToHSV = function(r, g, b, a) 

ditto

> Source/WebCore/inspector/front-end/Spectrum.js:165
> +WebInspector.Spectrum.draggable = function(element, onmove, onstart, onstop) {

I still think it is worth extra effort to make it work with WebInspector.elementDragStart. I am fine with landing this method for now since you change is up for review for so long. Please add //FIXME: migrate to WebInspector.elementDragStart above this line.

> Source/WebCore/inspector/front-end/Spectrum.js:254
> +            return WebInspector.Color.fromRGB(round[0], round[1], round[2], round[3]);

WebInspector.Color.fromRGB receives only 3 arguments.

> Source/WebCore/inspector/front-end/Spectrum.js:261
> +        var rgb = WebInspector.Spectrum.hsvToRGB(this.hsv[0], 1, 1);

WebInspector.Spectrum.hsvToRGB expects 4 parameters.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1609
> +                var spectrum = hasColorpicker && self._parentPane._spectrum;

Prefer hasColorpicker ? self._parentPane._spectrum : null

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.