Make Gesture Tap be aware of Shadow DOM.

Make Gesture Tap be aware of Shadow DOM.

There are several places where we should use NodeRenderingTraversal::parent rather than plain node->parentNode() in WebViewImpl.
Unless that, we can't handle the case where a distributed node is tapped.

This patch also fixed the crash which a newly added layout test, gesture-tapHighlight-shadow-tree.html, has revealed.

In LinkHighlight::computeQuads, we can't use node->childNode() to retrieve a child render object here
because a render tree is constructed from a composed tree and we should retrieve a render object of a distributed node here.

BUG=399522
TEST=gesture-tapHighlight-shadow-tree.html

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

[hayato, 2014-08-06 08:05:36]

PTAL

[dglazkov, 2014-08-06 15:30:10]

lgtm

[esprehn, 2014-08-06 17:05:44]

What is updating the distribution before you call ::parent() ?

https://codereview.chromium.org/448593002/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/448593002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1197: bestTouchNode =
NodeRenderingTraversal::parent(bestTouchNode);
What is updating the distribution before you call this in each of the places you
added this in this file?

https://codereview.chromium.org/448593002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1209: while ((parentNode =
NodeRenderingTraversal::parent(bestTouchNode)) && invokesHandCursor(parentNode,
toLocalFrame(m_page->mainFrame())))
This would be nicer as a for loop and a break statement. the assignment inside
the while() is pretty confusing.

[hayato, 2014-08-07 02:44:55]

Thank you for the reviews.

https://codereview.chromium.org/448593002/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/448593002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1197: bestTouchNode =
NodeRenderingTraversal::parent(bestTouchNode);
On 2014/08/06 17:05:44, esprehn wrote:
> What is updating the distribution before you call this in each of the places
you
> added this in this file?

I assumed distributions would be updated at the time of Hit tests.
I would make the HitTest have a responsibility of updating the distribution in
the next patch.

I think there is no side effect which affects distribution in this member
function.

I am feeling that we should have a well-established practice to make sure
distribution is surely updated before calling any functions which needs the
result of distributions.

[hayato, 2014-08-07 03:09:49]

I am now taking a look at HitTest. I can't find any HitTest which will try to
update distribution.

I encountered the fundamental question again here: "Who should have the
responsibility of updating distribution?"

I think the situation is not so simple, so we should call
updateDistributionIfNeeded() in every places basically before calling
NodeRenderingTraversal::parent or something because we can't tell exactly which
code affects the distribution result.

I think we should improve this situation somehow. Do you have any idea about
this?

[hayato, 2014-08-07 03:52:02]

On 2014/08/07 03:09:49, hayato wrote:
> I am now taking a look at HitTest. I can't find any HitTest which will try to
> update distribution.
> 
> I encountered the fundamental question again here: "Who should have the
> responsibility of updating distribution?"
> 
> I think the situation is not so simple, so we should call
> updateDistributionIfNeeded() in every places basically before calling
> NodeRenderingTraversal::parent or something because we can't tell exactly
which
> code affects the distribution result.
> 
> I think we should improve this situation somehow. Do you have any idea about
> this?

FYI. I am adding an assertion in the following CL to know how far we are to
enable this assertion.
https://codereview.chromium.org/448893003/

[hayato, 2014-08-15 10:46:43]

I've rebased this patch on ToT.

Because I've landed https://codereview.chromium.org/448893003/, let me land this
patch after I confirm trybots won't complain.

[hayato, 2014-08-20 10:48:08]

It looks failures on win_blink_rel are irreverent with this patch.

Let me land this patch.

[hayato, 2014-08-20 10:48:12]

The CQ bit was checked by hayato@chromium.org

[commit-bot: I haz the power, 2014-08-20 10:48:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hayato@chromium.org/448593002/80001

[commit-bot: I haz the power, 2014-08-20 11:29:00]

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

[commit-bot: I haz the power, 2014-08-20 12:06:14]

Committed patchset #5 (80001) as 180643