Fix default initialization for DeviceMotionEvent.interval.

Fix default initialization for DeviceMotionEvent.interval.

The specification requires that this field be initialized to zero by
default.

BUG=704186
Review-Url: https://codereview.chromium.org/2758933002
Cr-Commit-Position: refs/heads/master@{#458982}
Committed: https://chromium.googlesource.com/chromium/src/+/25b307908f44425f58b08445e42b1a3a7ca3ab00

[Reilly Grant (use Gerrit), 2017-03-17 22:12:04]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[Reilly Grant (use Gerrit), 2017-03-17 22:12:30]

reillyg@chromium.org changed reviewers:
+ timvolodine@chromium.org

[Reilly Grant (use Gerrit), 2017-03-17 22:12:31]

Please take a look.

[commit-bot: I haz the power, 2017-03-17 22:12:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 00:06:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 00:06:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Reilly Grant (use Gerrit), 2017-03-18 00:18:49]

Update LayoutTests.

[Reilly Grant (use Gerrit), 2017-03-18 00:25:30]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 00:26:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 01:53:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 01:53:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Reilly Grant (use Gerrit), 2017-03-20 17:32:24]

Fix layout test expectations.

[Reilly Grant (use Gerrit), 2017-03-20 17:33:16]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 17:33:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 19:27:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 19:27:41]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2017-03-22 17:05:27]

maybe add a bug esp. if there are multiple patches?
a few questions/comments below

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt
(left):

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt:1:
This is a testharness.js-based test.
why is this file being deleted?

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp
(right):

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp:84:
return m_deviceMotionData->interval();
isNull = false; ?

also likely the isNull etc can be removed altogether if the idl is updated..
maybe a follow-up patch?

[Reilly Grant (use Gerrit), 2017-03-22 17:10:21]

Description was changed from

==========
Fix default initialization for DeviceMotionEvent.interval.

The specification requires that this field be initialized to zero by
default.

BUG=None
==========

to

==========
Fix default initialization for DeviceMotionEvent.interval.

The specification requires that this field be initialized to zero by
default.

BUG=704186
==========

[Reilly Grant (use Gerrit), 2017-03-22 17:13:33]

I've filed a bug to track these changes and added the bug number.

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt
(left):

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt:1:
This is a testharness.js-based test.
On 2017/03/22 17:05:27, timvolodine wrote:
> why is this file being deleted?

Deleting the -expected.txt file for a testharness.js-based test causes the test
runner to simply check that all tests passed instead of comparing the output to
an expected value.

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp
(right):

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp:84:
return m_deviceMotionData->interval();
On 2017/03/22 17:05:27, timvolodine wrote:
> isNull = false; ?
> 
> also likely the isNull etc can be removed altogether if the idl is updated..
> maybe a follow-up patch?

The generated bindings code initializes isNull to false before calling this
function.

I have submitted a pull request to the deviceorientation spec which makes this
attribute non-nullable.

[timvolodine, 2017-03-22 18:27:26]

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt
(left):

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt:1:
This is a testharness.js-based test.
On 2017/03/22 17:13:33, Reilly Grant wrote:
> On 2017/03/22 17:05:27, timvolodine wrote:
> > why is this file being deleted?
> 
> Deleting the -expected.txt file for a testharness.js-based test causes the
test
> runner to simply check that all tests passed instead of comparing the output
to
> an expected value.

do you mean it's not needed? if so, why was it there in the first place?
this patch does not appear to change devicemotionevent-init.html so not sure why
this should be included here..

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp
(right):

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp:84:
return m_deviceMotionData->interval();
On 2017/03/22 17:13:33, Reilly Grant wrote:
> On 2017/03/22 17:05:27, timvolodine wrote:
> > isNull = false; ?
> > 
> > also likely the isNull etc can be removed altogether if the idl is updated..
> > maybe a follow-up patch?
> 
> The generated bindings code initializes isNull to false before calling this
> function.

In that case perhaps add a this as a comment? Otherwise think not very clear
what happens with this by reference parameter (and why it's needed at all)

> 
> I have submitted a pull request to the deviceorientation spec which makes this
> attribute non-nullable.

yes, thanks, I've merged it.

[Reilly Grant (use Gerrit), 2017-03-22 18:36:35]

Explicitly set isNull to false.

[Reilly Grant (use Gerrit), 2017-03-22 18:38:03]

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt
(left):

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt:1:
This is a testharness.js-based test.
On 2017/03/22 18:27:26, timvolodine wrote:
> On 2017/03/22 17:13:33, Reilly Grant wrote:
> > On 2017/03/22 17:05:27, timvolodine wrote:
> > > why is this file being deleted?
> > 
> > Deleting the -expected.txt file for a testharness.js-based test causes the
> test
> > runner to simply check that all tests passed instead of comparing the output
> to
> > an expected value.
> 
> do you mean it's not needed? if so, why was it there in the first place?
> this patch does not appear to change devicemotionevent-init.html so not sure
why
> this should be included here..

This file is no longer needed because with this patch applied all of the tests
are now passing. It was there before to encode the fact that we expected the
test on line 2 to fail.

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp
(right):

https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp:84:
return m_deviceMotionData->interval();
On 2017/03/22 18:27:26, timvolodine wrote:
> On 2017/03/22 17:13:33, Reilly Grant wrote:
> > On 2017/03/22 17:05:27, timvolodine wrote:
> > > isNull = false; ?
> > > 
> > > also likely the isNull etc can be removed altogether if the idl is
updated..
> > > maybe a follow-up patch?
> > 
> > The generated bindings code initializes isNull to false before calling this
> > function.
> 
> In that case perhaps add a this as a comment? Otherwise think not very clear
> what happens with this by reference parameter (and why it's needed at all)

It is required by the bindings code because the attribute is nullable. I've
added a line to explicitly set it to false though my next patch will delete it
since it changes the IDL to make the attribute non-nullable.

[Reilly Grant (use Gerrit), 2017-03-22 18:38:08]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 18:39:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[timvolodine, 2017-03-22 18:41:15]

On 2017/03/22 18:38:03, Reilly Grant wrote:
>
https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt
> (left):
> 
>
https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/orientation-event/devicemotionevent-init-expected.txt:1:
> This is a testharness.js-based test.
> On 2017/03/22 18:27:26, timvolodine wrote:
> > On 2017/03/22 17:13:33, Reilly Grant wrote:
> > > On 2017/03/22 17:05:27, timvolodine wrote:
> > > > why is this file being deleted?
> > > 
> > > Deleting the -expected.txt file for a testharness.js-based test causes the
> > test
> > > runner to simply check that all tests passed instead of comparing the
output
> > to
> > > an expected value.
> > 
> > do you mean it's not needed? if so, why was it there in the first place?
> > this patch does not appear to change devicemotionevent-init.html so not sure
> why
> > this should be included here..
> 
> This file is no longer needed because with this patch applied all of the tests
> are now passing. It was there before to encode the fact that we expected the
> test on line 2 to fail.
> 
>
https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp
> (right):
> 
>
https://codereview.chromium.org/2758933002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/device_orientation/DeviceMotionEvent.cpp:84:
> return m_deviceMotionData->interval();
> On 2017/03/22 18:27:26, timvolodine wrote:
> > On 2017/03/22 17:13:33, Reilly Grant wrote:
> > > On 2017/03/22 17:05:27, timvolodine wrote:
> > > > isNull = false; ?
> > > > 
> > > > also likely the isNull etc can be removed altogether if the idl is
> updated..
> > > > maybe a follow-up patch?
> > > 
> > > The generated bindings code initializes isNull to false before calling
this
> > > function.
> > 
> > In that case perhaps add a this as a comment? Otherwise think not very clear
> > what happens with this by reference parameter (and why it's needed at all)
> 
> It is required by the bindings code because the attribute is nullable. I've
> added a line to explicitly set it to false though my next patch will delete it
> since it changes the IDL to make the attribute non-nullable.

ok thanks, lgtm

[Reilly Grant (use Gerrit), 2017-03-22 19:03:16]

The CQ bit was unchecked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2017-03-22 19:20:18]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2017-03-22 19:21:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 21:46:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 21:46:32]

Failed to apply patch for
third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp:123
  error:
third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp: patch
does not apply

Patch:      
third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp
Index: third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp
diff --git
a/third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp
b/third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp
index
e0965a8dfc06bb45cba9722f27abbfc6564632ba..398858a9659d7cad8e23bdb9a854ca96e633fc3c
100644
--- a/third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp
+++ b/third_party/WebKit/Source/modules/device_orientation/DeviceMotionData.cpp
@@ -105,10 +105,9 @@ DeviceMotionData* DeviceMotionData::create(
     Acceleration* acceleration,
     Acceleration* accelerationIncludingGravity,
     RotationRate* rotationRate,
-    bool canProvideInterval,
     double interval) {
   return new DeviceMotionData(acceleration, accelerationIncludingGravity,
-                              rotationRate, canProvideInterval, interval);
+                              rotationRate, interval);
 }
 
 DeviceMotionData* DeviceMotionData::create(const DeviceMotionEventInit& init) {
@@ -123,7 +122,7 @@ DeviceMotionData* DeviceMotionData::create(const
DeviceMotionEventInit& init) {
       init.hasRotationRate()
           ? DeviceMotionData::RotationRate::create(init.rotationRate())
           : nullptr,
-      init.hasInterval(), init.hasInterval() ? init.interval() : 0);
+      init.hasInterval() ? init.interval() : 0);
 }
 
 DeviceMotionData* DeviceMotionData::create(const WebDeviceMotionData& data) {
@@ -142,21 +141,18 @@ DeviceMotionData* DeviceMotionData::create(const
WebDeviceMotionData& data) {
           data.hasRotationRateAlpha, data.rotationRateAlpha,
           data.hasRotationRateBeta, data.rotationRateBeta,
           data.hasRotationRateGamma, data.rotationRateGamma),
-      true, data.interval);
+      data.interval);
 }
 
-DeviceMotionData::DeviceMotionData()
-    : m_canProvideInterval(false), m_interval(0) {}
+DeviceMotionData::DeviceMotionData() : m_interval(0) {}
 
 DeviceMotionData::DeviceMotionData(Acceleration* acceleration,
                                    Acceleration* accelerationIncludingGravity,
                                    RotationRate* rotationRate,
-                                   bool canProvideInterval,
                                    double interval)
     : m_acceleration(acceleration),
       m_accelerationIncludingGravity(accelerationIncludingGravity),
       m_rotationRate(rotationRate),
-      m_canProvideInterval(canProvideInterval),
       m_interval(interval) {}
 
 DEFINE_TRACE(DeviceMotionData) {

[Reilly Grant (use Gerrit), 2017-03-23 00:19:34]

Rebased.

[Reilly Grant (use Gerrit), 2017-03-23 00:22:09]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2017-03-23 00:22:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2758933002/#ps80001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-03-23 00:22:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 02:28:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490228529336610,
"parent_rev": "7cbc0c2fa215a66f620a77054250a0d635446577", "commit_rev":
"25b307908f44425f58b08445e42b1a3a7ca3ab00"}

[commit-bot: I haz the power, 2017-03-23 02:29:05]

Description was changed from

==========
Fix default initialization for DeviceMotionEvent.interval.

The specification requires that this field be initialized to zero by
default.

BUG=704186
==========

to

==========
Fix default initialization for DeviceMotionEvent.interval.

The specification requires that this field be initialized to zero by
default.

BUG=704186

Review-Url: https://codereview.chromium.org/2758933002
Cr-Commit-Position: refs/heads/master@{#458982}
Committed:
https://chromium.googlesource.com/chromium/src/+/25b307908f44425f58b08445e42b...
==========

[commit-bot: I haz the power, 2017-03-23 02:29:09]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/25b307908f44425f58b08445e42b...