Bug 166938

Summary: Web Inspector: gradient editor should allow switching the units of the 'angle' parameter
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=166937
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Patch
bburg: review+
Patch
commit-queue: commit-queue-
Patch none

Description BJ Burg 2017-01-11 12:09:38 PST
Supported units include degrees, grads, rads, and turn o_O

https://developer.mozilla.org/en-US/docs/Web/CSS/angle
Comment 1 Radar WebKit Bug Importer 2017-01-11 12:16:25 PST
<rdar://problem/29974340>
Comment 2 Devin Rousso 2017-01-11 19:46:21 PST
It looks like both the Gradient model object and the RegExp to match CSS gradient strings only support using "deg" as the units for angles.  Both will have to be updated.
Comment 3 Devin Rousso 2017-01-30 11:40:34 PST
Created attachment 300127 [details]
Patch
Comment 4 Devin Rousso 2017-01-30 11:40:58 PST
Created attachment 300128 [details]
[Image] After Patch is applied
Comment 5 BJ Burg 2017-02-02 10:06:46 PST
Comment on attachment 300127 [details]
Patch

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

Code changes look good (you did very neat things!) but needs a basic test as noted below.

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:179
> +        if (components[0].length === 1) {

Since we are doing our own parsing in model code, I would expect that we have some basic test cases. Can you make a model test for Gradient that parses a few good and bad values for each angle unit of measure?

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:259
> +        if (deg === 0)

I am surprised there are no tests for toString() either. Can you do something basic to test this?

> Source/WebInspectorUI/UserInterface/Views/GradientEditor.js:102
> +        this._angleUnitsConfiguration = new Map(angleUnitsData.map(({name, min, max, step}) => {

Wow! This took a bit of thinking, but I think i like it as a pattern for fixed configurations (as long as the pattern is consistently used). It's similar to the Java way of creating anonymous class instances for enums.
Comment 6 Devin Rousso 2017-02-02 23:33:17 PST
Created attachment 300505 [details]
Patch
Comment 7 BJ Burg 2017-02-07 11:47:39 PST
Comment on attachment 300505 [details]
Patch

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

r=me, great work!

> Source/WebInspectorUI/UserInterface/Models/Gradient.js:318
> +            WebInspector.reportInternalError("Unknown angle units");

Nit: Please include the unknown value in the error message.
Comment 8 Devin Rousso 2017-02-07 12:48:50 PST
Created attachment 300837 [details]
Patch
Comment 9 WebKit Commit Bot 2017-02-07 12:50:54 PST
Comment on attachment 300837 [details]
Patch

Rejecting attachment 300837 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 300837, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3021224
Comment 10 Devin Rousso 2017-02-07 14:06:54 PST
Created attachment 300850 [details]
Patch
Comment 11 WebKit Commit Bot 2017-02-07 14:33:19 PST
Comment on attachment 300850 [details]
Patch

Clearing flags on attachment: 300850

Committed r211840: <http://trac.webkit.org/changeset/211840>
Comment 12 WebKit Commit Bot 2017-02-07 14:33:23 PST
All reviewed patches have been landed.  Closing bug.