Bug 153004 – Web Inspector: Add support for the existing GradientEditor in the CSS Rules sidebar

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Gradients are supported in the Visual sidebar and on Resource views, but not in the Rules sidebar.  This should be added there seeing as it is where most users will interact with gradients.

[Devin Rousso]

Created attachment 268749 [details]
Patch

[Blaze Burg]

Comment on attachment 268749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268749&action=review

Looking pretty good overall (with some naming suggestions). I would like a second look from Joe, Tim, or Matt.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:877
> +            let swatch = new WebInspector.ColorSwatch(color, this._codeMirror.getOption("readOnly"));

(Applies throughout) The code quickly gets confusing because a "color" could actually be a gradient, and clicking the swatch could show different things. What about renaming it to WebInspector.InlineSwatch(value, readonly)? Then value could be a color or gradient.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorTextMarkers.js:95
> +        let closedParenthesis = 0;

It would be easier to make sense of this code if it tracked the count of unmatched opening parenthesis. Maybe a comment above the if (closedParenthesis < 0) would make its purpose more obvious.

> Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js:29
> +    constructor(color, readOnly, isGradient)

:( I think it would be better to take a 'value' into this._value and make isGradient a getter that does an instanceof test. The parameter is redundant.

> Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js:39
> +            this._swatchElement.title = WebInspector.UIString("Click to select a gradient");

Nit: needs trailing period to match other tooltip.

> Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js:66
> +        this._color = this._addFallbackValueIfNeeded(color);

Try this:

this._value = value || this._fallbackValue()

> Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js:117
> +        colorPicker[this._isGradient ? "gradient" : "color"] = this._color;

Nit: we generally tend to avoid computed properties with ternary expression, as it's harder to read (and grep).

[Devin Rousso]

Created attachment 268851 [details]
Patch

[Timothy Hatcher]

Comment on attachment 268851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268851&action=review

Awesome clean up!

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:43
> +        else
> +            this._swatchElement.title = WebInspector.UIString("Click to select a color. Shift-click to switch color formats.");

assert Type.Color

[Timothy Hatcher]

Comment on attachment 268851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268851&action=review

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:43
>> +            this._swatchElement.title = WebInspector.UIString("Click to select a color. Shift-click to switch color formats.");
> 
> assert Type.Color

Could use a switch here and assert for the default case.

[Devin Rousso]

Comment on attachment 268851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268851&action=review

>>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:43
>>> +            this._swatchElement.title = WebInspector.UIString("Click to select a color. Shift-click to switch color formats.");
>> 
>> assert Type.Color
> 
> Could use a switch here and assert for the default case.

I made the change, but I am just wondering if there is a good indicator of when to use a switch instead of an if?  I know that there is some performance improvements on semi-large (3 to 10 options) possibility sets, but I wasn't sure how it effected JS.  Is there a rule-of-thumb that WebInspector uses?

[Devin Rousso]

Created attachment 268887 [details]
Patch

[Blaze Burg]

(In reply to comment #6)
> Comment on attachment 268851 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268851&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:43
> >>> +            this._swatchElement.title = WebInspector.UIString("Click to select a color. Shift-click to switch color formats.");
> >> 
> >> assert Type.Color
> > 
> > Could use a switch here and assert for the default case.
> 
> I made the change, but I am just wondering if there is a good indicator of
> when to use a switch instead of an if?  I know that there is some
> performance improvements on semi-large (3 to 10 options) possibility sets,
> but I wasn't sure how it effected JS.  Is there a rule-of-thumb that
> WebInspector uses?

It's mostly about readability and whether the switch will catch new unhandled values in the future. Don't worry about the performance impact, it's probably negligible.

[WebKit Commit Bot]

Comment on attachment 268887 [details]
Patch

Clearing flags on attachment: 268887

Committed r194977: <http://trac.webkit.org/changeset/194977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/24192983>