Support consecutive slash operators for border-image value

Support consecutive slash operators for border-image value

According to http://www.w3.org/TR/css3-background/#the-border-image,
border-image-width value is optional and there could be consecutive
slash operators between border-image-slice and border-image-outset.

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

[je_julie(Not used), 2014-08-14 08:37:13]

Bruno, PTAL.

[abinader, 2014-08-18 14:04:17]

On 2014/08/14 08:37:13, je_julie.kim wrote:
> Bruno, PTAL.

Looks good to me, could you please put some try bots to run? I tried but it
seems there is an error impeding me from running try bots on your change.

[je_julie(Not used), 2014-08-21 01:37:24]

On 2014/08/18 14:04:17, abinader wrote:
> On 2014/08/14 08:37:13, je_julie.kim wrote:
> > Bruno, PTAL.
> 
> Looks good to me, could you please put some try bots to run? I tried but it
> seems there is an error impeding me from running try bots on your change.

I rebased patch but I still got red from trybot.
I heard that it could be caused because I don't have permission for trybot.
So, I'll move to get official review.
Thank you for your help.

[je_julie(Not used), 2014-08-21 01:40:10]

Rob, PTAL.

[rwlbuis, 2014-08-21 15:54:26]

https://codereview.chromium.org/471893002/diff/40001/LayoutTests/fast/css/bor...
File LayoutTests/fast/css/border-image-value-grammar-expected.html (right):

https://codereview.chromium.org/471893002/diff/40001/LayoutTests/fast/css/bor...
LayoutTests/fast/css/border-image-value-grammar-expected.html:17:
print_output(element);
I think it is preferred to use shouldBe machinery. Can you use that instead?

https://codereview.chromium.org/471893002/diff/40001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (left):

https://codereview.chromium.org/471893002/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:5818: , m_requireWidth(false)
Renaming this variable makes the review much harder, is it really needed?

[je_julie(Not used), 2014-08-22 00:31:39]

Thank you for your review.
Please check my comment.

https://codereview.chromium.org/471893002/diff/40001/LayoutTests/fast/css/bor...
File LayoutTests/fast/css/border-image-value-grammar-expected.html (right):

https://codereview.chromium.org/471893002/diff/40001/LayoutTests/fast/css/bor...
LayoutTests/fast/css/border-image-value-grammar-expected.html:17:
print_output(element);
On 2014/08/21 15:54:25, rwlbuis wrote:
> I think it is preferred to use shouldBe machinery. Can you use that instead?
OK. I'll change to use 'shouldBe'.

https://codereview.chromium.org/471893002/diff/40001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (left):

https://codereview.chromium.org/471893002/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:5818: , m_requireWidth(false)
On 2014/08/21 15:54:25, rwlbuis wrote:
> Renaming this variable makes the review much harder, is it really needed?
From the spec,http://www.w3.org/TR/css3-background/#the-border-image, 
The value can have '<‘border-image-source’> || <‘border-image-slice’> [ /
<‘border-image-width’> | / <‘border-image-width’>? / <‘border-image-outset’> ]?
|| <‘border-image-repeat’>'.
Before this patch, CSS needed 'width' necessarily.
After this patch, 'width' is optional. So, I changed variable name for
readability.
Should I keep original variable name?

[je_julie(Not used), 2014-08-26 10:38:57]

Rob, I updated testcase using shoudBe.
As to rename field, please check my comment.
Thanks,

[rwlbuis, 2014-08-28 18:00:27]

https://codereview.chromium.org/471893002/diff/60001/LayoutTests/fast/css/bor...
File LayoutTests/fast/css/border-image-value-grammar.html (right):

https://codereview.chromium.org/471893002/diff/60001/LayoutTests/fast/css/bor...
LayoutTests/fast/css/border-image-value-grammar.html:10: }
Do you really need all of this? I think you just need one element to test this
right?

[je_julie(Not used), 2014-08-29 10:25:39]

https://codereview.chromium.org/471893002/diff/60001/LayoutTests/fast/css/bor...
File LayoutTests/fast/css/border-image-value-grammar.html (right):

https://codereview.chromium.org/471893002/diff/60001/LayoutTests/fast/css/bor...
LayoutTests/fast/css/border-image-value-grammar.html:10: }
On 2014/08/28 18:00:27, rwlbuis wrote:
> Do you really need all of this? I think you just need one element to test this
> right?
Yes, you're right. I'll make it simple.

[je_julie(Not used), 2014-08-29 10:31:38]

Rob, thank you for your comment and I updated TC.
PTAL.

[rwlbuis, 2014-08-29 15:12:11]

rob.buis@samsung.com changed reviewers:
+ eseidel@chromium.org, esprehn@chromium.org

[rwlbuis, 2014-08-29 15:12:11]

Code and test look reasonable to me. Handing it off to the CSS experts now, I
suspect since the yacc rules are changed performance needs to be tested too.
PTAL.

[je_julie(Not used), 2014-09-02 12:21:35]

esprehn, eseidel,
PTAL.

[rwlbuis, 2014-11-20 22:37:04]

On 2014/09/02 12:21:35, je_julie wrote:
> esprehn, eseidel,
> PTAL.

Hi, you may want to try Tim Loh for the review, but also I wonder if this patch
still compiles?

[esprehn, 2014-11-20 22:54:06]

https://codereview.chromium.org/471893002/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/471893002/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:5718: m_allowRepeat =
m_allowForwardSlashOperator = m_allowWidth = m_requireOutset = false;
One assignment per line.

[je_julie(Not used), 2014-11-21 08:36:38]

Thanks for looking into it.

rwlbuis,
I ran try bots with this patch.

esprehn,
I updated patchset with one assignments per line.

PTAL.

https://codereview.chromium.org/471893002/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/471893002/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:5718: m_allowRepeat =
m_allowForwardSlashOperator = m_allowWidth = m_requireOutset = false;
On 2014/11/20 22:54:06, esprehn wrote:
> One assignment per line.
Done.

[je_julie(Not used), 2014-12-16 04:18:17]

je_julie.kim@samsung.com changed reviewers:
+ timloh@chromium.org

[je_julie(Not used), 2014-12-16 04:20:34]

Tim and esprehn,
Could you take a look at this patch if you're available?

On 2014/11/21 08:36:38, je_julie wrote:
> Thanks for looking into it.
> 
> rwlbuis,
> I ran try bots with this patch.
> 
> esprehn,
> I updated patchset with one assignments per line.
> 
> PTAL.

[Timothy Loh, 2014-12-18 23:12:12]

On 2014/12/16 04:20:34, je_julie wrote:
> Tim and esprehn,
> Could you take a look at this patch if you're available?
> 
> On 2014/11/21 08:36:38, je_julie wrote:
> > Thanks for looking into it.
> > 
> > rwlbuis,
> > I ran try bots with this patch.
> > 
> > esprehn,
> > I updated patchset with one assignments per line.
> > 
> > PTAL.

lgtm, this is fine.

[Timothy Loh, 2014-12-18 23:13:06]

https://codereview.chromium.org/471893002/diff/100001/LayoutTests/fast/css/bo...
File LayoutTests/fast/css/border-image-value-grammar.html (right):

https://codereview.chromium.org/471893002/diff/100001/LayoutTests/fast/css/bo...
LayoutTests/fast/css/border-image-value-grammar.html:9:
shouldBe("style.getPropertyValue('border-image-slice')", "'3'");
Can we use shouldBeEqualToString? :)

[je_julie(Not used), 2014-12-19 02:17:53]

Tim,
Thanks a lot for review this patch.

I updated TC with 'shouldBeEqualToString'.
Could you take another look at that?
and do I need to get review from another reviewer additionally?

https://codereview.chromium.org/471893002/diff/100001/LayoutTests/fast/css/bo...
File LayoutTests/fast/css/border-image-value-grammar.html (right):

https://codereview.chromium.org/471893002/diff/100001/LayoutTests/fast/css/bo...
LayoutTests/fast/css/border-image-value-grammar.html:9:
shouldBe("style.getPropertyValue('border-image-slice')", "'3'");
On 2014/12/18 23:13:05, Timothy Loh wrote:
> Can we use shouldBeEqualToString? :)

Done.

[Timothy Loh, 2014-12-19 02:27:29]

On 2014/12/19 02:17:53, je_julie wrote:
> Tim,
> Thanks a lot for review this patch.
> 
> I updated TC with 'shouldBeEqualToString'.
> Could you take another look at that?
> and do I need to get review from another reviewer additionally?

Feel free to go ahead and land the patch.

[je_julie(Not used), 2014-12-19 03:29:08]

The CQ bit was checked by je_julie.kim@samsung.com

[commit-bot: I haz the power, 2014-12-19 03:30:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/471893002/120001

[commit-bot: I haz the power, 2014-12-19 05:17:28]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187506