[webkit-reviews] review canceled: [Bug 71262] Web Inspector: Add colorpicker functionality to color swatches in Styles Sidebar : [Attachment 119233] Patch to provide colorpicker functionality inside of the styles sidebar for the web inspector frontend

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has canceled Brian Grinstead
<briangrinstead at gmail.com>'s request for review:
Bug 71262: Web Inspector: Add colorpicker functionality to color swatches in
Styles Sidebar
https://bugs.webkit.org/show_bug.cgi?id=71262

Attachment 119233: Patch to provide colorpicker functionality inside of the
styles sidebar for the web inspector frontend
https://bugs.webkit.org/attachment.cgi?id=119233&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119233&action=review


Thanks for working on this. It looks like we are almost ready to get it landed.
I'd suggest that we do it in steps:
1) land utility changes
2) land the Spectrum component
3) start using it behind the flag

Steps (1) and (3) need separate bugs filed.

> Source/WebCore/inspector/front-end/Popover.js:35
> +WebInspector.Popover = function(popoverHelper, appendElement, hideOverflow)

appendElement does not looks like a good name to me and I don't think it
belongs to the popover constructor. I'd call it parentElement and would pass it
into show method. More importantly, you should land this as a separate change
since it seems universal.

> Source/WebCore/inspector/front-end/Spectrum.js:95
> +    this.hideProxy = this.hide.bind(this);

This seems to be private to this js file -> rename to this._hideProxy.

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

{ should be on the new line

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

ditto

> Source/WebCore/inspector/front-end/Spectrum.js:146
> +    else {

should be
} else {

> Source/WebCore/inspector/front-end/Spectrum.js:163
> +WebInspector.Spectrum.draggable = function(element, onmove, onstart, onstop)
{

I think I was suggesting to use WebInspector.elementDragStart earlier. I think
I missed your comment on that, did it prove not to be applicable to your case?

> Source/WebCore/inspector/front-end/Spectrum.js:276
> +    _updateHelperLocations: function() {

{ on the new line

> Source/WebCore/inspector/front-end/Spectrum.js:288
> +	   );

We don't fomat closing ) like this, I'd do:
dragX = Math.max(-this._dragHelperElementHeight, 
		 Math.min(this.dragWidth - this._dragHelperElementHeight, dragX
- this._dragHelperElementHeight));

> Source/WebCore/inspector/front-end/Spectrum.js:303
> +    _updateUI: function() {

{ on the new line

> Source/WebCore/inspector/front-end/Spectrum.js:322
> +    toggle: function(element, color) {

ditto

> Source/WebCore/inspector/front-end/Spectrum.js:331
> +    show: function(element, color) {

ditto

> Source/WebCore/inspector/front-end/Spectrum.js:351
> +    hide: function() {

ditto

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:92
> +    this._spectrum = new WebInspector.Spectrum(this.bodyElement);

I think you should land the spectrum first and then enable it in a separate
change. You could also hide its use behind the Preferences.useSpectrum flag
first (see Settings.js).

> Source/WebCore/inspector/front-end/inspector.css:2668
> +.spectrum-container { 

fyi, no need to fix now: we might want to load this CSS lazily via using the
View infrastructure.

> Source/WebCore/inspector/front-end/utilities.js:284
> +Element.prototype.totalOffset = function(parent) 

Changes to the Color, Element prototype could also land as a separate change.
Do you mind filing separate bugs and extracting the patches for them? I would
allow us landing the spectrum faster.

> Source/WebCore/inspector/front-end/utilities.js:902
> +function stopPropagation(e) {

ditto, should be landed along with the element prototype changes.