[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 #6 from Brian Grinstead <briangrinstead at gmail.com>  2011-11-20 08:23:44 PST ---
Copy that, I'll make the suggested changes.  Should I revert my initial changelog, and rebase all of the new changes into the initial commit, then generate a new changelog and patch?


(In reply to comment #3)
> (From update of attachment 115923 [details])
> 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.