Replace Timers used in ActiveDOMObject with AsyncMethodRunner

Replace Timers used in ActiveDOMObject with AsyncMethodRunner

AsyncMethodRunner is a wrapper of Timer with simplified interface
for running methods asynchronously. It's mainly for use in
ActiveDOMObject subclasses.

Where suspend() and resume() are not implemented assuming other code
suspends timers, implement them to ensure they work even when those
external code doesn't suspend timers correctly.

BUG=none

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

[Kunihiko Sakamoto, 2013-10-17 09:43:41]

lgtm

[tyoshino (SeeGerritForStatus), 2013-10-17 10:32:45]

+abarth for OWNER review.

[abarth-chromium, 2013-10-17 19:37:48]

What problem does this solve?  There's a ton of code that doesn't worry about
this problem.  Is there a particular reason to fix it for this code and not
other code?

[abarth-chromium, 2013-10-17 19:38:09]

If we're going to fix this issue, we should write a test that demonstrates the
fix.

[tyoshino (SeeGerritForStatus), 2013-10-18 14:23:33]

I'm checking if all ActiveDOMObject subclasses are implemented correctly. As
XMLHttpRequestProgressEventThrottle::suspend() does this, I thought FontFaceSet
also needs it and ported it. We don't have a bug entry to fix for this.

But for example, weird thing may happen when inspector is used.

Suppose we have this line:
  document.fonts.ready().then(...)
If we set a breakpoint next to this line, the script is paused at that line, but
since Timer is not stopped, it may get fired during inspector's pause is active.
This is hard to observe since the timers are set with 0sec timeout.

The other use case of suspendActiveDOMObjects() is alert(), prompt(), etc. Since
they're pumping events (see content::RenderThreadImpl::Send()), the message loop
will be run while the thread is waiting for the result of a sync IPCs to show
those dialogs. But this is ok since
WebKitPlatformSupportImpl::SuspendSharedTimer() is called in
RenderThreadImpl::Send() before running IPC. It suspends the ThreadTimer, so
FontFaceSet's Timer won't be fired.

See also my comment on EventSource::stop(). Some ActiveDOMObject subclasses can
be considered to be working by accident. Even without real problem, it may be a
good habit to implement suspend()/resume(), I thought.

Writing a test for this should be a bit hard. If you think it's ok to leave this
as-is, let me just put comment like EventSource::stop() explaining why we don't
need suspend()/resume().

[tyoshino (SeeGerritForStatus), 2013-10-18 14:28:43]

On 2013/10/18 14:23:33, tyoshino wrote:
> Since
> they're pumping events (see content::RenderThreadImpl::Send()), the message
loop
> will be run while the thread is waiting for the result of a sync IPCs to show
> those dialogs.

Sorry. This sentence was confusing. Correction:

Though they use a sync IPC to show those dialogs and so it looks that the Timer
running on the same thread shouldn't get fired, the message loop will be run
while the thread is waiting for the result of the sync IPC (see
content::RenderThreadImpl::Send()).

[abarth-chromium, 2013-10-18 16:23:41]

Rather than trying to point-fix all of these issues (there are a lot of them!),
is there something more systematic we can do?  Can we make the right way of
handling this issue the easy way so that new code uses it as well?

[tyoshino (SeeGerritForStatus), 2013-10-18 16:45:32]

I'll try to figure out.

Looks like we should start with refactoring PageGroupLoadDeferrer. What
suspend()/resume() are expected to do should be defined more clearly.

[tyoshino (SeeGerritForStatus), 2013-10-22 14:42:47]

Adam, please take a look again. Introducing AsynMethodRunner should simplify
async method running and also makes it easier for them to implement suspend() /
resume().

[abarth-chromium, 2013-10-22 18:46:51]

LGTM

Thanks for pulling this out into a separate class.

https://codereview.chromium.org/27571005/diff/78001/Source/modules/notificati...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/27571005/diff/78001/Source/modules/notificati...
Source/modules/notifications/Notification.cpp:1: 
supurious?

https://codereview.chromium.org/27571005/diff/78001/Source/platform/AsyncMeth...
File Source/platform/AsyncMethodRunner.h (right):

https://codereview.chromium.org/27571005/diff/78001/Source/platform/AsyncMeth...
Source/platform/AsyncMethodRunner.h:42: class PLATFORM_EXPORT AsyncMethodRunner
{
No need for PLATFORM_EXPORT.  The class is entirely inline in the header.

[tyoshino (SeeGerritForStatus), 2013-10-23 06:02:29]

https://codereview.chromium.org/27571005/diff/78001/Source/modules/notificati...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/27571005/diff/78001/Source/modules/notificati...
Source/modules/notifications/Notification.cpp:1: 
On 2013/10/22 18:46:51, abarth wrote:
> supurious?

oh, mistake. removed

https://codereview.chromium.org/27571005/diff/78001/Source/platform/AsyncMeth...
File Source/platform/AsyncMethodRunner.h (right):

https://codereview.chromium.org/27571005/diff/78001/Source/platform/AsyncMeth...
Source/platform/AsyncMethodRunner.h:42: class PLATFORM_EXPORT AsyncMethodRunner
{
On 2013/10/22 18:46:51, abarth wrote:
> No need for PLATFORM_EXPORT.  The class is entirely inline in the header.

got it. removed

[commit-bot: I haz the power, 2013-10-23 06:03:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/27571005/168001

[commit-bot: I haz the power, 2013-10-23 07:45:56]

Change committed as 160316