Bug 234562 – Web Inspector: Support conic gradients in gradient editor and autocompletion

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Tim Nguyen (:ntim)]

The web inspector has a swatch for gradients (linear, repeating-linear, radial, repeating-radial), it should also have one for conic and repeating-conic types.

[Tim Nguyen (:ntim)]

Created attachment 447727 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 447885 [details]
Patch

[Devin Rousso]

Comment on attachment 447885 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:42
> +        return { value: parseFloat(match[1]), units: match[2] };

Style: we dont normally add spaces after `{` or before `}` in Web Inspector

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:105
> +        else if (type === WI.Gradient.Types.Radial)

NIT: I'd use a `switch` instead of a bunch of `else if`

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:165
> +    set angleValue(value) { this._angle.value = value; }

Style: We normally only make `get`/`set` single line if the function body is simple (e.g. assign/return a variable of the same name) and if the corresponding `set`/`get` (assuming there is one) is also able to be a single line.  Since `get angleValue` is not single line, please change this to also not be single line.
Style: We normally put the `get` before the `set`.

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:181
> +    get angleUnits() { return this._angle.units; }

ditto (:165)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:221
> +        default:
> +            WI.reportInternalError(`Unknown angle units "${this._angle.units}"`);
> +            return 0;

NIT: I don't think this is needed since you've already covered all the unit types in the `switch`.

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:228
> +            value = deg;

NIT: rather than reassign to `value`, you could just `return deg;`

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:232
> +            value = deg * Math.PI / 180;

ditto (:228)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:236
> +            value = deg / 360 * 400;

ditto (:228)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:240
> +            value = deg / 360;

ditto (:228)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:244
> +        return value;

ditto (:228) with a final `return 0;`

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:375
> +        var sizing = !WI.Color.fromString(components[0].join(" ")) ? components.shift().join(" ") : "";

`let`

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:377
> +        var stops = WI.Gradient.stopsWithComponents(components);

ditto (:375)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:386
> +    set angleValue(value) { }

I think it'd be worth having a `console.assert(false, "CSS conic gradients do not have an angle")` so that if this is somehow invoked then an engineer can notice it and fix it.

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:388
> +    get angleValue() { return null; }

I think we should have this be `return 0;` as the default just in case since `WI.Gradient.prototype.get angleValue` always returns a number.

ditto (:386)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:390
> +    set angleUnits(units) { }

ditto (:386)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:392
> +    get angleUnits() { return null; }

I think we should have this be `return "";` as the default just in case since `WI.Gradient.prototype.get angleUnit` always returns a string.

ditto (:386)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:401
> +        var str = this.sizing;

ditto (:375)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:403
> +        if (str !== "")

Style: we normally dont compare directly against `""` instead preferring to just boolean check the value since `""` is a falsy value
```
if (str)
```

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:426
> +        let angle = { value: 0, units: WI.Gradient.AngleUnits.DEG };

ditto (:42)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:440
> +            // TODO: Potentially do some more elaborate parsing of position.

Can you create a FIXME bugzilla for this, ideally with some more elaborate examples?

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:446
> +        if (hasCustomAngleOrPosition) {

Style: no `{` `}` needed for single line control flow

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:468
> +        if (this._angle.value > 0)

Are negative angles not allowed?

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:472
> +            if (str != "")

ditto (:403)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:477
> +        if (str != "")

ditto (:403)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:484
> -};
> +}

Style: this should have a `;` at the end since it's an assignment

> Source/WebInspectorUI/UserInterface/Views/GradientEditor.js:146
> +            this._gradientTypePicker.value = this._gradient.repeats ? "repeating-conic-gradient" : "conic-gradient";

Does this also need a `this._angleUnitsChanged();` right after?

> Source/WebInspectorUI/UserInterface/Views/GradientEditor.js:205
> +            } else if (descriptor.type === WI.RadialGradient)

ditto (:105)

[Tim Nguyen (:ntim)]

(In reply to Devin Rousso from comment #3)
> > Source/WebInspectorUI/UserInterface/Views/GradientEditor.js:146
> > +            this._gradientTypePicker.value = this._gradient.repeats ? "repeating-conic-gradient" : "conic-gradient";
> 
> Does this also need a `this._angleUnitsChanged();` right after?

The angle UI is hidden for radial gradients, so it doesn't really matter.

[Tim Nguyen (:ntim)]

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:468
> +        if (this._angle.value > 0)


> Are negative angles not allowed?

Yes, they are, nice catch!

[Tim Nguyen (:ntim)]

Created attachment 447901 [details]
Patch

[Devin Rousso]

Comment on attachment 447901 [details]
Patch

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

r=me, nice fix! =D

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:117
> +        default:
> +            return null;

NIT: this isn't really necessary since you've covered all the other cases already and the below logic will handle if `gradient` is not set (tho you'd probably wanna make it `let gradient = null;` just in case)

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:458
> +            // FIXME: webkit.org/b/234643

NIT: we normally include the title of the bug too so it's clear (and searchable)
```
// FIXME: <https://webkit.org/b/234643> (Web Inspector: allow editing positions in gradient editor)
```

[Tim Nguyen (:ntim)]

Created attachment 447902 [details]
Patch

[Tim Nguyen (:ntim)]

Committed r287409 (245544@trunk): <https://commits.webkit.org/245544@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/86863005>

[Tim Nguyen (:ntim)]

Landed a fix up for the assert text: r287491