ScreenOrientationController: task-based dispatching of change events.

ScreenOrientationController: task-based dispatching of change events.

Move ScreenOrientationController over to posting a task for dispatching
its async change events. That is, no longer use a one-shot timer.

R=mlamouri
BUG=

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

[sof, 2015-04-15 09:02:12]

sigbjornf@opera.com changed reviewers:
+ mlamouri@chromium.org

[sof, 2015-04-15 09:02:13]

Please take a look.

Motivated/spurred on by the scheduler team wanting to see less use of one-shot
main thread timers in Blink, shifting these over to tasks instead.

[alex clarke (OOO till 29th), 2015-04-15 09:27:32]

alexclarke@chromium.org changed reviewers:
+ alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-04-15 09:27:32]

https://codereview.chromium.org/1089523002/diff/1/Source/modules/screen_orien...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/1089523002/diff/1/Source/modules/screen_orien...
Source/modules/screen_orientation/ScreenOrientationController.cpp:142:
document->postTask(FROM_HERE,
createSameThreadTask(&ScreenOrientationController::dispatchChangeEvent, this));
FYI the blink scheduler team are thinking of adding a helper to make this kind
of refactor easier.

Something on the lines of helper.postUniqueTask(FROM_HERE,
createSameThreadTask(&ScreenOrientationController::dispatchChangeEvent, this));

The helper would contain the m_isDispatchingEvent logic (since this seems to be
a reasonably common pattern).

The helper's constructor would take the WebThread you wanted the task to run on.

When we implement that, we'll probably refactor this to use the new method.

[sof, 2015-04-15 09:42:08]

On 2015/04/15 09:27:32, alexclarke1 wrote:
>
https://codereview.chromium.org/1089523002/diff/1/Source/modules/screen_orien...
> File Source/modules/screen_orientation/ScreenOrientationController.cpp
(right):
> 
>
https://codereview.chromium.org/1089523002/diff/1/Source/modules/screen_orien...
> Source/modules/screen_orientation/ScreenOrientationController.cpp:142:
> document->postTask(FROM_HERE,
> createSameThreadTask(&ScreenOrientationController::dispatchChangeEvent,
this));
> FYI the blink scheduler team are thinking of adding a helper to make this kind
> of refactor easier.
> 
> Something on the lines of helper.postUniqueTask(FROM_HERE,
> createSameThreadTask(&ScreenOrientationController::dispatchChangeEvent,
this));
> 
> The helper would contain the m_isDispatchingEvent logic (since this seems to
be
> a reasonably common pattern).
> 
> The helper's constructor would take the WebThread you wanted the task to run
on.
> 
> When we implement that, we'll probably refactor this to use the new method.

Thanks, that's good to know. This could of course wait until that is ready, no
particular rush.

[mlamouri (slow - plz ping), 2015-04-16 10:44:25]

lgtm

[sof, 2015-04-17 06:57:14]

On 2015/04/15 09:42:08, sof wrote:
> On 2015/04/15 09:27:32, alexclarke1 wrote:
> >
>
https://codereview.chromium.org/1089523002/diff/1/Source/modules/screen_orien...
> > File Source/modules/screen_orientation/ScreenOrientationController.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1089523002/diff/1/Source/modules/screen_orien...
> > Source/modules/screen_orientation/ScreenOrientationController.cpp:142:
> > document->postTask(FROM_HERE,
> > createSameThreadTask(&ScreenOrientationController::dispatchChangeEvent,
> this));
> > FYI the blink scheduler team are thinking of adding a helper to make this
kind
> > of refactor easier.
> > 
> > Something on the lines of helper.postUniqueTask(FROM_HERE,
> > createSameThreadTask(&ScreenOrientationController::dispatchChangeEvent,
> this));
> > 
> > The helper would contain the m_isDispatchingEvent logic (since this seems to
> be
> > a reasonably common pattern).
> > 
> > The helper's constructor would take the WebThread you wanted the task to run
> on.
> > 
> > When we implement that, we'll probably refactor this to use the new method.
> 
> Thanks, that's good to know. This could of course wait until that is ready, no
> particular rush.

For reference, https://code.google.com/p/chromium/issues/detail?id=477258

[sof, 2015-04-17 06:57:32]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-04-17 06:57:47]

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

[commit-bot: I haz the power, 2015-04-17 08:04:02]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193927

[tkent, 2015-04-17 08:25:08]

Can you use Document::enqueueUniqueAnimationFrameEvent?

[sof, 2015-04-17 09:09:12]

On 2015/04/17 08:25:08, tkent wrote:
> Can you use Document::enqueueUniqueAnimationFrameEvent?

I could, it would dispatch the change event via the animation controller's queue
just fine, I think (haven't tested.)

Its queue would be reused, but otherwise there's no reason to associate a
'change' simple event with delivery of animation-related events (which _may_
also be queued.) ?