[webkit-reviews] review denied: [Bug 117919] Web Inspector: Display color picker in popover on color swatch click : [Attachment 205458] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied Matt Holden
<jftholden at yahoo.com>'s request for review:
Bug 117919: Web Inspector: Display color picker in popover on color swatch
click
https://bugs.webkit.org/show_bug.cgi?id=117919

Attachment 205458: Patch
https://bugs.webkit.org/attachment.cgi?id=205458&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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.