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

Fri Nov 25 07:27:33 PST 2011

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

--- Comment #19 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2011-11-25 07:27:32 PST ---
(From update of attachment 116391)
View in context: https://bugs.webkit.org/attachment.cgi?id=116391&action=review

The change looks better with the previous comments addressed. A couple of general notes:
- try to make Spectrum a reusable component not directly tied to the Styles sidebar
- use "===" rather than "==" wherever possible
- don't use blank lines to separate opening and closing braces of method bodies
- clean up duplicate blank lines (you use them them very rarely in the code)
- if in doubt, use the same style as the surrounding code does, even if it's against the rules :)

> Source/WebCore/ChangeLog:7
> +        updated commit for colorpicker functionality based on feedback from https://bugs.webkit.org/show_bug.cgi?id=71262.  Changes include: using only one colorpicker for the whole styles panel, UI improvements, working on element.style, native WebKit.Color within plugin, use webkit style for code, update project references, use popover instead of custom measurements

The message usually starts with a capital letter and has line breaks at reasonable intervals (usually around 100-120 characters).

> Source/WebCore/ChangeLog:9
> +	Reviewed by NOBODY (OOPS!).

This should be aligned with the rest of text. Webkit uses only spaces not tabs.

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

Inadvertent change

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

ditto

> Source/WebCore/inspector/front-end/Spectrum.js:22
> +    alphaLabel.textContent = "alpha: ";

This should be i18n-ized using WebInspector.UIString() / localizedStrings.js

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

extra blank line

> Source/WebCore/inspector/front-end/Spectrum.js:31
> +    var swatchClone = WebInspector.Spectrum.getSwatchElement();

In general, I'd be wary of this ad-hoc solution, as we'd like to make Spectrum a reusable component. What if it should be invoked on a button click and modify the contents of an edit box on change? Spectrum might inherit Object and dispatch events when changed and closed, and clients would add/remove listeners for these events.

> Source/WebCore/inspector/front-end/Spectrum.js:46
> +    //container.appendChild(this._containerElement);

stray commented line

> Source/WebCore/inspector/front-end/Spectrum.js:53
> +        this._onchange();  

can _onchange() incorporate a this._updateUI() call?

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

This should be "hsvToRGB" according to the webkit naming guidelines

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

Ditto

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

Prefer "===" over "==" everywhere

> Source/WebCore/inspector/front-end/Spectrum.js:153
> +    onmove = onmove || function() { };

Pavel feels strongly against nested anonymous functions, you can define a named one above.

> Source/WebCore/inspector/front-end/Spectrum.js:158
> +    var dragging = false;

You don't need to initialize these 4 locals here (dragging will be undefined which is equivalent of false in the inspector code most of the time).

> Source/WebCore/inspector/front-end/Spectrum.js:171
> +        e.returnValue = false;

We don't normally rely on non-standard properties, this must be a leftover from IE-compatible code. You also don't need the method presence checks above.

> Source/WebCore/inspector/front-end/Spectrum.js:177
> +            var dragX = Math.max(0, Math.min(e.pageX - offset.left + scrollOffset.left, maxWidth));

Where does scrollOffset come from? It should be at least defined in the closure scope, or else it gets defined on the window object.

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

Webkit tends to use camelcased identifiers ("rightClick")

> Source/WebCore/inspector/front-end/Spectrum.js:188
> +            if (onstart.apply(element, arguments) !== false) {

Try to avoid explicit boolean comparisons (see the WebKit coding guidelines)

> Source/WebCore/inspector/front-end/Spectrum.js:226
> +        if (rgba.length < 4) {

One-line blocks do not have braces.

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

We use "===" wherever possible.

> Source/WebCore/inspector/front-end/Spectrum.js:243
> +    get colorNoSatVal()

Webkit mostly avoids abbreviations ("colorWithFullSaturation"?)

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

extra blank line?

> Source/WebCore/inspector/front-end/Spectrum.js:257
> +    _isShown: false,

No need to explicitly define this. Historically, we either compute such properties in getters or create them when they turn "true" and delete (using the "delete" operator) when they turn "false".

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

extra blank line?

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

Web Inspector does not separate opening and closing braces for a function with blank lines.

> Source/WebCore/inspector/front-end/Spectrum.js:285
> +        // Where to show the bar that displays your current selected hue

Period at the end.

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

extra blank line?

> Source/WebCore/inspector/front-end/Spectrum.js:298
> +        if (rgb.length < 4)

can it be, for example, 2? If applicable, check for "=== 3".

> Source/WebCore/inspector/front-end/Spectrum.js:301
> +        var rgbNoSatVal = this.colorNoSatVal.rgb;

Please avoid abbreviations if possible

> Source/WebCore/inspector/front-end/Spectrum.js:318
> +		return this._isShown;

odd indent

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

extra blank line?

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

extra blank line?

> Source/WebCore/inspector/front-end/Spectrum.js:355
> +        // existing event listeners

Period at the end.

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

extra blank lines

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1672
> +                    // Alt + click toggles color formats

Comments should be full sentences followed by periods.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1673
> +                    // Click opens colorpicker, only if the element is not in computed styles section)

A closing parenthesis instead of a period.

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

"else if" should be found on this line

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

ditto

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

inadvertent change?

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

ditto

> Source/WebCore/inspector/front-end/inspector.css:2669
> +    position:absolute; 

The CSS property format everywhere is:
property: value;

> Source/WebCore/inspector/front-end/inspector.css:2680
> +.spectrum-top {

separate all rules with blank lines

> Source/WebCore/inspector/front-end/inspector.css:2681
> +  position:relative; 

odd indentation

> Source/WebCore/inspector/front-end/inspector.css:2686
> +   position:absolute; top:0; left:0; bottom:0; right:0;

odd indentation.

Also, please don't use more than one property per line.

> Source/WebCore/inspector/front-end/inspector.css:2690
> +    top:0;left:0;bottom:0;right:20%;

One property per line

> Source/WebCore/inspector/front-end/inspector.css:2694
> +    top:0;right:0;bottom:0;left:83%;

ditto

> Source/WebCore/inspector/front-end/inspector.css:2697
> +    margin-top: 80%;  /* Same as spectrum-color width */

you can have a common class for the elements that should have margin-top: 80%

> Source/WebCore/inspector/front-end/utilities.js:286
> +    var totalLeft = totalTop = 0;

One declaration per line.

> Source/WebCore/inspector/front-end/utilities.js:296
> +    var curleft = curtop = 0;

Webkit uses one declaration per line, and camelcased variable identifiers (also, |left| and |top| would do just fine here).

> Source/WebCore/inspector/front-end/utilities.js:298
> +    if (el.offsetParent) {

Is this really consistent? I.e., the loop does not run if |this| has no offsetParent but will run for some of |this|'s offsetParent-ancestors having no offsetParent.
Or do you mean that BODY has no offsetParent but can have scrollLeft/Top? 
If so, please add a comment to clarify this, otherwise the following snippet could be reused:

while (el.offsetParent) {
    curLeft += el.scrollLeft;
    curTop += el.scrollTop;
    el = el.offsetParent;
}

> Source/WebCore/inspector/front-end/utilities.js:325
> + 

Odd change

> Source/WebCore/inspector/front-end/utilities.js:907
> +

extra blank line

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