[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





--- Comment #3 from Pavel Feldman <pfeldman at chromium.org>  2011-11-20 00:50:42 PST ---
(From update of attachment 115923)
View in context: https://bugs.webkit.org/attachment.cgi?id=115923&action=review

Thanks for putting it together. I made a first pass on the change to highlight some of the WebKit style guidelines and pointed to the UI utilities you could leverage. I'll comment on the UI separately.

> Source/WebCore/ChangeLog:3
> +        first commit for colorpicker functionality

This should be below WebInspector: topic, above Reviewed by line. Should also describe what you are doing in 1-2 sentences.

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

You should remove this line. There are currently no tests for inspector front-end UI components.

> Source/WebCore/inspector/front-end/Spectrum.js:2
> +WebInspector.Spectrum = function(swatch, rgb)

We are using Element suffix for all DOM element variables: swatch -> swatchElement
Passing Color instead of rgb would make more sense: otherwise it is not clear whether this is rgb or rgba.

> Source/WebCore/inspector/front-end/Spectrum.js:4
> +    this.document = swatch.ownerDocument;

We don't use iframes at the moment, so you should not need this.

> Source/WebCore/inspector/front-end/Spectrum.js:5
> +    this.swatch = swatch;

Here and below: prefix all private variables that are not accessed from outside this file with "_". this._swatchElement in this case.

> Source/WebCore/inspector/front-end/Spectrum.js:8
> +    this.element.innerHTML = WebInspector.Spectrum.markup;

We are not using any templating in the front-end, you should create your element using DOM API instead. There is Element.prototype.createChild = function(elementName, className) defined that is likely to make your code compact.

> Source/WebCore/inspector/front-end/Spectrum.js:16
> +    this.slider = this.element.querySelectorAll(".sp-hue")[0];

You won't need these once you create your dom programmatically.

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

Methods should also be private (this._updateUI())

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

Seems like this could belong to Color.js

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

ditto

> Source/WebCore/inspector/front-end/Spectrum.js:149
> +WebInspector.Spectrum.stopPropagation = function(e) {

Please try to use existing framework methods where possible or define generic ones in utilities.js.

> Source/WebCore/inspector/front-end/Spectrum.js:154
> +    if (typeof name === "object") {

We are preparing the front-end for closure compilation, so flexibility like this harms us.

> Source/WebCore/inspector/front-end/Spectrum.js:176
> +    return { left: el.totalOffsetLeft(), top: el.totalOffsetTop() };

should be defined on Element.prototype in utilities.js

> Source/WebCore/inspector/front-end/Spectrum.js:179
> +WebInspector.Spectrum.getScrollOffset = function(el) {

ditto

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

There is WebInspector.elementDragStart that has somewhat similar logic. Have you seen it?

> Source/WebCore/inspector/front-end/Spectrum.js:260
> +WebInspector.Spectrum.prototype = {

This class should probably be called WebInspector.SpectrumControl to better reflect the nature of the component.

> Source/WebCore/inspector/front-end/Spectrum.js:386
> +    addChangeListener: function(listener) {

Inheriting from Object.js provides you with the generic event mechanism. You would declare WebInspector.Spectrum.Event object with event names and dispatch them from here and from onhide.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1680
> +                var rgba = (color.rgba || color.rgb).slice(0);

It sounds like Color class itself is broken. It should maintain single data entry for rgb / rgba.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1681
> +                var spectrum = new WebInspector.Spectrum(swatchElement, rgba);

As I mentioned earlier, passing Color type here would be more straightforward.

> Source/WebCore/inspector/front-end/inspector.css:2644
> +.sp-container { 

Please be more verbose and expand those to .spectrum-*

> Source/WebCore/inspector/front-end/inspector.html:84
> +    <script type="text/javascript" src="Spectrum.js"></script>

You will need to add this file to: WebCore/WebCore.gypi, WebCore/inspector/front-end/WebKit.qrc and WebCore/WebCore.vcproj/WebCore.vjproj (last one uses tabs, not spaces). Sorry for trouble.

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