add pan-x and pan-y support to CSS touch-action parsing.

add pan-x and pan-y support to CSS touch-action parsing.
Made TouchAction set of flags in renderstyleconstants.h as we need to support TouchAction: auto | none |[pan-x || pan-y].
Also in future may add more custom value similar to IE.

BUG=321643
TBR=rbyers@chromium.org,eseidel@chromium.org

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

[gnana, 2013-12-06 10:58:05]

Please take a look. ThankYou!

[Rick Byers, 2013-12-06 21:44:03]

Thanks!  This looks mostly good - just a bunch of small issues.

Also please update the CL description and title to be more precise.  Eg. the
summary should just be something like 'add pan-x and pan-y support to CSS
touch-action parsing'.  I'd also say something in the description about making
TouchAction a set of flags (since we may have more flags in the future - like
IE), modeled after TextDecoration.

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
File LayoutTests/fast/css/touch-action-parsing.html (right):

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:20: <div class="test defnone"
id="explicit-pan-x" style="touch-action: pan-x;" expected="pan-x"></div>
why use 'defnone' on these three tests?  I'd just remove it (like the
explicit-none below).  It's there to differentiate between 'auto' as an error
and 'auto' being explicitly applied.

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:22: <div class="test defnone"
id="explicit-pan-x-pan-y" style="touch-action: pan-x pan-y;" expected="pan-x
pan-y"></div>
it's probably worth adding 'pan-y pan-x'.  I'm guessing we'll flip the order
around, that's consistent with IE behavior (always lists pan-x before pan-y).

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:25: <div class="test"
id="explicit-invalid" style="touch-action: auto pan-x;" expected="auto"></div>
it's illegal to use the same ID more than once - please choose a unique name for
the test and the one below (invalid-2, invalid-3 etc. is fine, or something more
descriptive).

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:25: <div class="test"
id="explicit-invalid" style="touch-action: auto pan-x;" expected="auto"></div>
Apply the 'defnone' class here and expect it to be 'none' to make it clear that
the whole rule is being rejected, not just the 'pan-x' token.

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:26: <div class="test"
id="explicit-invalid" style="touch-action: none pan-y;" expected="auto"></div>
just for a bit more variety, I'd suggest 'pan-y none' instead of 'none pan-y'.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1545: static
PassRefPtr<CSSValue> touchActionFlagsToCSSValue(int touchAction)
This should take a TouchAction.   You could always add an operator& to
TouchAction (along with your operator|) if you like.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1552: if (touchAction &
TouchActionPanX)
We should probably ASSERT or otherwise reject the invalid combinations here (so
we're sure we never produce a value list that would fail to parse) - including
an empty list. 

Is there precedent for similar invalid value handling elsewhere?

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:2706: // auto | none | (panX || panY)
indent comment

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9016: void
CSSParser::addTouchActionProperty(CSSPropertyID propId, PassRefPtr<CSSValue>
value, bool important)
this looks like it's copied from a function dealing with shorthand properties
(like 'background' which is a shorthand for the combination of a bunch of
background-* properties).  You don't need any of this shorthand logic for
touch-action.  Instead of this function, I believe you can call addProperty
directly.

In fact you may just want to return the parsedValue and rely on the common
addProperty call at the end of parseValue (slightly simpler I think).

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9047:
list->append(cssValuePool().createIdentifierValue(value->id));
You should check that a given value never occurs more than once, and then add a
test for, eg. 'touch-action: pan-x pan-x' and verify that it fails to parse.  I
double checked that IE11 rejects this too.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
Source/core/css/CSSPrimitiveValueMappings.h:4875: case TouchActionPanX:
I think these two functions should go away.  They treat TouchAction as a
primitive value, but it's now a set of flags and so the TouchAction to CSSValue
mapping is more complex.

Instead you should update
StyleBuilderFunctions::applyValueCSSPropertyTouchAction to know the mapping (and
you already have the other direction in touchActionFlagsToCSSValue).

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1156: case CSSPropertyTouchAction:
Why is this here?  What does touch-action have to do with "cue style"?

https://codereview.chromium.org/87973002/diff/40001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.h:187: unsigned
m_touchAction : 3; // TouchAction
should probably define a TouchActionBits constant like TextDecorationBits

[gnana, 2013-12-10 18:24:36]

Thanks for the review comments
Please have a look at this.

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
File LayoutTests/fast/css/touch-action-parsing.html (right):

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:20: <div class="test defnone"
id="explicit-pan-x" style="touch-action: pan-x;" expected="pan-x"></div>
On 2013/12/06 21:44:04, Rick Byers wrote:
> why use 'defnone' on these three tests?  I'd just remove it (like the
> explicit-none below).  It's there to differentiate between 'auto' as an error
> and 'auto' being explicitly applied.

Done.

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:22: <div class="test defnone"
id="explicit-pan-x-pan-y" style="touch-action: pan-x pan-y;" expected="pan-x
pan-y"></div>
On 2013/12/06 21:44:04, Rick Byers wrote:
> it's probably worth adding 'pan-y pan-x'.  I'm guessing we'll flip the order
> around, that's consistent with IE behavior (always lists pan-x before pan-y).

Done.

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:25: <div class="test"
id="explicit-invalid" style="touch-action: auto pan-x;" expected="auto"></div>
On 2013/12/06 21:44:04, Rick Byers wrote:
> it's illegal to use the same ID more than once - please choose a unique name
for
> the test and the one below (invalid-2, invalid-3 etc. is fine, or something
more
> descriptive).

Done.

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:25: <div class="test"
id="explicit-invalid" style="touch-action: auto pan-x;" expected="auto"></div>
On 2013/12/06 21:44:04, Rick Byers wrote:
> it's illegal to use the same ID more than once - please choose a unique name
for
> the test and the one below (invalid-2, invalid-3 etc. is fine, or something
more
> descriptive).

Done.

https://codereview.chromium.org/87973002/diff/40001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:26: <div class="test"
id="explicit-invalid" style="touch-action: none pan-y;" expected="auto"></div>
On 2013/12/06 21:44:04, Rick Byers wrote:
> just for a bit more variety, I'd suggest 'pan-y none' instead of 'none pan-y'.

Done.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1545: static
PassRefPtr<CSSValue> touchActionFlagsToCSSValue(int touchAction)
On 2013/12/06 21:44:04, Rick Byers wrote:
> This should take a TouchAction.   You could always add an operator& to
> TouchAction (along with your operator|) if you like.

Done.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1552: if (touchAction &
TouchActionPanX)
On 2013/12/06 21:44:04, Rick Byers wrote:
> We should probably ASSERT or otherwise reject the invalid combinations here
(so
> we're sure we never produce a value list that would fail to parse) - including
> an empty list. 
> 
> Is there precedent for similar invalid value handling elsewhere? 

I m checking on this. i will update this in next patch.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:2706: // auto | none | (panX || panY)
On 2013/12/06 21:44:04, Rick Byers wrote:
> indent comment

Done.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9016: void
CSSParser::addTouchActionProperty(CSSPropertyID propId, PassRefPtr<CSSValue>
value, bool important)
On 2013/12/06 21:44:04, Rick Byers wrote:
> this looks like it's copied from a function dealing with shorthand properties
> (like 'background' which is a shorthand for the combination of a bunch of
> background-* properties).  You don't need any of this shorthand logic for
> touch-action.  Instead of this function, I believe you can call addProperty
> directly.
> 
> In fact you may just want to return the parsedValue and rely on the common
> addProperty call at the end of parseValue (slightly simpler I think).

Done.
Removed this function and called addProperty directly.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9047:
list->append(cssValuePool().createIdentifierValue(value->id));
On 2013/12/06 21:44:04, Rick Byers wrote:
> You should check that a given value never occurs more than once, and then add
a
> test for, eg. 'touch-action: pan-x pan-x' and verify that it fails to parse. 
I
> double checked that IE11 rejects this too.

Done.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
Source/core/css/CSSPrimitiveValueMappings.h:4875: case TouchActionPanX:
On 2013/12/06 21:44:04, Rick Byers wrote:
> I think these two functions should go away.  They treat TouchAction as a
> primitive value, but it's now a set of flags and so the TouchAction to
CSSValue
> mapping is more complex.
> 
> Instead you should update
> StyleBuilderFunctions::applyValueCSSPropertyTouchAction to know the mapping
(and
> you already have the other direction in touchActionFlagsToCSSValue).

Removing this two function caused build error. Working on it.
Will update in next patch.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1156: case CSSPropertyTouchAction:
On 2013/12/06 21:44:04, Rick Byers wrote:
> Why is this here?  What does touch-action have to do with "cue style"?

Done.

Removed.

https://codereview.chromium.org/87973002/diff/40001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.h:187: unsigned
m_touchAction : 3; // TouchAction
On 2013/12/06 21:44:04, Rick Byers wrote:
> should probably define a TouchActionBits constant like TextDecorationBits

Done.

[Rick Byers, 2013-12-10 21:57:14]

Thanks!  Almost there.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1552: if (touchAction &
TouchActionPanX)
On 2013/12/10 18:24:37, gnana wrote:
> On 2013/12/06 21:44:04, Rick Byers wrote:
> > We should probably ASSERT or otherwise reject the invalid combinations here
> (so
> > we're sure we never produce a value list that would fail to parse) -
including
> > an empty list. 
> > 
> > Is there precedent for similar invalid value handling elsewhere? 
> 
> I m checking on this. i will update this in next patch.

Ok.  Whatever the common error-handling pattern seems to be is fine with me - it
seems like it should be pretty unlikely for this to ever be an issue.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
Source/core/css/CSSPrimitiveValueMappings.h:4875: case TouchActionPanX:
On 2013/12/10 18:24:37, gnana wrote:
> On 2013/12/06 21:44:04, Rick Byers wrote:
> > I think these two functions should go away.  They treat TouchAction as a
> > primitive value, but it's now a set of flags and so the TouchAction to
> CSSValue
> > mapping is more complex.
> > 
> > Instead you should update
> > StyleBuilderFunctions::applyValueCSSPropertyTouchAction to know the mapping
> (and
> > you already have the other direction in touchActionFlagsToCSSValue).
> 
> Removing this two function caused build error. Working on it.
> Will update in next patch.

Ok.  It's possible I'm missing something here - let me know if there's something
that depends on this that's not trivial to remove.  Naively it seems that
anything that depends on this would be broken in flag combination cases like
'pan-x pan-y'

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:572: action |=
*toCSSPrimitiveValue(item);
The other indication that this should be calling your touch-action conversion
routines directly is that we should make it clear here that only the
touch-action values are permitted (eg. what would it mean if there was a length
value or something here?).  Hopefully this falls out cleanly from the removal of
the value conversion functions.

https://codereview.chromium.org/87973002/diff/80001/LayoutTests/fast/css/touc...
File LayoutTests/fast/css/touch-action-parsing.html (right):

https://codereview.chromium.org/87973002/diff/80001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:28: <div class="test"
id="explicit-invalid-4" style="touch-action: pan-x Pan-X;"
expected="auto"></div>
nit: to avoid any potential confusion, use a lower-case 'p' like everywhere
else.

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:2706: // auto | none | (panX || panY)
nit: gramar is ... | [pan-x || pan-y] 
not ... | (panX || panY)

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9036: if (list->length() &&
list->hasValue(cssValuePool().createIdentifierValue(value->id).get())) {
nit: Looks like you should be able to skip the call to length() - hasValue will
always return false for a 0-element list.

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9040:
list->append(cssValuePool().createIdentifierValue(value->id));
nit: call createIdentifier just once above and re-use the result in both places
rather than calling it twice.

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9050: // Values are either valid or in
shorthand scope.
nit: remove this comment - hold-over from the shorthand code that's not relevant
here.

[gnana, 2013-12-11 14:02:53]

Applied review comments. Please have a look at this.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1552: if (touchAction &
TouchActionPanX)
On 2013/12/10 21:57:14, Rick Byers wrote:
> On 2013/12/10 18:24:37, gnana wrote:
> > On 2013/12/06 21:44:04, Rick Byers wrote:
> > > We should probably ASSERT or otherwise reject the invalid combinations
here
> > (so
> > > we're sure we never produce a value list that would fail to parse) -
> including
> > > an empty list. 
> > > 
> > > Is there precedent for similar invalid value handling elsewhere? 
> > 
> > I m checking on this. i will update this in next patch.
> 
> Ok.  Whatever the common error-handling pattern seems to be is fine with me -
it
> seems like it should be pretty unlikely for this to ever be an issue.

Done.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
Source/core/css/CSSPrimitiveValueMappings.h:4875: case TouchActionPanX:
On 2013/12/10 21:57:14, Rick Byers wrote:
> On 2013/12/10 18:24:37, gnana wrote:
> > On 2013/12/06 21:44:04, Rick Byers wrote:
> > > I think these two functions should go away.  They treat TouchAction as a
> > > primitive value, but it's now a set of flags and so the TouchAction to
> > CSSValue
> > > mapping is more complex.
> > > 
> > > Instead you should update
> > > StyleBuilderFunctions::applyValueCSSPropertyTouchAction to know the
mapping
> > (and
> > > you already have the other direction in touchActionFlagsToCSSValue).
> > 
> > Removing this two function caused build error. Working on it.
> > Will update in next patch.
> 
> Ok.  It's possible I'm missing something here - let me know if there's
something
> that depends on this that's not trivial to remove.  Naively it seems that
> anything that depends on this would be broken in flag combination cases like
> 'pan-x pan-y'

Done.
I removed one function. But cannot remove the operator TouchAction()
Because in StyleBuilderFunctions::applyValueCSSPropertyTouchAction the arguement
is a cssvaluelist of primitive values. hence we need this function.

https://codereview.chromium.org/87973002/diff/80001/LayoutTests/fast/css/touc...
File LayoutTests/fast/css/touch-action-parsing.html (right):

https://codereview.chromium.org/87973002/diff/80001/LayoutTests/fast/css/touc...
LayoutTests/fast/css/touch-action-parsing.html:28: <div class="test"
id="explicit-invalid-4" style="touch-action: pan-x Pan-X;"
expected="auto"></div>
On 2013/12/10 21:57:14, Rick Byers wrote:
> nit: to avoid any potential confusion, use a lower-case 'p' like everywhere
> else.

Done.

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:2706: // auto | none | (panX || panY)
On 2013/12/10 21:57:14, Rick Byers wrote:
> nit: gramar is ... | [pan-x || pan-y] 
> not ... | (panX || panY)

Done.

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9036: if (list->length() &&
list->hasValue(cssValuePool().createIdentifierValue(value->id).get())) {
On 2013/12/10 21:57:14, Rick Byers wrote:
> nit: Looks like you should be able to skip the call to length() - hasValue
will
> always return false for a 0-element list.

Done.

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9040:
list->append(cssValuePool().createIdentifierValue(value->id));
On 2013/12/10 21:57:14, Rick Byers wrote:
> nit: call createIdentifier just once above and re-use the result in both
places
> rather than calling it twice.

Done.

https://codereview.chromium.org/87973002/diff/80001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:9050: // Values are either valid or in
shorthand scope.
On 2013/12/10 21:57:14, Rick Byers wrote:
> nit: remove this comment - hold-over from the shorthand code that's not
relevant
> here.

Done.

[Rick Byers, 2013-12-12 15:30:34]

Thanks.   A couple more tweaks.

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/87973002/diff/40001/Source/core/css/CSSPrimit...
Source/core/css/CSSPrimitiveValueMappings.h:4875: case TouchActionPanX:
On 2013/12/11 14:02:53, gnana wrote:
> On 2013/12/10 21:57:14, Rick Byers wrote:
> > On 2013/12/10 18:24:37, gnana wrote:
> > > On 2013/12/06 21:44:04, Rick Byers wrote:
> > > > I think these two functions should go away.  They treat TouchAction as a
> > > > primitive value, but it's now a set of flags and so the TouchAction to
> > > CSSValue
> > > > mapping is more complex.
> > > > 
> > > > Instead you should update
> > > > StyleBuilderFunctions::applyValueCSSPropertyTouchAction to know the
> mapping
> > > (and
> > > > you already have the other direction in touchActionFlagsToCSSValue).
> > > 
> > > Removing this two function caused build error. Working on it.
> > > Will update in next patch.
> > 
> > Ok.  It's possible I'm missing something here - let me know if there's
> something
> > that depends on this that's not trivial to remove.  Naively it seems that
> > anything that depends on this would be broken in flag combination cases like
> > 'pan-x pan-y'
> 
> Done.
> I removed one function. But cannot remove the operator TouchAction()
> Because in StyleBuilderFunctions::applyValueCSSPropertyTouchAction the
arguement
> is a cssvaluelist of primitive values. hence we need this function.

Yeah, I was thinking you'd want to essentially inline this logic into
applyValueCSSPropertyTouchAction.  But that's fine - there's nothing lossy with
going that direction, and I see it's consistent with how TextDecoration (also a
set of flags) works.  Thanks.

https://codereview.chromium.org/87973002/diff/100001/LayoutTests/fast/css/tou...
File LayoutTests/fast/css/touch-action-parsing.html (right):

https://codereview.chromium.org/87973002/diff/100001/LayoutTests/fast/css/tou...
LayoutTests/fast/css/touch-action-parsing.html:28: <div class="test"
id="explicit-invalid-4" style="touch-action: pan-x Pan-x;"
expected="auto"></div>
you fixed the case of 'x' (which I didn't notice), but not the case of 'P'.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1549: ASSERT(!(list->length()));
This is trivially true.

I think you can simplify these asserts by making them on the touchAction instead
of the list.  Eg. make this 'ASSERT(touchAction == touchActionNone))'.  Then I
think you can remove all the other ASSERTs - the only invalid combinations are
'None | PanX' and 'None | PanY', which will both be covered by the assert here.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1552: if (touchAction &
TouchActionAuto) {
The way you've defined it (which I think is reasonable) is that Auto isn't a
flag - it's the zero value.  So touchAction & TouchActionAuto is always false. 
So this should instead be 'if (touchAction == touchActionAuto)', and since it's
special perhaps put it first in the list of statements.  Sorry I missed this in
the previous review.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1565: if (!list->length())
When the above is fixed, you should be able to assert that the list is never
empty.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:2706: // auto | none | [panX || panY]
still need to correct 'panX' to 'pan-x' etc.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9036: RefPtr<CSSValue> panValue = 
cssValuePool().createIdentifierValue(value->id);
nit: remove one of the two spaces after '='

[gnana, 2013-12-13 11:10:53]

Applied review comments and merge to trunk. Hope its ready for owners review

https://codereview.chromium.org/87973002/diff/100001/LayoutTests/fast/css/tou...
File LayoutTests/fast/css/touch-action-parsing.html (right):

https://codereview.chromium.org/87973002/diff/100001/LayoutTests/fast/css/tou...
LayoutTests/fast/css/touch-action-parsing.html:28: <div class="test"
id="explicit-invalid-4" style="touch-action: pan-x Pan-x;"
expected="auto"></div>
On 2013/12/12 15:30:35, Rick Byers wrote:
> you fixed the case of 'x' (which I didn't notice), but not the case of 'P'.

Done.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1549: ASSERT(!(list->length()));
On 2013/12/12 15:30:35, Rick Byers wrote:
> This is trivially true.
> 
> I think you can simplify these asserts by making them on the touchAction
instead
> of the list.  Eg. make this 'ASSERT(touchAction == touchActionNone))'.  Then I
> think you can remove all the other ASSERTs - the only invalid combinations are
> 'None | PanX' and 'None | PanY', which will both be covered by the assert
here.

Done.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1552: if (touchAction &
TouchActionAuto) {
On 2013/12/12 15:30:35, Rick Byers wrote:
> The way you've defined it (which I think is reasonable) is that Auto isn't a
> flag - it's the zero value.  So touchAction & TouchActionAuto is always false.

> So this should instead be 'if (touchAction == touchActionAuto)', and since
it's
> special perhaps put it first in the list of statements.  Sorry I missed this
in
> the previous review.

Done.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1565: if (!list->length())
On 2013/12/12 15:30:35, Rick Byers wrote:
> When the above is fixed, you should be able to assert that the list is never
> empty.

Done.

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:2706: // auto | none | [panX || panY]
On 2013/12/12 15:30:35, Rick Byers wrote:
> still need to correct 'panX' to 'pan-x' etc.

Done.
:-)

https://codereview.chromium.org/87973002/diff/100001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9036: RefPtr<CSSValue> panValue = 
cssValuePool().createIdentifierValue(value->id);
On 2013/12/12 15:30:35, Rick Byers wrote:
> nit: remove one of the two spaces after '='

Done.

[Rick Byers, 2013-12-13 13:34:22]

LGTM with nit - thanks!

+eseidel for OWNERS

https://codereview.chromium.org/87973002/diff/120001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/120001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1569: return
cssValuePool().createIdentifierValue(CSSValueAuto);
nit: just remove this whole if block and just ASSERT(list->length()) - we'll
never get here, so it's not worth worrying about a fallback.

[vivekg, 2013-12-13 14:14:31]

https://codereview.chromium.org/87973002/diff/120001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/87973002/diff/120001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyleConstants.h:521: TouchActionNone =  0x1,
Same goes here as below comment about the enum.

https://codereview.chromium.org/87973002/diff/120001/public/web/WebTouchAction.h
File public/web/WebTouchAction.h (right):

https://codereview.chromium.org/87973002/diff/120001/public/web/WebTouchActio...
public/web/WebTouchAction.h:39: WebTouchActionNone =  0x1,
nit: Extra space here..

Also we are now assigning WebTouchActionNone = 1 which earlier was = 0. Will it
cause any issue elsewhere? 

The enum values like this are defined as below elsewhere in the blink source.
Should we also align to the same? 

enum WebTouchAction {
    WebTouchActionAuto = 1 << 0,
    WebTouchActionNone = 1 << 1,
    ...
}

[gnana, 2013-12-13 17:06:23]

Thanks Rick
Thanks vivek
Applied the review comments

https://codereview.chromium.org/87973002/diff/120001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/120001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1569: return
cssValuePool().createIdentifierValue(CSSValueAuto);
On 2013/12/13 13:34:23, Rick Byers wrote:
> nit: just remove this whole if block and just ASSERT(list->length()) - we'll
> never get here, so it's not worth worrying about a fallback.

Done.

https://codereview.chromium.org/87973002/diff/120001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/87973002/diff/120001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyleConstants.h:521: TouchActionNone =  0x1,
On 2013/12/13 14:14:31, vivekg_ wrote:
> Same goes here as below comment about the enum. 

Done.

https://codereview.chromium.org/87973002/diff/120001/public/web/WebTouchAction.h
File public/web/WebTouchAction.h (right):

https://codereview.chromium.org/87973002/diff/120001/public/web/WebTouchActio...
public/web/WebTouchAction.h:39: WebTouchActionNone =  0x1,
On 2013/12/13 14:14:31, vivekg_ wrote:
> nit: Extra space here..
> 
> Also we are now assigning WebTouchActionNone = 1 which earlier was = 0. Will
it
> cause any issue elsewhere? 
auto is a default value for any element for touch-action when not specified.
Thats the reason i changed it. It will not cause any issue.
> 
> The enum values like this are defined as below elsewhere in the blink source.
> Should we also align to the same? 
> 
> enum WebTouchAction {
>     WebTouchActionAuto = 1 << 0,
>     WebTouchActionNone = 1 << 1,
>     ...
> }
> 
in RenderStyleConstants.h there are other enum which are similar to this one. so
i think it aligns and should not be issue in blink.

Done.

[eseidel, 2013-12-13 17:19:06]

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1553: if (touchAction ==
TouchActionAuto) {
nit: most places in blink we drop the {} around single line blocks.

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9019: &&
!RuntimeEnabledFeatures::cssTouchActionEnabled())
CSSRuntimeFeatures has the ability to enable properties individually if that's
what you're going for.  WE don't have support for toggling values through
CSSRuntimeFeatures yet however.

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9024: if (m_valueList->size() == 1 && value &&
(value->id == CSSValueAuto || value->id == CSSValueNone)) {
I don't understand the size=1 case.  Is this an optimization?

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParser.h
File Source/core/css/CSSParser.h (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
Source/core/css/CSSParser.h:284: bool parseTouchAction(CSSPropertyID propId,
bool important);
nit: propId name doens't add anything, I'd remove it.

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderCustom.cpp:571: CSSValue* item = i.value();
nit: I'm not sure the local is adding anything.

https://codereview.chromium.org/87973002/diff/140001/Source/core/rendering/st...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/rendering/st...
Source/core/rendering/style/StyleRareNonInheritedData.h:188: unsigned
m_touchAction : TouchActionBits; // TouchAction
Is this object specially sized?  Do we need to update the count of free bits?

[eseidel, 2013-12-13 17:20:20]

Overall this looks fine to me.  It looks like its runtime guarded, which is
good.  I'm not super familiar with the CSSParser bits but what I see looks
right. My only question was lack of understanidng of the size=1 case.

[eseidel, 2013-12-13 17:34:36]

lgtm

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9024: if (m_valueList->size() == 1 && value &&
(value->id == CSSValueAuto || value->id == CSSValueNone)) {
On 2013/12/13 17:19:06, eseidel wrote:
> I don't understand the size=1 case.  Is this an optimization?

nevermind.  This is to distinguis touch-action: single-value from touch-action:
many values;

the former can only be auto or none, the later is x/y pan.

[Rick Byers, 2013-12-13 21:16:15]

Thanks Eric.

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9019: &&
!RuntimeEnabledFeatures::cssTouchActionEnabled())
On 2013/12/13 17:19:06, eseidel wrote:
> CSSRuntimeFeatures has the ability to enable properties individually if that's
> what you're going for.  WE don't have support for toggling values through
> CSSRuntimeFeatures yet however.

Oh cool, I hadn't noticed RuntimeCSSEnabled.cpp before (perhaps it was added
after I landed my initial touch-action parsing CL ages ago).  Since Gnana is
just moving my code around here, I suggest he land it as is, and I'll move this
over to the RuntimeCSSEnabled.cpp infrastructure when I land the CL that
switches from 'test' to 'experimental' mode.

https://codereview.chromium.org/87973002/diff/140001/Source/core/rendering/st...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/rendering/st...
Source/core/rendering/style/StyleRareNonInheritedData.h:188: unsigned
m_touchAction : TouchActionBits; // TouchAction
On 2013/12/13 17:19:06, eseidel wrote:
> Is this object specially sized?  Do we need to update the count of free bits?

No I think it's just packed as tightly as possible.  I didn't have to do
anything special when I first added m_touchAction as a single bit.

[gnana, 2013-12-16 08:16:25]

Thanks Eric.
Thanks Rick.
Applied review comments

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1553: if (touchAction ==
TouchActionAuto) {
On 2013/12/13 17:19:06, eseidel wrote:
> nit: most places in blink we drop the {} around single line blocks.

Done.

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParser.h
File Source/core/css/CSSParser.h (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/CSSParse...
Source/core/css/CSSParser.h:284: bool parseTouchAction(CSSPropertyID propId,
bool important);
On 2013/12/13 17:19:06, eseidel wrote:
> nit: propId name doens't add anything, I'd remove it.

Done.

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/87973002/diff/140001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderCustom.cpp:571: CSSValue* item = i.value();
On 2013/12/13 17:19:06, eseidel wrote:
> nit: I'm not sure the local is adding anything.

Done.

[commit-bot: I haz the power, 2013-12-16 08:27:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/87973002/160001

[commit-bot: I haz the power, 2013-12-16 09:29:12]

Change committed as 163948