[CSS Shapes] inset args and radial args should serialize to the simplest form

[CSS Shapes] inset args and radial args should serialize to the simplest form

Add code for suppressing serializing of duplicate values. The implementation for the border-radius
part is inspired by valueForBorderRadiusShorthand.

BUG=350092
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171089

[rwlbuis, 2014-04-07 20:13:02]

This is showing up green. Marking Bear as reviewer first since he has done
similar patches in the past.

[Bear Travis, 2014-04-07 21:59:13]

The patch is looking good, only some minor comments for additional tests and
some potential refactorings in the serialization code.

My concern is just that the serialization function is getting a bit long, so any
potential cleanup / refactoring would be great.

https://codereview.chromium.org/227793002/diff/20001/LayoutTests/fast/shapes/...
File LayoutTests/fast/shapes/parsing/parsing-shape-outside-inset.html (right):

https://codereview.chromium.org/227793002/diff/20001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/parsing/parsing-shape-outside-inset.html:10: var
inset_tests = ['inset(10px)', 'inset(10px round 10px)']
Why have this test (parsing-shape-outside-inset) in addition to the parsing
tests in parsing-shape-outside?

https://codereview.chromium.org/227793002/diff/20001/LayoutTests/fast/shapes/...
File LayoutTests/fast/shapes/parsing/parsing-test-utils.js (right):

https://codereview.chromium.org/227793002/diff/20001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/parsing/parsing-test-utils.js:33: ["inset(10px 20px 30px
40px round 5px 6px)", "inset(10px 20px 30px 40px round 5px 6px)"],
The above all have the same set and get values, and therefore can be specified
as a string rather than an array (eg 'inset(10px)' rather than ['inset(10px)',
'inset(10px)'].

In addition, it would be helpful to add some tests where specified, duplicate
values are omitted during serialization, eg: inset(10px 20px 10px 10px) should
serialize to inset(10px 20px 10px), or inset(10px round 10px 20px 10px 10px /
10px 20px 10px 10px).

https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
File Source/core/css/CSSBasicShapes.cpp (right):

https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
Source/core/css/CSSBasicShapes.cpp:301: if (!right.isNull() && (right != top ||
showBottomArg)) {
Personally, I think this should be stored in a 'showRightArg' since it's using
the same format as show[Left/Bottom]Arg.

https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
Source/core/css/CSSBasicShapes.cpp:315: bool showHorizontalBottomLeft =
topRightRadiusWidth != bottomLeftRadiusWidth;
This logic seems like it could be factored out and shared with the above inset
simplification. Something like: appendSimplifiedInsetSyntax(result, one, two,
three, four). (Since it's either top, right, bottom, left or topLeft, topRight,
bottomRight, bottomLeft).

https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
Source/core/css/CSSBasicShapes.cpp:319: Vector<String> horizontalRadii;
Would it be possible to get around always creating these two vectors? I know
that the valueForBorderRadiusShorthand creates separate lists, but it seems a
bit expensive here.

https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
Source/core/css/CSSBasicShapes.cpp:328: if (horizontalRadii.size() > 1 ||
horizontalRadii[0] != "0px") {
This would not appear to catch the case inset(10px round 0px / 10px). I think it
should be: If any of the vertical radii are different than the horizontal radii,
append '/' and run the appendSimplifiedInsetSyntax method.

[rwlbuis, 2014-04-08 15:42:45]

On 2014/04/07 21:59:13, Bear Travis wrote:
> The patch is looking good, only some minor comments for additional tests and
> some potential refactorings in the serialization code.

Thanks for the review!

> My concern is just that the serialization function is getting a bit long, so
any
> potential cleanup / refactoring would be great.

Fair enough.

>
https://codereview.chromium.org/227793002/diff/20001/LayoutTests/fast/shapes/...
> File LayoutTests/fast/shapes/parsing/parsing-shape-outside-inset.html (right):
> 
>
https://codereview.chromium.org/227793002/diff/20001/LayoutTests/fast/shapes/...
> LayoutTests/fast/shapes/parsing/parsing-shape-outside-inset.html:10: var
> inset_tests = ['inset(10px)', 'inset(10px round 10px)']
> Why have this test (parsing-shape-outside-inset) in addition to the parsing
> tests in parsing-shape-outside?

Good point, I removed it.

>
https://codereview.chromium.org/227793002/diff/20001/LayoutTests/fast/shapes/...
> File LayoutTests/fast/shapes/parsing/parsing-test-utils.js (right):
> 
>
https://codereview.chromium.org/227793002/diff/20001/LayoutTests/fast/shapes/...
> LayoutTests/fast/shapes/parsing/parsing-test-utils.js:33: ["inset(10px 20px
30px
> 40px round 5px 6px)", "inset(10px 20px 30px 40px round 5px 6px)"],
> The above all have the same set and get values, and therefore can be specified
> as a string rather than an array (eg 'inset(10px)' rather than ['inset(10px)',
> 'inset(10px)'].

Fixed.

> In addition, it would be helpful to add some tests where specified, duplicate
> values are omitted during serialization, eg: inset(10px 20px 10px 10px) should
> serialize to inset(10px 20px 10px), or inset(10px round 10px 20px 10px 10px /
> 10px 20px 10px 10px).

I added some.

>
https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
> File Source/core/css/CSSBasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
> Source/core/css/CSSBasicShapes.cpp:301: if (!right.isNull() && (right != top
||
> showBottomArg)) {
> Personally, I think this should be stored in a 'showRightArg' since it's using
> the same format as show[Left/Bottom]Arg.

That is indeed more clear, fixed.

>
https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
> Source/core/css/CSSBasicShapes.cpp:315: bool showHorizontalBottomLeft =
> topRightRadiusWidth != bottomLeftRadiusWidth;
> This logic seems like it could be factored out and shared with the above inset
> simplification. Something like: appendSimplifiedInsetSyntax(result, one, two,
> three, four). (Since it's either top, right, bottom, left or topLeft,
topRight,
> bottomRight, bottomLeft).

I am not so sure, I think we need the Vector's.

>
https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
> Source/core/css/CSSBasicShapes.cpp:319: Vector<String> horizontalRadii;
> Would it be possible to get around always creating these two vectors? I know
> that the valueForBorderRadiusShorthand creates separate lists, but it seems a
> bit expensive here.

I am not seeing that atm.

>
https://codereview.chromium.org/227793002/diff/20001/Source/core/css/CSSBasic...
> Source/core/css/CSSBasicShapes.cpp:328: if (horizontalRadii.size() > 1 ||
> horizontalRadii[0] != "0px") {
> This would not appear to catch the case inset(10px round 0px / 10px). I think
it
> should be: If any of the vertical radii are different than the horizontal
radii,
> append '/' and run the appendSimplifiedInsetSyntax method.

You are right. I added the test and make it work.

My latest upload should fix all the tests, maybe we can chat on irc to see if
the code can be improved/simplified.

[Bear Travis, 2014-04-08 19:08:35]

The code is much easier to read with your changes. Had two quick comments then
lgtm.

https://codereview.chromium.org/227793002/diff/40001/Source/core/css/CSSBasic...
File Source/core/css/CSSBasicShapes.cpp (right):

https://codereview.chromium.org/227793002/diff/40001/Source/core/css/CSSBasic...
Source/core/css/CSSBasicShapes.cpp:320: Vector<String> horizontalRadii;
Would it be possible / worth it to factor out the horizontalRadii and
verticalRadii building into a helper method that calculates the booleans and
adds the requisite strings to the vector?

https://codereview.chromium.org/227793002/diff/40001/Source/core/css/CSSBasic...
Source/core/css/CSSBasicShapes.cpp:342: if (!(horizontalRadii.size() == 1 &&
horizontalRadii[0] == "0px" && verticalRadii[0] == "0px")) {
I think this also needs to check for verticalRadii.size being 1, otherwise
you'll miss the case of inset(10px round 0px / 0px 10px). Might be worth
factoring this into a method so you can specify what you're testing, eg
'areDefaultCornerRadii' or some such.

[rwlbuis, 2014-04-08 19:54:21]

On 2014/04/08 19:08:35, Bear Travis wrote:
> The code is much easier to read with your changes. Had two quick comments then
> lgtm.
> 
>
https://codereview.chromium.org/227793002/diff/40001/Source/core/css/CSSBasic...
> File Source/core/css/CSSBasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/227793002/diff/40001/Source/core/css/CSSBasic...
> Source/core/css/CSSBasicShapes.cpp:320: Vector<String> horizontalRadii;
> Would it be possible / worth it to factor out the horizontalRadii and
> verticalRadii building into a helper method that calculates the booleans and
> adds the requisite strings to the vector?

Yep, good idea, that cleans up the code.

>
https://codereview.chromium.org/227793002/diff/40001/Source/core/css/CSSBasic...
> Source/core/css/CSSBasicShapes.cpp:342: if (!(horizontalRadii.size() == 1 &&
> horizontalRadii[0] == "0px" && verticalRadii[0] == "0px")) {
> I think this also needs to check for verticalRadii.size being 1, otherwise
> you'll miss the case of inset(10px round 0px / 0px 10px). Might be worth
> factoring this into a method so you can specify what you're testing, eg
> 'areDefaultCornerRadii' or some such.

Again a good idea, I tried to do as much in my new upload.

[Bear Travis, 2014-04-08 21:19:46]

Very nice, lgtm.

[rwlbuis, 2014-04-08 21:20:37]

More serialization finetuning.

[eseidel, 2014-04-08 21:24:55]

lgtm

[commit-bot: I haz the power, 2014-04-08 21:25:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/227793002/60001

[commit-bot: I haz the power, 2014-04-08 21:25:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-08 21:25:10]

Failed to apply patch for
LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt
  Hunk #1 FAILED at 5.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt.rej

Patch:       LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt
Index: LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt
diff --git a/LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt
b/LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt
index
9d70233092a4e9ee5944bb7b9f8ec5822cf3b348..8656523d13ce1ae00d530722fb7d44268ca44f72
100644
--- a/LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt
+++ b/LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt
@@ -5,56 +5,68 @@ On success, you will see a series of "PASS" messages, followed
by "TEST COMPLETE
 
 PASS getCSSText("shape-outside", "none") is "none"
 PASS getComputedStyleValue("shape-outside", "none") is "none"
-PASS getCSSText("shape-outside", "inset(10px)") is "inset(10px 10px 10px 10px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px)") is "inset(10px 10px
10px 10px round 0px 0px 0px 0px / 0px 0px 0px 0px)"
-PASS getCSSText("shape-outside", "inset(10px 9px)") is "inset(10px 9px 10px
9px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px 9px)") is "inset(10px
9px 10px 9px round 0px 0px 0px 0px / 0px 0px 0px 0px)"
-PASS getCSSText("shape-outside", "inset(10px 9px 8px)") is "inset(10px 9px 8px
9px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px 9px 8px)") is
"inset(10px 9px 8px 9px round 0px 0px 0px 0px / 0px 0px 0px 0px)"
+PASS getCSSText("shape-outside", "inset(10px)") is "inset(10px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px)") is "inset(10px)"
+PASS getCSSText("shape-outside", "inset(10px 9px)") is "inset(10px 9px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px 9px)") is "inset(10px
9px)"
+PASS getCSSText("shape-outside", "inset(10px 9px 8px)") is "inset(10px 9px
8px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px 9px 8px)") is
"inset(10px 9px 8px)"
 PASS getCSSText("shape-outside", "inset(10px 20px 30px 40px)") is "inset(10px
20px 30px 40px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px 20px 30px 40px)") is
"inset(10px 20px 30px 40px round 0px 0px 0px 0px / 0px 0px 0px 0px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px)") is "inset(10px 10px
10px 10px round 9px 9px 9px 9px / 9px 9px 9px 9px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px)") is
"inset(10px 10px 10px 10px round 9px 9px 9px 9px / 9px 9px 9px 9px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px / 9px)") is "inset(10px
10px 10px 10px round 9px 9px 9px 9px / 9px 9px 9px 9px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px / 9px)") is
"inset(10px 10px 10px 10px round 9px 9px 9px 9px / 9px 9px 9px 9px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px / 8px)") is "inset(10px
10px 10px 10px round 9px 9px 9px 9px / 8px 8px 8px 8px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px / 8px)") is
"inset(10px 10px 10px 10px round 9px 9px 9px 9px / 8px 8px 8px 8px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px / 8px 7px)") is
"inset(10px 10px 10px 10px round 9px 9px 9px 9px / 8px 7px 8px 7px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px / 8px 7px)")
is "inset(10px 10px 10px 10px round 9px 9px 9px 9px / 8px 7px 8px 7px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px / 8px 7px 6px)") is
"inset(10px 10px 10px 10px round 9px 9px 9px 9px / 8px 7px 6px 7px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px / 8px 7px
6px)") is "inset(10px 10px 10px 10px round 9px 9px 9px 9px / 8px 7px 6px 7px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px / 8px 7px 6px 5px)") is
"inset(10px 10px 10px 10px round 9px 9px 9px 9px / 8px 7px 6px 5px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px / 8px 7px 6px
5px)") is "inset(10px 10px 10px 10px round 9px 9px 9px 9px / 8px 7px 6px 5px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px / 7px)") is
"inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 7px 7px 7px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px / 7px)")
is "inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 7px 7px 7px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px / 7px 6px)") is
"inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 6px 7px 6px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px / 7px
6px)") is "inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 6px 7px 6px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px / 7px 6px)") is
"inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 6px 7px 6px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px / 7px
6px)") is "inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 6px 7px 6px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px / 7px 6px 5px)") is
"inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 6px 5px 6px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px / 7px 6px
5px)") is "inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 6px 5px 6px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px / 7px 6px 5px 4px)")
is "inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 6px 5px 4px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px / 7px 6px
5px 4px)") is "inset(10px 10px 10px 10px round 9px 8px 9px 8px / 7px 6px 5px
4px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px 7px / 6px)") is
"inset(10px 10px 10px 10px round 9px 8px 7px 8px / 6px 6px 6px 6px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px 7px /
6px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 8px / 6px 6px 6px 6px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px 7px / 6px 5px)") is
"inset(10px 10px 10px 10px round 9px 8px 7px 8px / 6px 5px 6px 5px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px 7px / 6px
5px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 8px / 6px 5px 6px 5px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px 7px / 6px 5px 4px)")
is "inset(10px 10px 10px 10px round 9px 8px 7px 8px / 6px 5px 4px 5px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px 7px / 6px
5px 4px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 8px / 6px 5px 4px
5px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px 7px / 6px 5px 4px
3px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 8px / 6px 5px 4px 3px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px 7px / 6px
5px 4px 3px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 8px / 6px 5px 4px
3px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px 7px 6px / 5px)") is
"inset(10px 10px 10px 10px round 9px 8px 7px 6px / 5px 5px 5px 5px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px 7px 6px /
5px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 6px / 5px 5px 5px 5px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px 7px 6px / 5px 4px)")
is "inset(10px 10px 10px 10px round 9px 8px 7px 6px / 5px 4px 5px 4px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px 7px 6px /
5px 4px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 6px / 5px 4px 5px
4px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px 7px 6px / 5px 4px
3px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 6px / 5px 4px 3px 4px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px 7px 6px /
5px 4px 3px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 6px / 5px 4px 3px
4px)"
-PASS getCSSText("shape-outside", "inset(10px round 9px 8px 7px 6px / 5px 4px
3px 2px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 6px / 5px 4px 3px
2px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px round 9px 8px 7px 6px /
5px 4px 3px 2px)") is "inset(10px 10px 10px 10px round 9px 8px 7px 6px / 5px 4px
3px 2px)"
-PASS getCSSText("shape-outside", "inset(10px 20px 30px 40px round 5px 6px)") is
"inset(10px 20px 30px 40px round 5px 6px 5px 6px / 5px 6px 5px 6px)"
-PASS getComputedStyleValue("shape-outside", "inset(10px 20px 30px 40px round
5px 6px)") is "inset(10px 20px 30px 40px round 5px 6px 5px 6px / 5px 6px 5px
6px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px 20px 30px 40px)") is
"inset(10px 20px 30px 40px)"
+PASS getCSSText("shape-outside", "inset(10px 20px 30px 20px)") is "inset(10px
20px 30px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px 20px 30px 20px)") is
"inset(10px 20px 30px)"
+PASS getCSSText("shape-outside", "inset(10px 20px 10px 20px)") is "inset(10px
20px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px 20px 10px 20px)") is
"inset(10px 20px)"
+PASS getCSSText("shape-outside", "inset(10px round 9px)") is "inset(10px round
9px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px round 9px)") is
"inset(10px round 9px)"
+PASS getCSSText("shape-outside", "inset(10px round 0px / 0px 10px)") is
"inset(10px round 0px / 0px 10px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px round 0px / 0px 10px)")
is "inset(10px round 0px / 0px 10px)"
+PASS getCSSText("shape-outside", "inset(10px round 9px / 9px)") is "inset(10px
round 9px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px round 9px / 9px)") is
"inset(10px round 9px)"
+PASS getCSSText("shape-outside", "inset(10px round 9px / 8px)") is "inset(10px
round 9px / 8px)"
+PASS getComputedStyleValue("shape-outside", "inset(10px round 9px / 8px)") is
"inset(10px round 9px / 8px)"
+PASS getCSSText("shape-outside", "inset(10p…
(message too large)

[rwlbuis, 2014-04-08 21:29:31]

On 2014/04/08 21:24:55, eseidel wrote:
> lgtm

Thanks, looks like I have to rebase.

[rwlbuis, 2014-04-08 21:39:14]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-04-08 21:39:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/227793002/80001

[commit-bot: I haz the power, 2014-04-08 22:50:45]

Change committed as 171089