Bug 166938 - Web Inspector: gradient editor should allow switching the units of the 'angle' parameter
Summary: Web Inspector: gradient editor should allow switching the units of the 'angle...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
Keywords: InRadar
Depends on:
Reported: 2017-01-11 12:09 PST by BJ Burg
Modified: 2017-02-07 14:33 PST (History)
4 users (show)

See Also:

Patch (13.76 KB, patch)
2017-01-30 11:40 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (50.07 KB, image/png)
2017-01-30 11:40 PST, Devin Rousso
no flags Details
Patch (26.36 KB, patch)
2017-02-02 23:33 PST, Devin Rousso
bburg: review+
Details | Formatted Diff | Diff
Patch (25.88 KB, patch)
2017-02-07 12:48 PST, Devin Rousso
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (25.88 KB, patch)
2017-02-07 14:06 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-01-11 12:09:38 PST
Supported units include degrees, grads, rads, and turn o_O

Comment 1 Radar WebKit Bug Importer 2017-01-11 12:16:25 PST
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]
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]

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]
Comment 7 BJ Burg 2017-02-07 11:47:39 PST
Comment on attachment 300505 [details]

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]
Comment 9 WebKit Commit Bot 2017-02-07 12:50:54 PST
Comment on attachment 300837 [details]

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]
Comment 11 WebKit Commit Bot 2017-02-07 14:33:19 PST
Comment on attachment 300850 [details]

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.