flex shorthand incorrectly accepting 1 0% 1 (i.e. flex-basis in the middle)

flex shorthand incorrectly accepting 1 0% 1 (i.e. flex-basis in the middle)

On flex items, specifying a flex shorthand with a flex basis in the
middle of two flex factors, e.g. flex: 1 0% 1 is getting accepted in
blink.

According to http://www.w3.org/TR/css3-flexbox/#flex-property, the
grammar of the flex shorthand is as follows:

Value:	none | [ <‘flex-grow’> <‘flex-shrink’>? || <‘flex-basis’> ]

It means that flex-grow and flex-shrink must appear together. Therefore flex
basis in the middle of two flex factors is invalid.

Currently blink consider flex: 1 0% 1 as valid, mapping the values as
 <flex-grow> <flex-basis> <flex-shrink>, i.e. flex: 1 0% 1 is equivalent
to flex: 1 1 0%.

This patch fixes this bug by adding an additional condition checking
if flex-basis is in the middle or not in flex argument list
while setting values for flex basis.

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

[harpreet.sk, 2014-07-16 16:42:14]

ptal....

[tony, 2014-07-16 17:12:38]

https://codereview.chromium.org/396263003/diff/20001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flex-shorthand-flex-basis-middle.html (right):

https://codereview.chromium.org/396263003/diff/20001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flex-shorthand-flex-basis-middle.html:6: background:
green;
Nit: Style in our test html files isn't that important, but I don't think we
ever use 8 space indents.

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

https://codereview.chromium.org/396263003/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:5733: } else if (!flexBasis &&
(arg->id == CSSValueAuto || (validUnit(arg, FLength | FPercent | FNonNeg) &&
!(flexGrow != unsetValue && flexShrink == unsetValue && args->size() == 3))))
I would make a helper function for this to help document what this check is. 
Something like isFlexBasisMiddleArg.

[harpreet.sk, 2014-07-17 08:03:25]

uploaded new patch... ptal...

https://codereview.chromium.org/396263003/diff/20001/LayoutTests/css3/flexbox...
File LayoutTests/css3/flexbox/flex-shorthand-flex-basis-middle.html (right):

https://codereview.chromium.org/396263003/diff/20001/LayoutTests/css3/flexbox...
LayoutTests/css3/flexbox/flex-shorthand-flex-basis-middle.html:6: background:
green;
On 2014/07/16 17:12:38, tony wrote:
> Nit: Style in our test html files isn't that important, but I don't think we
> ever use 8 space indents.

Done.

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

https://codereview.chromium.org/396263003/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:5733: } else if (!flexBasis &&
(arg->id == CSSValueAuto || (validUnit(arg, FLength | FPercent | FNonNeg) &&
!(flexGrow != unsetValue && flexShrink == unsetValue && args->size() == 3))))
On 2014/07/16 17:12:38, tony wrote:
> I would make a helper function for this to help document what this check is. 
> Something like isFlexBasisMiddleArg.

Done.

[tony, 2014-07-17 14:47:43]

LGTM

[harpreet.sk, 2014-07-17 15:04:46]

@leviw: Require owners lgtm

[harpreet.sk, 2014-07-18 03:47:25]

+tkent, eseidel - Requires owners LGTM

[leviw_travelin_and_unemployed, 2014-07-18 18:16:15]

Okay. LGTM.

[harpreet.sk, 2014-07-18 18:30:04]

The CQ bit was checked by harpreet.sk@samsung.com

[commit-bot: I haz the power, 2014-07-18 18:30:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/396263003/80001

[commit-bot: I haz the power, 2014-07-18 19:28:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-07-18 19:53:37]

Change committed as 178479