[Webkit-unassigned] [Bug 117919] Web Inspector: Display color picker in popover on color swatch click

[bugzilla-daemon at webkit.org]

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


Timothy Hatcher <timothy at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #205458|review?                     |review-
               Flag|                            |




--- Comment #16 from Timothy Hatcher <timothy at apple.com>  2013-06-26 06:30:43 PST ---
(From update of attachment 205458)
View in context: https://bugs.webkit.org/attachment.cgi?id=205458&action=review

Looking good. Found some more minor things to fix.

>> Source/WebInspectorUI/ChangeLog:8
>> +		This patch implements suggests from the previous review.
> 
> Line contains tab character.  [whitespace/tab] [5]

Replace the tab with spaces.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:100
> +            if (e.button != 0 || e.ctrlKey)

!==

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:147
> +        this.hsv[0] = (dragY / this.slideHeight);

No need for the ().

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:156
> +        initialHelperOffset = { x: this._dragHelperElement.offsetLeft, y: this._dragHelperElement.offsetTop };

No need for the spaces inside {}, except after : and ,.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:193
> +    ColorChanged: "colorChanged"

The event text should be: "css-color-picker-color-changed"

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:208
> +        if (!this._originalFormat) {
> +            this._originalFormat = color.format;
> +        }

No need for the { }

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:231
> +        var color;
> +
> +        if (rgba[3] === 1)
> +            color = WebInspector.Color.fromRGB(rgba[0], rgba[1], rgba[2]);
> +        else
> +            color = WebInspector.Color.fromRGBA(rgba[0], rgba[1], rgba[2], rgba[3]);
> +
> +        var colorValue = color.toString(this.outputColorFormat);
> +        if (!colorValue)
> +            colorValue = color.toString(); // this.outputColorFormat can be invalid for current color (e.g. "nickname").
> +        return new WebInspector.Color(colorValue);

I think this can be simplified to:

var color = WebInspector.Color.fromRGBA(rgba[0], rgba[1], rgba[2], rgba[3]);
color.format = this.outputColorFormat;
if (!color.toString())
    color.format = color.nextFormat();
return color;

WebInspector.Color should be tweaked to allow this code to be less roundabout. Can be a follow up.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:243
> +            else if (format === WebInspector.Color.Format.HSVA)
> +                format = WebInspector.Color.Format.HSV;

These constants do not exist. Maybe the old Inspector had it?

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:248
> +            // Don't output ShortHex
> +            else if (format === WebInspector.Color.Format.ShortHEX)
> +                format = WebInspector.Color.Format.HEX;

This comment breaks the flow of the else ifs. It should go inside of a block, or at the end of a line instead, or merged with the "Simplify transparent formats." comment.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:250
> +            // Everything except HSL(A)&HSV(A) should be returned as RGBA if transparency is involved.

There isn't hsv() in CSS. THis comment should only mention HSLA.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:253
> +                WebInspector.Color.Format.HSVA,

This constant does not exist.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:257
> +            if (printableTransparentFormats.indexOf(format) == -1)

===

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:285
> +    _onchange: function()

Can we come up with a better name here? Maybe just _update? This should have a // Private comment before it separated by blank lines to match other files.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:288
> +        this.dispatchEventToListeners(WebInspector.CSSColorPicker.Event.ColorChanged, { color: this.color });

No needed for spaces inside {}, except after :.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:315
> +    _updateUI: function()

We prefer to not abbreviate. _updateInterface? _updateDisplay?

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:336
> +        var colorValue = this.color.toString(this.outputColorFormat);
> +        if (!colorValue)
> +            colorValue = color.toString(); // this.outputColorFormat can be invalid for current color (e.g. "nickname").

We should fix WebInspector.Color so this isn't needed. Maybe use nextFormat() internally if the current format is invalid. Can be a follow up.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:192
> +    // WebInspector.Popover delegate protocol method

Comment not needed.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:193
> +    didDismissPopover: function(popover) {

{ should be on new line. This should also be inside the Protected section below.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:194
> +        if (popover == this._colorPickerPopover)

===

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:681
> +            this._codeMirror.operation(function() {

{ should be on a new line.

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