[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 #22 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2011-11-28 01:02:09 PST ---
(From update of attachment 116686)
View in context: https://bugs.webkit.org/attachment.cgi?id=116686&action=review

Thanks for addressing the comments, this is getting much closer to the webkit-ish code! Since I'm not a reviewer, we'll need some of those (pfeldman at chromium.org and yurys at chromium.org are the closest ones) to run the final check some time soon.

> Source/WebCore/inspector/front-end/Color.js:46
> +WebInspector.Color.fromRGB = function(r, g, b)

blank line above

> Source/WebCore/inspector/front-end/Popover.js:52
> +    if (hideOverflow) {

No braces necessary for one-line if-blocks

> Source/WebCore/inspector/front-end/Popover.js:-163
> -

Inadvertent change

> Source/WebCore/inspector/front-end/Spectrum.js:29
> +WebInspector.Spectrum = function(container)

Annotating your new object for the Closure compiler is a good thing to do once you have fixed other issues (see other files in the "front-end" directory and http://code.google.com/closure/compiler/docs/js-for-compiler.html)

> Source/WebCore/inspector/front-end/Spectrum.js:59
> +    swatchElement.createChild("span", "swatch-inner");

Please combine the two lines into one (querySelectorAll is generally expensive and is unnecessary here):

this._swatchInnerElement = swatchElement.createChild("span", "swatch-inner");

> Source/WebCore/inspector/front-end/Spectrum.js:142
> +    s = max === 0 ? 0 : d / max;

Is the case of "-0" possible here? Also, a better version is "s = max ? d / max : 0" since comparisons to NULL, false, and 0 are disapproved and allowed only to resolve ambiguities (http://www.webkit.org/coding/coding-style.html, "Null, false and 0").

> Source/WebCore/inspector/front-end/Spectrum.js:190
> +                onmove.apply(element, [dragX, dragY]); 

It is not a good idea to pass an arbitrary "this" to a client callback, since it may be a bound function having its own "this". Sorry for overlooking this. A better option is just "onmove(element, [dragX, dragY]);".

> Source/WebCore/inspector/front-end/Spectrum.js:196
> +        var rightClick = (e.which) ? (e.which === 3) : (e.button === 2);

The parentheses around e.which are not required.

> Source/WebCore/inspector/front-end/Spectrum.js:201
> +                onstart.apply(element, arguments)

A better option is just "onstart(element, e);".

> Source/WebCore/inspector/front-end/Spectrum.js:219
> +    function stop() 

You should declare the Event ("e") argument...

> Source/WebCore/inspector/front-end/Spectrum.js:228
> +                onstop.apply(element, arguments); 

...and use it as "onstop(element, e);".

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

Should hsvToRGB handle the rounding as well? I can't think of an RGB color using floats as its components.

> Source/WebCore/inspector/front-end/Spectrum.js:254
> +            return WebInspector.Color.fromRGB.apply(this, round);

Why do you need "apply" if this is a "static" method? Just "return WebInspector.Color.fromRGB(round);" would do here.

> Source/WebCore/inspector/front-end/Spectrum.js:256
> +        return WebInspector.Color.fromRGBA.apply(this, round);

ditto

> Source/WebCore/inspector/front-end/Spectrum.js:264
> +        return WebInspector.Color.fromRGBA.apply(this, round);

ditto

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

extra blank line

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

extra blank line

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

ditto

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:-1584
> -

Inadvertent change

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1650
> +                swatchInnerElement.style.setProperty("background-color", text);

Web Inspector typically uses ...style.backgroundColor = text;

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1660
> +                    swatchInnerElement.style.setProperty("background-color", colorString);

ditto

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1671
> +                

extra blank line

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1677
> +                    if (e.altKey) {

No braces necessary for if (e.altKey). We also tend to check for other modifiers not being active.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1760
> +                

Inadvertent change

> Source/WebCore/inspector/front-end/inspector.css:2708
> +    top:0; 

spaces after colons

> Source/WebCore/inspector/front-end/inspector.css:2753
> +   border: solid black 3px;

a typical format in the inspector CSS is "3px solid #000" (historically, there were only rgb's, so it would have been ...rgb (0, 0, 0))

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