Initial touch-action main thread implementation

Initial touch-action main thread implementation

This adds initial support for touch-action: none/auto on the blink main thread.
This works by computing the effective touch action on any touchstart event and
reporting the result to the embedder.  Chrome will then do appropriate filtering
of gesture events in the browser process.  This is necessary because the browser may do other optimizations based on the result (such as suppressing sending of
touchmove events during a scroll).  We report this via WebWidgetClient in advance of the
touch event ACK because in the future it will need to be able to come from the impl thread without waiting on main (plus there can be some performance benefit to doing it in advance
of running any JS handlers).

For more details see the design document:
https://docs.google.com/a/chromium.org/document/d/1CV2AXyrdPdGSRypAQcfGrgQVuWYi50EzTmVsMLWgRPM/edit#

Note that this does not yet integrate at all with compositor fast paths (eg.
a touch handler is necessary for touch-action to take effect in order to
bypass compositor touch hit testing).

Intent-to-implement thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/mDSwpxfdrdA

Chromium-side patch to consume this: https://codereview.chromium.org/67383002/

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

[Rick Byers, 2013-11-15 16:22:47]

Adam,
Can you review this patch which adds a new blink API for communicating
touch-action to the embedder?

There will be a handful of follow-on patches to this.  Feel free to suggest an
alternate reviewer if you prefer.  Most of the blink code for touch-action will
be around the hit testing stuff, which I'll ask leviw to review.

Thanks,
   Rick

[abarth-chromium, 2013-11-18 21:16:58]

@esprehn: Would you be willing to review this CL as well?  I'm not sure about
the interaction between EventHandler.cpp and the render tree.

https://codereview.chromium.org/16507017/diff/113001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/16507017/diff/113001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3639: }
No need for { }

https://codereview.chromium.org/16507017/diff/113001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3870: if (renderer) {
You can combine this line with the previous one.

https://codereview.chromium.org/16507017/diff/113001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3879: break;
I'm not sure if poking the render tree in this way is legit.  We'll need to get
help from some render tree experts.

https://codereview.chromium.org/16507017/diff/113001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/16507017/diff/113001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:970: client->setTouchAction(touchId,
webTouchAction);
Usually we just use a static_cast and COMPILE_ASSERT that the enum values match.

https://codereview.chromium.org/16507017/diff/113001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/16507017/diff/113001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:134: void runTouchActionTest(std::string
file)
This looks pretty involved.  Would it be better to test this with a LayoutTest?

https://codereview.chromium.org/16507017/diff/113001/public/web/WebWidgetClie...
File public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/16507017/diff/113001/public/web/WebWidgetClie...
public/web/WebWidgetClient.h:181: virtual void setTouchAction(int touchId,
TouchAction touchAction) { }
Why a touchId?  Is there some sort of more structured object we could use
instead?  Perhaps WebTouchPoint?

Also, the TouchAction enum should probably be declared in the blink namespace,
likely in a new header.

[Rick Byers, 2013-11-19 21:24:21]

Thanks Adam.

https://codereview.chromium.org/16507017/diff/113001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/16507017/diff/113001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3639: }
On 2013/11/18 21:16:59, abarth wrote:
> No need for { }

Done.

https://codereview.chromium.org/16507017/diff/113001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3870: if (renderer) {
On 2013/11/18 21:16:59, abarth wrote:
> You can combine this line with the previous one.

Done.

https://codereview.chromium.org/16507017/diff/113001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3879: break;
On 2013/11/18 21:16:59, abarth wrote:
> I'm not sure if poking the render tree in this way is legit.  We'll need to
get
> help from some render tree experts.

I think this is similar to what scrollNode (here in EventHandler.cpp) does for
mouse wheel and GestureScrollUpdate events - finding the appropriate renderBox
in the render tree and accessing it's state (in that case by calling
RenderBox::calling into it to scroll.  But yes, Elliott, I appreciate any input
you have here.


I think I need to add some iframe test cases though - I might not be handling
them properly (eg. iframes in multi-touch scenarios is already quite
complicated).

https://codereview.chromium.org/16507017/diff/113001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/16507017/diff/113001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:970: client->setTouchAction(touchId,
webTouchAction);
On 2013/11/18 21:16:59, abarth wrote:
> Usually we just use a static_cast and COMPILE_ASSERT that the enum values
match.

Done.

https://codereview.chromium.org/16507017/diff/113001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/16507017/diff/113001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:134: void runTouchActionTest(std::string
file)
On 2013/11/18 21:16:59, abarth wrote:
> This looks pretty involved.  Would it be better to test this with a
LayoutTest?

Yeah that was my first thought as well.  I eventually decided I should be using
a unit test so that I can properly test right up to the WebViewClient calls,
including for future multi-point scenarios (where there will be more than one
setTouchAction call for a single touch event).

Doing this as a layout test means (I think), adding an Internals API that lets
me hook the setTouchAction calls somehow, eg. one of:
 - inject a WebViewClient wrapper objects which logs setTouchAction calls and
makes all their details available to Internals
 - enable dispatch of a custom DOM event for each setTouchAction call
 - enable logging of setTouchAction calls in EventHandler (or existing
WebViewClient goo) and report them to the test

I did a quick search and couldn't find precedent for doing anything like any of
the above options (except that in trivial cases we do store some state just for
internals API to read).  What do you suggest?  In retrospect I should have asked
about this before starting - it took me a couple days to get this test working
properly as a unit test :-(

https://codereview.chromium.org/16507017/diff/113001/public/web/WebWidgetClie...
File public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/16507017/diff/113001/public/web/WebWidgetClie...
public/web/WebWidgetClient.h:181: virtual void setTouchAction(int touchId,
TouchAction touchAction) { }
On 2013/11/18 21:16:59, abarth wrote:
> Why a touchId?  Is there some sort of more structured object we could use
> instead?  Perhaps WebTouchPoint?

The touchid is the minimal data the embedder needs to correlate this call with a
particular finger.  The browser keeps a copy of all the outstanding
WebTouchPoint data (since we wouldn't want to trust what we get from the
renderer), so we just need an index into that data.  I think recreating a whole
WebTouchPoint from the pieces we have in WebCore would be a little silly since
we'd never want to trust any of that data in the browser.

But it turns out I'm not currently using this in chromium.  I had thought I
would, but we just agreed in the PE WG that the behavior here is sadly out of
scope of the spec (for IP reasons), so it's going to take a while longer to
decide how exactly we should handle it in Chrome.  For now I'm using a very
simple multi-point algorithm that doesn't need to correlate fingers.  

So how about I just omit this entirely from the API for now and worry about the
right encoding later if/when we decide we want a richer multi-pointer behavior?

> Also, the TouchAction enum should probably be declared in the blink namespace,
> likely in a new header.

Done.

[abarth-chromium, 2013-11-20 05:23:53]

On 2013/11/19 21:24:21, Rick Byers wrote:
> So how about I just omit this entirely from the API for now and worry about
the
> right encoding later if/when we decide we want a richer multi-pointer
behavior?

Sounds like a good idea.

[Rick Byers, 2013-11-20 15:14:54]

On 2013/11/20 05:23:53, abarth wrote:
> On 2013/11/19 21:24:21, Rick Byers wrote:
> > So how about I just omit this entirely from the API for now and worry about
> the
> > right encoding later if/when we decide we want a richer multi-pointer
> behavior?
> 
> Sounds like a good idea.

Ok, I will do that ASAP.

What about the layout test vs. unit test issue?  Should I stick with the unit
test for now and keep in mind the possibility of moving to a layout test?  Eg.
if I run into more layout issues in the future (where content shell behaves
differently enough from the unit test that it makes it tricky to understand
unexpected hit test results) then I'll almost certainly want to figure out some
way to move this to a layout test.

[abarth-chromium, 2013-11-20 17:37:36]

On 2013/11/20 15:14:54, Rick Byers wrote:
> What about the layout test vs. unit test issue?  Should I stick with the unit
> test for now and keep in mind the possibility of moving to a layout test?  Eg.
> if I run into more layout issues in the future (where content shell behaves
> differently enough from the unit test that it makes it tricky to understand
> unexpected hit test results) then I'll almost certainly want to figure out
some
> way to move this to a layout test.

The bigger thing to worry about here is to get Elliot to look at how you're
interacting with the render tree.  I'm happy to defer to his opinion about how
to test this CL as well.

[esprehn, 2013-11-21 16:26:28]

Your ancestor walk in the DOM doesn't work for Shadow DOM and can be simpler.
You either want parentOrShadowHostNode() or to use
NodeRenderingTraversal::parent() to deal with Shadow DOM. Which one depends on
what the spec says, but I'd assume NodeRenderingTraversal is probably the
correct one?

https://codereview.chromium.org/16507017/diff/363001/Source/core/loader/Empty...
File Source/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/16507017/diff/363001/Source/core/loader/Empty...
Source/core/loader/EmptyClients.h:165: virtual void setTouchAction(TouchAction
touchActions) OVERRIDE { };
This is confusingly named, it's all singular except the argument. Is it actions
or action?

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/ChromeC...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/ChromeC...
Source/core/page/ChromeClient.h:225: virtual void setTouchAction(TouchAction
touchActions) = 0;
Ditto

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3855: if (RenderObject* renderer =
node->renderer()) {
This loop checks for renderer() repeatedly (isBlockFlowElement, renderer here
and then below.). Instead you should just abort before the loop if there's no
renderer for the argument |node|. If you have a renderer than all your ancestors
do.

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3866: node = node->parentNode();
This doesn't appear correct, Shadow DOM means parentNode() can be null, and you
probably want to traverse up through projections?

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
Source/core/page/EventHandler.h:234: TouchAction
computeEffectiveTouchAction(Node*);
Node&, also const?

https://codereview.chromium.org/16507017/diff/363001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/16507017/diff/363001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:955: void
ChromeClientImpl::setTouchAction(TouchAction touchAction)
Now it's singular, seems the plurals in the interface files are a typo.

https://codereview.chromium.org/16507017/diff/363001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:958: if (client) {
if (WebViewClient* client = ...) {

}

or an early return.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:117: void runTouchActionTest(std::string
file)
It would be nicer to put this method outside the class def. Having it all
indented so far makes it harder to read.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:231: void sendTouchEvent(WebView* webView,
WebInputEvent::Type type, WebCore::IntPoint clientPoint)
Same.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-overflow.html (right):

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-overflow.html:4: <link rel="stylesheet"
type="text/css" href="touch-action-tests.css"/>
Leave off /

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-overflow.html:7: <body>
We usually leave off html, head and body unless you really need them (this test
doesn't).

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-overflow.html:13: <div class='scroll'
style='height:75px'>
nit: It's nice to be consistent with quoting, the start of the test uses double
quotes.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-simple.html (right):

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:4: <link rel="stylesheet"
type="text/css" href="touch-action-tests.css"/>
leave off /

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:7: <body>
same comments as the other test

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

https://codereview.chromium.org/16507017/diff/363001/public/web/WebTouchActio...
public/web/WebTouchAction.h:39: WebTouchActionAuto = 1
Is there a reason to specify the values? We usually just let the compiler assign
them. = 0 is enough for the first one (others will just be +1)

[Rick Byers, 2013-11-21 22:09:28]

Thanks Elliott!
I've fixed up the shadow-DOM cases and added some tests (which required a bit of
a change in the test infrastructure).  Look good now?

Rick

https://codereview.chromium.org/16507017/diff/363001/Source/core/loader/Empty...
File Source/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/16507017/diff/363001/Source/core/loader/Empty...
Source/core/loader/EmptyClients.h:165: virtual void setTouchAction(TouchAction
touchActions) OVERRIDE { };
On 2013/11/21 16:26:29, esprehn wrote:
> This is confusingly named, it's all singular except the argument. Is it
actions
> or action?

Yes, sorry - I switched from plural to singular but missed some.  It's a little
debatable which is better since it can be a collection of flags (although that's
not implemented here yet), but since the spec deals in singular I'll use that
consistently.  Thanks, Done.

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/ChromeC...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/ChromeC...
Source/core/page/ChromeClient.h:225: virtual void setTouchAction(TouchAction
touchActions) = 0;
On 2013/11/21 16:26:29, esprehn wrote:
> Ditto

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3855: if (RenderObject* renderer =
node->renderer()) {
On 2013/11/21 16:26:29, esprehn wrote:
> This loop checks for renderer() repeatedly (isBlockFlowElement, renderer here
> and then below.). Instead you should just abort before the loop if there's no
> renderer for the argument |node|. If you have a renderer than all your
ancestors
> do.

Thanks.  I could have sworn I've seen situations where a node had a renderer but
it's parent node didn't, but looking back at some examples it seems I was
probably wrong.  I've added a comment to Node::renderer() to document this
invariant.

However, I don't want to abort if the provided node has no renderer - I want to
walk up the DOM until I find one that does.  So how is this?  It still tests for
renderer inside the loop, but it's simpler to read than having one loop to walk
up until we find a renderer then another to keep walking up accessing renderers
(but not checking them for null).  Note that this is not a hot path here - it's
called only when a touch point goes down.

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3866: node = node->parentNode();
On 2013/11/21 16:26:29, esprehn wrote:
> This doesn't appear correct, Shadow DOM means parentNode() can be null, and
you
> probably want to traverse up through projections?

Yes, thank you!  I've fixed this to traverse up through projections, and I've
added a few shadow DOM test cases.

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
Source/core/page/EventHandler.h:234: TouchAction
computeEffectiveTouchAction(Node*);
On 2013/11/21 16:26:29, esprehn wrote:
> Node&, also const?

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/16507017/diff/363001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:958: if (client) {
On 2013/11/21 16:26:29, esprehn wrote:
> if (WebViewClient* client = ...) {
> 
> }
> 
> or an early return.

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
File Source/web/tests/TouchActionTest.cpp (right):

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:117: void runTouchActionTest(std::string
file)
On 2013/11/21 16:26:29, esprehn wrote:
> It would be nicer to put this method outside the class def. Having it all
> indented so far makes it harder to read.

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
Source/web/tests/TouchActionTest.cpp:231: void sendTouchEvent(WebView* webView,
WebInputEvent::Type type, WebCore::IntPoint clientPoint)
On 2013/11/21 16:26:29, esprehn wrote:
> Same.

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-overflow.html (right):

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-overflow.html:4: <link rel="stylesheet"
type="text/css" href="touch-action-tests.css"/>
On 2013/11/21 16:26:29, esprehn wrote:
> Leave off /

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-overflow.html:7: <body>
On 2013/11/21 16:26:29, esprehn wrote:
> We usually leave off html, head and body unless you really need them (this
test
> doesn't).

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-overflow.html:13: <div class='scroll'
style='height:75px'>
On 2013/11/21 16:26:29, esprehn wrote:
> nit: It's nice to be consistent with quoting, the start of the test uses
double
> quotes.

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-simple.html (right):

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:4: <link rel="stylesheet"
type="text/css" href="touch-action-tests.css"/>
On 2013/11/21 16:26:29, esprehn wrote:
> leave off /

Done.

https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-simple.html:7: <body>
On 2013/11/21 16:26:29, esprehn wrote:
> same comments as the other test

Done.

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

https://codereview.chromium.org/16507017/diff/363001/public/web/WebTouchActio...
public/web/WebTouchAction.h:39: WebTouchActionAuto = 1
On 2013/11/21 16:26:29, esprehn wrote:
> Is there a reason to specify the values? We usually just let the compiler
assign
> them. = 0 is enough for the first one (others will just be +1)

Note yet.  I did this because the next two values will be flags (1<<1 and 1<<2),
but we can worry about that in the next CL.  Remove the =1 for now.

[Rick Byers, 2013-12-05 16:20:36]

On 2013/11/21 22:09:28, Rick Byers wrote:
> Thanks Elliott!
> I've fixed up the shadow-DOM cases and added some tests (which required a bit
of
> a change in the test infrastructure).  Look good now?

Ping.  Elliott, you're back in the office now, right?  There's a couple CLs
blocked behind this one.

Thanks,
   Rick

> 
> Rick
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/loader/Empty...
> File Source/core/loader/EmptyClients.h (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/loader/Empty...
> Source/core/loader/EmptyClients.h:165: virtual void setTouchAction(TouchAction
> touchActions) OVERRIDE { };
> On 2013/11/21 16:26:29, esprehn wrote:
> > This is confusingly named, it's all singular except the argument. Is it
> actions
> > or action?
> 
> Yes, sorry - I switched from plural to singular but missed some.  It's a
little
> debatable which is better since it can be a collection of flags (although
that's
> not implemented here yet), but since the spec deals in singular I'll use that
> consistently.  Thanks, Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/page/ChromeC...
> File Source/core/page/ChromeClient.h (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/page/ChromeC...
> Source/core/page/ChromeClient.h:225: virtual void setTouchAction(TouchAction
> touchActions) = 0;
> On 2013/11/21 16:26:29, esprehn wrote:
> > Ditto
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:3855: if (RenderObject* renderer =
> node->renderer()) {
> On 2013/11/21 16:26:29, esprehn wrote:
> > This loop checks for renderer() repeatedly (isBlockFlowElement, renderer
here
> > and then below.). Instead you should just abort before the loop if there's
no
> > renderer for the argument |node|. If you have a renderer than all your
> ancestors
> > do.
> 
> Thanks.  I could have sworn I've seen situations where a node had a renderer
but
> it's parent node didn't, but looking back at some examples it seems I was
> probably wrong.  I've added a comment to Node::renderer() to document this
> invariant.
> 
> However, I don't want to abort if the provided node has no renderer - I want
to
> walk up the DOM until I find one that does.  So how is this?  It still tests
for
> renderer inside the loop, but it's simpler to read than having one loop to
walk
> up until we find a renderer then another to keep walking up accessing
renderers
> (but not checking them for null).  Note that this is not a hot path here -
it's
> called only when a touch point goes down.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:3866: node = node->parentNode();
> On 2013/11/21 16:26:29, esprehn wrote:
> > This doesn't appear correct, Shadow DOM means parentNode() can be null, and
> you
> > probably want to traverse up through projections?
> 
> Yes, thank you!  I've fixed this to traverse up through projections, and I've
> added a few shadow DOM test cases.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
> File Source/core/page/EventHandler.h (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/core/page/EventHa...
> Source/core/page/EventHandler.h:234: TouchAction
> computeEffectiveTouchAction(Node*);
> On 2013/11/21 16:26:29, esprehn wrote:
> > Node&, also const?
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/ChromeClientI...
> File Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/ChromeClientI...
> Source/web/ChromeClientImpl.cpp:958: if (client) {
> On 2013/11/21 16:26:29, esprehn wrote:
> > if (WebViewClient* client = ...) {
> > 
> > }
> > 
> > or an early return.
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
> File Source/web/tests/TouchActionTest.cpp (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
> Source/web/tests/TouchActionTest.cpp:117: void runTouchActionTest(std::string
> file)
> On 2013/11/21 16:26:29, esprehn wrote:
> > It would be nicer to put this method outside the class def. Having it all
> > indented so far makes it harder to read.
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/TouchAc...
> Source/web/tests/TouchActionTest.cpp:231: void sendTouchEvent(WebView*
webView,
> WebInputEvent::Type type, WebCore::IntPoint clientPoint)
> On 2013/11/21 16:26:29, esprehn wrote:
> > Same.
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
> File Source/web/tests/data/touch-action-overflow.html (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
> Source/web/tests/data/touch-action-overflow.html:4: <link rel="stylesheet"
> type="text/css" href="touch-action-tests.css"/>
> On 2013/11/21 16:26:29, esprehn wrote:
> > Leave off /
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
> Source/web/tests/data/touch-action-overflow.html:7: <body>
> On 2013/11/21 16:26:29, esprehn wrote:
> > We usually leave off html, head and body unless you really need them (this
> test
> > doesn't).
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
> Source/web/tests/data/touch-action-overflow.html:13: <div class='scroll'
> style='height:75px'>
> On 2013/11/21 16:26:29, esprehn wrote:
> > nit: It's nice to be consistent with quoting, the start of the test uses
> double
> > quotes.
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
> File Source/web/tests/data/touch-action-simple.html (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
> Source/web/tests/data/touch-action-simple.html:4: <link rel="stylesheet"
> type="text/css" href="touch-action-tests.css"/>
> On 2013/11/21 16:26:29, esprehn wrote:
> > leave off /
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/Source/web/tests/data/to...
> Source/web/tests/data/touch-action-simple.html:7: <body>
> On 2013/11/21 16:26:29, esprehn wrote:
> > same comments as the other test
> 
> Done.
> 
>
https://codereview.chromium.org/16507017/diff/363001/public/web/WebTouchAction.h
> File public/web/WebTouchAction.h (right):
> 
>
https://codereview.chromium.org/16507017/diff/363001/public/web/WebTouchActio...
> public/web/WebTouchAction.h:39: WebTouchActionAuto = 1
> On 2013/11/21 16:26:29, esprehn wrote:
> > Is there a reason to specify the values? We usually just let the compiler
> assign
> > them. = 0 is enough for the first one (others will just be +1)
> 
> Note yet.  I did this because the next two values will be flags (1<<1 and
1<<2),
> but we can worry about that in the next CL.  Remove the =1 for now.

[esprehn, 2013-12-06 22:19:19]

Please add an early return to the loop before landing.

lgtm

https://codereview.chromium.org/16507017/diff/663001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/16507017/diff/663001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3918: TouchAction effectiveTouchAction =
TouchActionAuto;
Remove this variable.

https://codereview.chromium.org/16507017/diff/663001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3925: if (action == TouchActionNone)
This should be:

if (renderer->style()->touchAction() == TouchActionNone)
  return TouchActionNone;

This loop doesn't early out, but it should.

https://codereview.chromium.org/16507017/diff/663001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3934: return effectiveTouchAction;
return TouchActionAuto;

https://codereview.chromium.org/16507017/diff/663001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-shadow-dom.html (right):

https://codereview.chromium.org/16507017/diff/663001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-shadow-dom.html:22: <div class='ta-none'
shadow-host></div>
nit: these tests switch from single to double quotes a lot.

https://codereview.chromium.org/16507017/diff/663001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-tests.js (right):

https://codereview.chromium.org/16507017/diff/663001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-tests.js:42: for( var i = 0; i <
tests.length; i++) {
nit: bad spacing, should be "for (var i"

[Rick Byers, 2013-12-07 03:13:35]

Thanks Elliott!

Adam - back to you for OWNERS in the remaining directories.

https://codereview.chromium.org/16507017/diff/663001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/16507017/diff/663001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3918: TouchAction effectiveTouchAction =
TouchActionAuto;
On 2013/12/06 22:19:20, esprehn wrote:
> Remove this variable.

Done.

I wrote it this way in anticipation of the other upcoming touch-action values,
to emphasize that the logic is progressively eliminating possible actions.  But
we can just add that in the next CL when we add pan-x and pan-y actions.  And
regardless, it should definitely early out once it reaches TouchActionNone.

https://codereview.chromium.org/16507017/diff/663001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3925: if (action == TouchActionNone)
On 2013/12/06 22:19:20, esprehn wrote:
> This should be:
> 
> if (renderer->style()->touchAction() == TouchActionNone)
>   return TouchActionNone;

Done

> This loop doesn't early out, but it should.

https://codereview.chromium.org/16507017/diff/663001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3934: return effectiveTouchAction;
On 2013/12/06 22:19:20, esprehn wrote:
> return TouchActionAuto;

Done.

https://codereview.chromium.org/16507017/diff/663001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-shadow-dom.html (right):

https://codereview.chromium.org/16507017/diff/663001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-shadow-dom.html:22: <div class='ta-none'
shadow-host></div>
On 2013/12/06 22:19:20, esprehn wrote:
> nit: these tests switch from single to double quotes a lot.

Dammit, I fixed this once then added new occurrences.  Sigh.  All fixed now,
thanks.

https://codereview.chromium.org/16507017/diff/663001/Source/web/tests/data/to...
File Source/web/tests/data/touch-action-tests.js (right):

https://codereview.chromium.org/16507017/diff/663001/Source/web/tests/data/to...
Source/web/tests/data/touch-action-tests.js:42: for( var i = 0; i <
tests.length; i++) {
On 2013/12/06 22:19:20, esprehn wrote:
> nit: bad spacing, should be "for (var i"

Done.

[abarth-chromium, 2013-12-07 16:13:22]

Source/web LGTM

[commit-bot: I haz the power, 2013-12-07 16:56:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/16507017/683001

[commit-bot: I haz the power, 2013-12-07 17:52:34]

Change committed as 163373