|
|
Created:
3 years, 9 months ago by Reilly Grant (use Gerrit) Modified:
3 years, 9 months ago Reviewers:
timvolodine CC:
chromium-reviews, blink-reviews, mlamouri+watch-blink_chromium.org, blink-reviews-w3ctests_chromium.org, haraken Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix 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
Patch Set 1 #Patch Set 2 : Update LayoutTests. #Patch Set 3 : Fix layout test expectations. #
Total comments: 8
Patch Set 4 : Explicitly set isNull to false. #Patch Set 5 : Rebased. #Messages
Total messages: 37 (23 generated)
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
reillyg@chromium.org changed reviewers: + timvolodine@chromium.org
Please take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Update LayoutTests.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Fix layout test expectations.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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 ==========
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.
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.
Explicitly set isNull to false.
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.
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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
The CQ bit was unchecked by reillyg@chromium.org
The CQ bit was checked by reillyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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) {
Rebased.
The CQ bit was checked by reillyg@chromium.org
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.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490228529336610, "parent_rev": "7cbc0c2fa215a66f620a77054250a0d635446577", "commit_rev": "25b307908f44425f58b08445e42b1a3a7ca3ab00"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/25b307908f44425f58b08445e42b... |