Migrate touch events to EventHandlerRegistry

Migrate touch events to EventHandlerRegistry

Remove the specialized code for tracking touch events in favor of the
generic EventHandlerRegistry. Covered by existing tests.

See https://codereview.chromium.org/206603002/ for full review history.

BUG=347366
TEST=fast/events/touch*

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

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

[Rick Byers, 2014-04-11 18:32:25]

I assume this is also the same as what I already reviewed in the mega patch, so
LGTM

[Sami, 2014-06-18 15:36:18]

Finally got around to reworking this. In short:

- Redid migration of EventTargets between FrameHosts to properly move handlers
that do not appear in EventTarget.getEventListeners() (i.e., the made-up
listeners).
- We can't verify the consistency of the registry in the middle of migration, so
we now only do it when someone queries the registry.
- Updated test expectations to match the fact that the registry is no longer
per-Document.

PTAL, thanks!

[abarth-chromium, 2014-06-18 23:43:06]

lgtm

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2266: if (document().frameHost() &&
node() && !node()->isTextNode() && (oldTouchAction == TouchActionAuto) !=
(newStyle.touchAction() == TouchActionAuto)) {
The document will always have a frameHost here.

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2788: if (!documentBeingDestroyed() &&
document().frameHost() && node() && !node()->isTextNode() && m_style &&
m_style->touchAction() != TouchActionAuto)
Same here

[Rick Byers, 2014-06-19 17:10:36]

Very nice, thank you!

https://codereview.chromium.org/225903009/diff/140001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/touch-action-touch-handlers.html (right):

https://codereview.chromium.org/225903009/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/touch-action-touch-handlers.html:101:
shouldBe("getTouchHandlerCount(nestedDocument)", "2");
why this change?  I'd expect 1.

https://codereview.chromium.org/225903009/diff/140001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:1942: if (oldDocument.frameHost() !=
document().frameHost())
Since the boundary of interest has moved from frames to FrameHosts, I guess we
should really be adding at least one basic test that hits code like this, right?
 All the redundant checks in the existing tests for the counts in different
frames have become pretty silly now (since which frame you ask about is now
irrelevant, except for the framehost it's in).

https://codereview.chromium.org/225903009/diff/140001/Source/core/frame/Event...
File Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/frame/Event...
Source/core/frame/EventHandlerRegistry.cpp:130: if (newFrameHost &&
!oldFrameHost) {
this code reads like it should really be 3 functions (none of which permit null
arguments), not one function with 3 overloaded scenarios.

https://codereview.chromium.org/225903009/diff/140001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.cpp:827: if
(!node->inDocument())
I'd also check for null node here.  There are a LOT of non-Node EventTarget
types.  I don't think EventRegistry uses any today except the window case you're
handling above, but it's not unreasonable that it might in the future.

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2267: EventHandlerRegistry* registry =
&document().frameHost()->eventHandlerRegistry();
nit - use ref instead of taking the address just to dereference it below?

https://codereview.chromium.org/225903009/diff/140001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.cpp:515: EventHandlerRegistry* registry =
&m_element->document().frameHost()->eventHandlerRegistry();
nit again: ref instead of pointer?

[Sami, 2014-06-27 17:58:52]

Thanks for the comments -- all addressed.

https://codereview.chromium.org/225903009/diff/140001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/touch-action-touch-handlers.html (right):

https://codereview.chromium.org/225903009/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/touch-action-touch-handlers.html:101:
shouldBe("getTouchHandlerCount(nestedDocument)", "2");
On 2014/06/19 17:10:36, Rick Byers wrote:
> why this change?  I'd expect 1.

Both documents now point to the same (global) registry, so the counts are equal.
The first handler is the div.style.touchAction on the main document and the
second one is a similar one in the child document.

https://codereview.chromium.org/225903009/diff/140001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:1942: if (oldDocument.frameHost() !=
document().frameHost())
On 2014/06/19 17:10:36, Rick Byers wrote:
> Since the boundary of interest has moved from frames to FrameHosts, I guess we
> should really be adding at least one basic test that hits code like this,
right?

Yes, actually the previous CLs for scroll and mouse wheel handlers already added
a bunch of tests for this. I checked whether all three cases were being
exercised, and turns out we weren't checking the behavior of handlers in the
final case (just nodes in general) so I've added a test specifically for it.

>  All the redundant checks in the existing tests for the counts in different
> frames have become pretty silly now (since which frame you ask about is now
> irrelevant, except for the framehost it's in).

Yeah, those tests are a bit unnecessary now, but I figure keeping them around is
still good insurance.

https://codereview.chromium.org/225903009/diff/140001/Source/core/frame/Event...
File Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/frame/Event...
Source/core/frame/EventHandlerRegistry.cpp:130: if (newFrameHost &&
!oldFrameHost) {
On 2014/06/19 17:10:36, Rick Byers wrote:
> this code reads like it should really be 3 functions (none of which permit
null
> arguments), not one function with 3 overloaded scenarios. 

Agreed, let me clean this up.

https://codereview.chromium.org/225903009/diff/140001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.cpp:827: if
(!node->inDocument())
On 2014/06/19 17:10:36, Rick Byers wrote:
> I'd also check for null node here.  There are a LOT of non-Node EventTarget
> types.  I don't think EventRegistry uses any today except the window case
you're
> handling above, but it's not unreasonable that it might in the future.

Well spotted, I'll add a fail-safe.

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2266: if (document().frameHost() &&
node() && !node()->isTextNode() && (oldTouchAction == TouchActionAuto) !=
(newStyle.touchAction() == TouchActionAuto)) {
On 2014/06/18 23:43:06, abarth wrote:
> The document will always have a frameHost here.

Done.

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2267: EventHandlerRegistry* registry =
&document().frameHost()->eventHandlerRegistry();
On 2014/06/19 17:10:36, Rick Byers wrote:
> nit - use ref instead of taking the address just to dereference it below?

Done.

https://codereview.chromium.org/225903009/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2788: if (!documentBeingDestroyed() &&
document().frameHost() && node() && !node()->isTextNode() && m_style &&
m_style->touchAction() != TouchActionAuto)
On 2014/06/18 23:43:06, abarth wrote:
> Same here

Done.

https://codereview.chromium.org/225903009/diff/140001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.cpp:515: EventHandlerRegistry* registry =
&m_element->document().frameHost()->eventHandlerRegistry();
On 2014/06/19 17:10:36, Rick Byers wrote:
> nit again: ref instead of pointer?

Done.

[Rick Byers, 2014-06-27 21:36:51]

lgtm.

Note that I didn't re-review the entire patch.  It looks like your ps9 contains
both a lot of changes from trunk and your edits, so it's not easy to see what
exactly you changed.  But I looked at the places where you responded to my
comments, I assume there's no other substantive changes I should be looking at.

https://codereview.chromium.org/225903009/diff/140001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/touch-action-touch-handlers.html (right):

https://codereview.chromium.org/225903009/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/touch-action-touch-handlers.html:101:
shouldBe("getTouchHandlerCount(nestedDocument)", "2");
On 2014/06/27 17:58:51, Sami wrote:
> On 2014/06/19 17:10:36, Rick Byers wrote:
> > why this change?  I'd expect 1.
> 
> Both documents now point to the same (global) registry, so the counts are
equal.
> The first handler is the div.style.touchAction on the main document and the
> second one is a similar one in the child document.

Ah, of course - thanks.

https://codereview.chromium.org/225903009/diff/140001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:1942: if (oldDocument.frameHost() !=
document().frameHost())
On 2014/06/27 17:58:51, Sami wrote:
> On 2014/06/19 17:10:36, Rick Byers wrote:
> > Since the boundary of interest has moved from frames to FrameHosts, I guess
we
> > should really be adding at least one basic test that hits code like this,
> right?
> 
> Yes, actually the previous CLs for scroll and mouse wheel handlers already
added
> a bunch of tests for this. I checked whether all three cases were being
> exercised, and turns out we weren't checking the behavior of handlers in the
> final case (just nodes in general) so I've added a test specifically for it.
> 
> >  All the redundant checks in the existing tests for the counts in different
> > frames have become pretty silly now (since which frame you ask about is now
> > irrelevant, except for the framehost it's in).
> 
> Yeah, those tests are a bit unnecessary now, but I figure keeping them around
is
> still good insurance.

SGTM, thanks.

https://codereview.chromium.org/225903009/diff/140001/Source/core/frame/Event...
File Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/225903009/diff/140001/Source/core/frame/Event...
Source/core/frame/EventHandlerRegistry.cpp:130: if (newFrameHost &&
!oldFrameHost) {
On 2014/06/27 17:58:51, Sami wrote:
> On 2014/06/19 17:10:36, Rick Byers wrote:
> > this code reads like it should really be 3 functions (none of which permit
> null
> > arguments), not one function with 3 overloaded scenarios. 
> 
> Agreed, let me clean this up.

Looks better, thanks.

[Sami, 2014-06-30 11:07:40]

On 2014/06/27 21:36:51, Rick Byers wrote:
> lgtm.
> 
> Note that I didn't re-review the entire patch.  It looks like your ps9
contains
> both a lot of changes from trunk and your edits, so it's not easy to see what
> exactly you changed.  But I looked at the places where you responded to my
> comments, I assume there's no other substantive changes I should be looking
at.

Ah, my bad for not uploading the rebaseline separately. The only real changes
were in the commented parts.

[Sami, 2014-06-30 11:15:25]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-06-30 11:15:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/225903009/200001

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

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8902)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13556)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14558)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/23132)

[commit-bot: I haz the power, 2014-06-30 12:07:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-30 12:07:22]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/23147)

[Sami, 2014-06-30 12:58:39]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-06-30 12:59:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/225903009/220001

[commit-bot: I haz the power, 2014-06-30 14:09:20]

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

[commit-bot: I haz the power, 2014-06-30 15:14:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-30 15:14:05]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[Sami, 2014-07-01 17:33:21]

One of the bots found a bug here where HTMLInputElement was adding a custom
touch handler while the Document was being destroyed but *after* the registry
was notified about the Document going away. I fixed that by not registering that
handler if the Document was already stopped (and removed a tiny bit of dead
Oilpan code while I was at it).

[Rick Byers, 2014-07-01 18:21:33]

https://codereview.chromium.org/225903009/diff/260001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/225903009/diff/260001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:430: if (document().frameHost() &&
document().isActive()) {
Looks like this change will also skip updating the registry if the document is
still in the Uninitialized or Inactive state too.  Are you sure that's not
possible?  Maybe better to check explicitly for the shutdown states?

Also probably worth adding a comment saying the intention is to skip updating
the registry during destruction.

[Sami, 2014-07-01 18:32:52]

On 2014/07/01 18:21:33, Rick Byers wrote:
> Looks like this change will also skip updating the registry if the document is
> still in the Uninitialized or Inactive state too.  Are you sure that's not
> possible?  Maybe better to check explicitly for the shutdown states?
> 
> Also probably worth adding a comment saying the intention is to skip updating
> the registry during destruction.

Fair point, I can't say for sure we don't get here before the document becomes
active for the first time. I've changed this to check for
stopping/stopped/disposed instead and added a comment.

[Sami, 2014-07-09 18:34:30]

Bots look happy now. Rick, any objections? :)

[Rick Byers, 2014-07-09 18:52:37]

On 2014/07/09 18:34:30, Sami wrote:
> Bots look happy now. Rick, any objections? :)

Nope, still looks good to me - thanks!

[Sami, 2014-07-10 08:48:08]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-07-10 08:48:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/225903009/300001

[commit-bot: I haz the power, 2014-07-10 08:53:04]

Change committed as 177812

[Sami, 2014-08-13 14:55:07]

Now that the EventHandlerRegistry crashers have been eradicated by
https://codereview.chromium.org/392283002 and
https://codereview.chromium.org/415203003 (knock on wood), I'll reland this
patch. It's the same as before % a rebase.

[Sami, 2014-08-13 14:55:25]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-08-13 14:55:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/225903009/420001

[commit-bot: I haz the power, 2014-08-13 16:23:33]

Committed patchset #20 (420001) as 180191