[webkit-reviews] review requested: [Bug 71262] Web Inspector: Add colorpicker functionality to color swatches in Styles Sidebar : [Attachment 125058] Add colorpicker functionality to color swatches in Styles Sidebar - Using native popover and keeping original color format

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has asked  for review:
Bug 71262: Web Inspector: Add colorpicker functionality to color swatches in
Styles Sidebar
https://bugs.webkit.org/show_bug.cgi?id=71262

Attachment 125058: Add colorpicker functionality to color swatches in Styles
Sidebar - Using native popover and keeping original color format
https://bugs.webkit.org/attachment.cgi?id=125058&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125058&action=review


Make sure all strings you pass into WebInspector.UIString(..) are present in
Source/WebCore/English.lproj/localizedStrings.js (it is in UTF-16).

> Source/WebCore/ChangeLog:1
> +2012-02-01  bgrins  <briangrinstead at gmail.com>

A real name will look nice here
(http://trac.webkit.org/wiki/UsingGitWithWebKit#WebKitScriptsupportforGit has a
good description of the git setup process for WebKit development)

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

hsvaToRGBA?

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

rgbaToHSVA?

> Source/WebCore/inspector/front-end/Spectrum.js:138
> +    var max = Math.max(r, g, b), min = Math.min(r, g, b);

One variable per line

> Source/WebCore/inspector/front-end/Spectrum.js:254
> +	   if (rgb[3] === 1) {

Please remove braces around single-line blocks

> Source/WebCore/inspector/front-end/Spectrum.js:257
> +	   else {

ditto

> Source/WebCore/inspector/front-end/Spectrum.js:291
> +	   var round = [Math.round(rgb[0]), Math.round(rgb[1]),
Math.round(rgb[2]), rgb[3]];

Rounding of the RGB values can be extracted into a separate method, since you
do exactly the same in the "get color()" above.

> Source/WebCore/inspector/front-end/Spectrum.js:306
> +    

a lonely extra blank line :)

> Source/WebCore/inspector/front-end/Spectrum.js:398
> +	   this.dispatchEventToListeners("hide", this.color);

Typically we define something along the lines of

WebInspector.Spectrum.Events = {
    ColorChanged: "ColorChanged",
    Hidden: "Hidden"
}

(using the participle instead of infinitive) - see CSSStyleModel.js for an
example.

Also, the Hidden event doesn't seem to need the color as the payload, since the
updated color has been dispatched earlier in ColorChanged (and it's a good idea
to dispatch the default initial color when the picker has just been opened.)

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1644
> +		       self.applyStyleText(nameElement.textContent + ": " +
valueElement.textContent, true, true, false);

The user seems to be unable to cancel colorpicking (e.g. using Esc)?

> Source/WebCore/inspector/front-end/inspector.css:2690
> +.spectrum-sat, .spectrum-val, .spectrum-top-inner { 

Instead of these separate classes, you can use the .fill class on respective
elements, defined at the top of inspector.css. This will at least slightly help
the size of our giant CSS.