Only unregister touch action handler conditionally

Only unregister touch action handler conditionally

When a RenderObject with a non-default touch action is about to be
destroyed, we remove the fake event handler that implements the touch
action from the event handler registry. However the handler may have
already been removed previously by the host Document (e.g., from
Document::open()), so this code can cause the same handler to get
removed twice.

This patch fixes the issue by only removing the handler conditionally
if it is still there.

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

[Sami, 2014-10-09 18:37:54]

skyostil@chromium.org changed reviewers:
+ rbyers@chromium.org

[Sami, 2014-10-09 18:39:33]

Hey Rick, any opinion on this? This isn't ideal, but given that the fake handler
can get removed at any time without the RenderObject knowing, I'm not sure we
can do any better.

Could touch action be somehow tracked in the node hierarchy instead of on the
render tree?

[Rick Byers, 2014-10-09 19:29:34]

This seems OK to me.  Really the node should be adding the handler, but it's
based on style so it's a little tricky.  Removing it conditionally in this case
seems like the right pragmatic choice.  Ideally we'll get touch-action hit
testing on impl and then not need this fake handler at all.

Just a couple questions/concerns about the test.

https://codereview.chromium.org/647503002/diff/1/LayoutTests/fast/events/touc...
File LayoutTests/fast/events/touch/touch-action-double-remove.html (right):

https://codereview.chromium.org/647503002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/touch-action-double-remove.html:4: * {
does this really need to be * (that's rare) or is the <source> tag you're
targeting explicitly?  The more clear the test can be about what scenario
triggers the problem the better IMHO.

https://codereview.chromium.org/647503002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/touch-action-double-remove.html:9: onload =
function() {
please add a description() - i.e. what exactly is this doing to trigger a
premature touch-action removal?

https://codereview.chromium.org/647503002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/touch-action-double-remove.html:13: <source
onerror="foo()">
I thought <source> was only valid inside of <picture>, <audio> and <video>.  Is
an invalid <source> tag part of the repro steps somehow?

[Sami, 2014-10-15 11:06:24]

skyostil@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Sami, 2014-10-15 11:07:10]

Hey Dan, could you have a look while Rick is out? Rick seemed to be happy with
the fix but wanted some improvements to the test.

[dsinclair, 2014-10-15 12:50:49]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2014-10-15 12:50:49]

lgtm

[commit-bot: I haz the power, 2014-10-15 12:51:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/647503002/110001

[commit-bot: I haz the power, 2014-10-15 12:56:13]

Committed patchset #3 (id:110001) as 183743