Fire null Device Motion events only once.

Fire null Device Motion events only once.

This CL makes sure that the all-null events are fired only once. This lets the developers determine that no motion information can be provided and allows to save battery.

Summarized this CL also implements the following features:
- Unregister controller when receiving a null event.
- Don't fire controllers removed or added during event dispatch.
- Purge the controllers asap, i.e. not only on dispatch. This is also necessary to save battery in the case of Device Orientation events which fire only when substantially changed.

Layout-Tests:
- Tests for firing all null or partial null events.
- Tests no firing of listeners added during event dispatch.
- Tests firing last available event in case of multiple iframes.

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

[abarth-chromium, 2013-07-31 00:56:19]

https://chromiumcodereview.appspot.com/21256002/diff/1/LayoutTests/fast/dom/D...
File LayoutTests/fast/dom/DeviceMotion/fire-last-event.html (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/LayoutTests/fast/dom/D...
LayoutTests/fast/dom/DeviceMotion/fire-last-event.html:6: <script
src="script-tests/fire-last-event.js"></script>
You can just line this script in the HTML file if you like.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/DeviceMotionController.cpp (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceMotionController.cpp:88:
DeviceMotionEvent* motionEvent = static_cast<DeviceMotionEvent*>(event);
How do we know this static_cast is valid?  Can we add some sort of ASSERT to
verify that the type is correct?  For example, we could check the
interfaceName() of the event.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceMotionDispatcher.cpp:79: m_isDispatching
= true;
Consider using TemporaryChange rather than manually twiddling this flag.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceMotionDispatcher.cpp:81: size_t end =
m_controllers.size();
end -> length or size

We use the term |end| to refer to iterators, and this is an index not an
iterator.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/DeviceSensorEventController.cpp (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceSensorEventController.cpp:69:
m_needsCheckingNullEvents = false;
We still need to check for null events in the stopUpdating branch?

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/DeviceSensorEventDispatcher.cpp (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceSensorEventDispatcher.cpp:86:
m_needsPurge = false;
Should we ASSERT(m_needsPurge) at the top of this function?

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/NewDeviceOrientationController.cpp
(right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/NewDeviceOrientationController.cpp:35: 
This blank line looks spurious.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/NewDeviceOrientationController.cpp:90:
DeviceOrientationEvent* orientationEvent =
static_cast<DeviceOrientationEvent*>(event);
Same comment about this blind static_cast.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/NewDeviceOrientationController.cpp:92: return
!data->canProvideAlpha() && !data->canProvideBeta() && !data->canProvideGamma();
Do we have any mechanism to keep this check up to date in the case that
DeviceOrientationEvent adds more fields over time?

[timvolodine, 2013-07-31 13:59:00]

https://chromiumcodereview.appspot.com/21256002/diff/1/LayoutTests/fast/dom/D...
File LayoutTests/fast/dom/DeviceMotion/fire-last-event.html (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/LayoutTests/fast/dom/D...
LayoutTests/fast/dom/DeviceMotion/fire-last-event.html:6: <script
src="script-tests/fire-last-event.js"></script>
On 2013/07/31 00:56:19, abarth wrote:
> You can just line this script in the HTML file if you like.

Done.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/DeviceMotionController.cpp (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceMotionController.cpp:88:
DeviceMotionEvent* motionEvent = static_cast<DeviceMotionEvent*>(event);
On 2013/07/31 00:56:19, abarth wrote:
> How do we know this static_cast is valid?  Can we add some sort of ASSERT to
> verify that the type is correct?  For example, we could check the
> interfaceName() of the event.

The static cast is a bit unfortunate here, but my reasoning for its correctness
is as follows:

The virtual method isNullEvent is only called from
DeviceSensorEventController::dispatchDeviceEvent. For motion,
dispatchDeviceEvent is called from two places, either from
DeviceMotionController::didChangeDeviceMotion or
DeviceSensorEventController::fireDeviceEvent. In the first case the passed event
will be of type motion by definition, in the second case the event will be
fetched by getLastEvent which is an virtual overriden method.

I've added an assert on event->type().

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/DeviceMotionDispatcher.cpp (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceMotionDispatcher.cpp:79: m_isDispatching
= true;
On 2013/07/31 00:56:19, abarth wrote:
> Consider using TemporaryChange rather than manually twiddling this flag.

Done.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceMotionDispatcher.cpp:81: size_t end =
m_controllers.size();
On 2013/07/31 00:56:19, abarth wrote:
> end -> length or size
> 
> We use the term |end| to refer to iterators, and this is an index not an
> iterator.

changed to size.
Actually I was basing myself on EventTarget::fireEventListeners which does a
similar dispatch and uses |end| to refer to an index as well..

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/DeviceSensorEventController.cpp (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceSensorEventController.cpp:69:
m_needsCheckingNullEvents = false;
On 2013/07/31 00:56:19, abarth wrote:
> We still need to check for null events in the stopUpdating branch?

If you mean we are not setting m_needsCheckingNullEvents in the stopUpdating
case that is intentional. If javascript does addDeviceListenter twice one after
each other then each time we'll fire once and unregister.

m_needsCheckingNullEvents flag is meant to optimize the case where the events
are not null, once we receive one non-null event we can be certain there will be
no null-events in the future.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/DeviceSensorEventDispatcher.cpp (right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/DeviceSensorEventDispatcher.cpp:86:
m_needsPurge = false;
On 2013/07/31 00:56:19, abarth wrote:
> Should we ASSERT(m_needsPurge) at the top of this function?

Done.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
File Source/modules/device_orientation/NewDeviceOrientationController.cpp
(right):

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/NewDeviceOrientationController.cpp:35: 
On 2013/07/31 00:56:19, abarth wrote:
> This blank line looks spurious.

Done.

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/NewDeviceOrientationController.cpp:90:
DeviceOrientationEvent* orientationEvent =
static_cast<DeviceOrientationEvent*>(event);
On 2013/07/31 00:56:19, abarth wrote:
> Same comment about this blind static_cast.

added assert (analogous to the motion case).

https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
Source/modules/device_orientation/NewDeviceOrientationController.cpp:92: return
!data->canProvideAlpha() && !data->canProvideBeta() && !data->canProvideGamma();
On 2013/07/31 00:56:19, abarth wrote:
> Do we have any mechanism to keep this check up to date in the case that
> DeviceOrientationEvent adds more fields over time?

hmm, good point -- I've moved some of the code to canProvideEventData() in
DeviceMotionData and DeviceOrientationData, it looks a better place to test
'null-ness' there.

[abarth-chromium, 2013-07-31 22:28:00]

On 2013/07/31 13:59:00, timvolodine wrote:
>
https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
> Source/modules/device_orientation/DeviceMotionDispatcher.cpp:81: size_t end =
> m_controllers.size();
> On 2013/07/31 00:56:19, abarth wrote:
> > end -> length or size
> > 
> > We use the term |end| to refer to iterators, and this is an index not an
> > iterator.
> 
> changed to size.
> Actually I was basing myself on EventTarget::fireEventListeners which does a
> similar dispatch and uses |end| to refer to an index as well..

Would you be willing to fix that one as well?  (Maybe in a separate CL?)

>
https://chromiumcodereview.appspot.com/21256002/diff/1/Source/modules/device_...
> Source/modules/device_orientation/NewDeviceOrientationController.cpp:92:
return
> !data->canProvideAlpha() && !data->canProvideBeta() &&
!data->canProvideGamma();
> On 2013/07/31 00:56:19, abarth wrote:
> > Do we have any mechanism to keep this check up to date in the case that
> > DeviceOrientationEvent adds more fields over time?
> 
> hmm, good point -- I've moved some of the code to canProvideEventData() in
> DeviceMotionData and DeviceOrientationData, it looks a better place to test
> 'null-ness' there.

Thanks!

[abarth-chromium, 2013-07-31 22:28:58]

lgtm

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/21256002/12001

[commit-bot: I haz the power, 2013-08-01 00:31:56]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[timvolodine, 2013-08-01 11:00:52]

+ Mark FYI

[commit-bot: I haz the power, 2013-08-01 15:02:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/21256002/12001

[commit-bot: I haz the power, 2013-08-01 16:25:38]

Change committed as 155340