Distinguish actions registered with setTimeout() and setInterval().

Distinguish actions registered with setTimeout() and setInterval().

This change makes actions created with setTimeout() and setInterval()
distinguishable, so actions created with setTimeout() cannot be removed by
clearInterval() (and vice versa).

BUG=230705
R=kbr@chromium.org,haraken@chromium.org

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

[Yuta Kitamura, 2013-07-17 03:43:16]

Hi, could you take a look?

haraken: Please look at bindings/
kbr: Please look at the other parts

[haraken, 2013-07-17 03:49:35]

The change looks good.

Do we already have a test that verifies if timers are correctly cleared?

https://codereview.chromium.org/19494002/diff/1/LayoutTests/fast/dom/Window/s...
File LayoutTests/fast/dom/Window/set-and-clear-Timeout-and-Interval.html
(right):

https://codereview.chromium.org/19494002/diff/1/LayoutTests/fast/dom/Window/s...
LayoutTests/fast/dom/Window/set-and-clear-Timeout-and-Interval.html:34:
window.clearTimeout(timeoutID);

What is this for?

[Yuta Kitamura, 2013-07-17 04:02:34]

https://codereview.chromium.org/19494002/diff/1/LayoutTests/fast/dom/Window/s...
File LayoutTests/fast/dom/Window/set-and-clear-Timeout-and-Interval.html
(right):

https://codereview.chromium.org/19494002/diff/1/LayoutTests/fast/dom/Window/s...
LayoutTests/fast/dom/Window/set-and-clear-Timeout-and-Interval.html:34:
window.clearTimeout(timeoutID);
On 2013/07/17 03:49:35, haraken wrote:
> What is this for?

Oops, I intended this to be clearInterval() to prevent
potential test flakiness that may be caused by having
the action uncleared.

Will fix this.

[Yuta Kitamura, 2013-07-17 04:11:13]

On 2013/07/17 03:49:35, haraken wrote:
> Do we already have a test that verifies if timers are correctly cleared?

For clearTimeout(), we have fast/dom/Window/clear-timeout.html.
However, there seems no test in fast/dom/Window/ that explicitly
tests the functionality of clearInterval(), so it seems a good
idea to add one for it. Will do.

[Yuta Kitamura, 2013-07-17 05:26:54]

haraken: PTAL

[haraken, 2013-07-17 05:31:40]

LGTM

[Yuta Kitamura, 2013-07-18 09:42:54]

kbr: Do you have any comments on this?

[Ken Russell (switch to Gerrit), 2013-07-18 19:05:12]

Sorry for the delay.

With the changes below, this LGTM.

https://codereview.chromium.org/19494002/diff/9001/LayoutTests/fast/dom/Windo...
File LayoutTests/fast/dom/Window/clear-interval.html (right):

https://codereview.chromium.org/19494002/diff/9001/LayoutTests/fast/dom/Windo...
LayoutTests/fast/dom/Window/clear-interval.html:16: var timeoutID;
Should this be named intervalID for clarity?

https://codereview.chromium.org/19494002/diff/9001/LayoutTests/fast/dom/Windo...
File LayoutTests/fast/dom/Window/set-and-clear-Timeout-and-Interval.html
(right):

https://codereview.chromium.org/19494002/diff/9001/LayoutTests/fast/dom/Windo...
LayoutTests/fast/dom/Window/set-and-clear-Timeout-and-Interval.html:30: var
timeoutID;
intervalID?

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer.cpp
File Source/core/page/DOMTimer.cpp (right):

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer....
Source/core/page/DOMTimer.cpp:82:
InspectorInstrumentation::didRemoveTimer(context, timeoutID ==
TimerTypeTimeout); // FIXME: Ditto.
Shouldn't didRemoveTimer only be called if
ScriptExecutionContext::removeTimeoutByIDIfTypeMatches actually did work? Since
that method returns void, it's impossible to know. I think it should return a
bool.

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer.h
File Source/core/page/DOMTimer.h (right):

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer....
Source/core/page/DOMTimer.h:45: TimerTypeInterval
This seems verbose; since it's scoped in DOMTimer, can it not just be enum Type,
with TimeoutType and IntervalType members?

[Yuta Kitamura, 2013-07-19 05:14:07]

kbr: I addressed most of your comments, except for
InspectorInstrumentation::didRemoveTimer one.
Could you take another look at that part, to make
sure you are okay with my resolution?

https://codereview.chromium.org/19494002/diff/9001/LayoutTests/fast/dom/Windo...
File LayoutTests/fast/dom/Window/clear-interval.html (right):

https://codereview.chromium.org/19494002/diff/9001/LayoutTests/fast/dom/Windo...
LayoutTests/fast/dom/Window/clear-interval.html:16: var timeoutID;
On 2013/07/18 19:05:13, Ken Russell wrote:
> Should this be named intervalID for clarity?

Done.

https://codereview.chromium.org/19494002/diff/9001/LayoutTests/fast/dom/Windo...
File LayoutTests/fast/dom/Window/set-and-clear-Timeout-and-Interval.html
(right):

https://codereview.chromium.org/19494002/diff/9001/LayoutTests/fast/dom/Windo...
LayoutTests/fast/dom/Window/set-and-clear-Timeout-and-Interval.html:30: var
timeoutID;
On 2013/07/18 19:05:13, Ken Russell wrote:
> intervalID?

Done.

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer.cpp
File Source/core/page/DOMTimer.cpp (right):

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer....
Source/core/page/DOMTimer.cpp:82:
InspectorInstrumentation::didRemoveTimer(context, timeoutID ==
TimerTypeTimeout); // FIXME: Ditto.
On 2013/07/18 19:05:13, Ken Russell wrote:
> Shouldn't didRemoveTimer only be called if
> ScriptExecutionContext::removeTimeoutByIDIfTypeMatches actually did work?
Since
> that method returns void, it's impossible to know. I think it should return a
> bool.

Sounds like a good idea, but when I tried locally,
an inspector test started to fail for unknown reason.
That said, your suggestion actually changes the
behavior of inspector, and it would be better to
handle it in a separate CL to have the change
reviewed by inspector folks. Does this make sense
to you?

I changed ScriptExecutionContext::removeTimeoutByIDIfTypeMatches()
to return a bool, and added a FIXME comment
describing your suggestion.

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer.h
File Source/core/page/DOMTimer.h (right):

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer....
Source/core/page/DOMTimer.h:45: TimerTypeInterval
On 2013/07/18 19:05:13, Ken Russell wrote:
> This seems verbose; since it's scoped in DOMTimer, can it not just be enum
Type,
> with TimeoutType and IntervalType members?

Done.

[Ken Russell (switch to Gerrit), 2013-07-19 20:28:36]

LGTM again

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer.cpp
File Source/core/page/DOMTimer.cpp (right):

https://codereview.chromium.org/19494002/diff/9001/Source/core/page/DOMTimer....
Source/core/page/DOMTimer.cpp:82:
InspectorInstrumentation::didRemoveTimer(context, timeoutID ==
TimerTypeTimeout); // FIXME: Ditto.
On 2013/07/19 05:14:08, Yuta Kitamura wrote:
> On 2013/07/18 19:05:13, Ken Russell wrote:
> > Shouldn't didRemoveTimer only be called if
> > ScriptExecutionContext::removeTimeoutByIDIfTypeMatches actually did work?
> Since
> > that method returns void, it's impossible to know. I think it should return
a
> > bool.
> 
> Sounds like a good idea, but when I tried locally,
> an inspector test started to fail for unknown reason.
> That said, your suggestion actually changes the
> behavior of inspector, and it would be better to
> handle it in a separate CL to have the change
> reviewed by inspector folks. Does this make sense
> to you?

Yes, thanks for trying to make the change. Sounds OK.

> I changed ScriptExecutionContext::removeTimeoutByIDIfTypeMatches()
> to return a bool, and added a FIXME comment
> describing your suggestion.

OK.

[commit-bot: I haz the power, 2013-07-22 02:31:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yutak@chromium.org/19494002/43001

[commit-bot: I haz the power, 2013-07-22 03:39:52]

Change committed as 154624

[adamk, 2013-07-26 19:59:05]

Heads-up: this was reverted in
https://src.chromium.org/viewvc/blink?revision=155001&view=revision on haraken's
recommendation that it's likely the cause of
https://code.google.com/p/chromium/issues/detail?id=263503