Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.
Since parseRadii does not add properties, do that when handling
the CSSPropertyBorderRadius case.

BUG=499780
Committed: https://crrev.com/9fae3fedaa0cd615de30062aed6aeec8a4051351
Cr-Commit-Position: refs/heads/master@{#368596}

[rwlbuis, 2015-09-01 14:59:20]

Patchset #2 (id:20001) has been deleted

[rwlbuis, 2015-12-16 03:29:58]

Description was changed from

==========
parseBorderRadius WIP

BUG=
==========

to

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.

BUG=
==========

[rwlbuis, 2015-12-16 03:30:54]

Description was changed from

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.

BUG=
==========

to

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.

BUG=499780
==========

[rwlbuis, 2015-12-16 03:36:58]

Description was changed from

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.

BUG=499780
==========

to

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.
Since parseRadii does not add properties, do that when handling
the CSSPropertyBorderRadius case.

BUG=499780
==========

[rwlbuis, 2015-12-16 03:37:48]

rob.buis@samsung.com changed reviewers:
+ alancutter@chromium.org, timloh@chromium.org

[rwlbuis, 2015-12-16 03:37:48]

PTAL. I think it is best to do this before tackling shapes.

[rwlbuis, 2016-01-07 06:18:56]

On 2015/12/16 03:37:48, rwlbuis wrote:
> PTAL. I think it is best to do this before tackling shapes.

PTAL, this one is convenient and I have a border-radius patch building on it.

[Timothy Loh, 2016-01-11 02:01:46]

ok lgtm

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:535: case
CSSPropertyAliasWebkitBorderRadius: {
btw the switch is on a resolved property so the alias case isn't actually
needed.

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2863: //
FIXME: Refactor completeBorderRadii and the array
What did you have in mind? Or is this just a legacy comment?

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3999: if
(!i || (value && value->iValue != '/'))
probably need to check that m_unit is Operator

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:4012: for
(i = 0; i < 4 && value && validUnit(value, FLength | FPercent | FNonNeg); ++i,
value = args->next())
I don't like having so much logic inside a for-loop header, but I guess it's ok
for now -- I'll comment on the coding style more when we start to move the logic
across to use CSSParserTokens.

[rwlbuis, 2016-01-11 14:44:27]

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

[rwlbuis, 2016-01-11 14:44:28]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1311413004/#ps100001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-01-11 14:44:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1311413004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1311413004/100001

[rwlbuis, 2016-01-11 14:46:19]

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:535: case
CSSPropertyAliasWebkitBorderRadius: {
On 2016/01/11 02:01:46, Timothy Loh wrote:
> btw the switch is on a resolved property so the alias case isn't actually
> needed.

Done.

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2863: //
FIXME: Refactor completeBorderRadii and the array
On 2016/01/11 02:01:46, Timothy Loh wrote:
> What did you have in mind? Or is this just a legacy comment?

Well I am still not completely sure the array is the best solution, hence the
FIXME. Maybe it can be addressed once it is ported to use CSSParserTokens.

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3999: if
(!i || (value && value->iValue != '/'))
On 2016/01/11 02:01:46, Timothy Loh wrote:
> probably need to check that m_unit is Operator

Done.

https://codereview.chromium.org/1311413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:4012: for
(i = 0; i < 4 && value && validUnit(value, FLength | FPercent | FNonNeg); ++i,
value = args->next())
On 2016/01/11 02:01:46, Timothy Loh wrote:
> I don't like having so much logic inside a for-loop header, but I guess it's
ok
> for now -- I'll comment on the coding style more when we start to move the
logic
> across to use CSSParserTokens.

Acknowledged.

[commit-bot: I haz the power, 2016-01-11 16:03:45]

Description was changed from

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.
Since parseRadii does not add properties, do that when handling
the CSSPropertyBorderRadius case.

BUG=499780
==========

to

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.
Since parseRadii does not add properties, do that when handling
the CSSPropertyBorderRadius case.

BUG=499780
==========

[commit-bot: I haz the power, 2016-01-11 16:03:45]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-01-11 16:04:28]

Description was changed from

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.
Since parseRadii does not add properties, do that when handling
the CSSPropertyBorderRadius case.

BUG=499780
==========

to

==========
Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.
Since parseRadii does not add properties, do that when handling
the CSSPropertyBorderRadius case.

BUG=499780

Committed: https://crrev.com/9fae3fedaa0cd615de30062aed6aeec8a4051351
Cr-Commit-Position: refs/heads/master@{#368596}
==========

[commit-bot: I haz the power, 2016-01-11 16:04:28]

Patchset 5 (id:??) landed as
https://crrev.com/9fae3fedaa0cd615de30062aed6aeec8a4051351
Cr-Commit-Position: refs/heads/master@{#368596}