[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


Alexander Pavlov (apavlov) <apavlov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #125058|review+                     |review?
               Flag|                            |




--- Comment #39 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2012-02-09 03:55:02 PST ---
(From update of attachment 125058)
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.

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