[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


Pavel Feldman <pfeldman at chromium.org> changed:

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




--- Comment #31 from Pavel Feldman <pfeldman at chromium.org>  2011-12-15 02:23:42 PST ---
(From update of attachment 119233)
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.

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