Make DeviceOrientationEvent.prototype.absolute non-nullable

Make DeviceOrientationEvent.prototype.absolute non-nullable

This is to match the spec and Gecko:
http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceorientation
https://hg.mozilla.org/mozilla-central/file/85e218929a7a/dom/webidl/DeviceOrientationEvent.webidl

BUG=460722
Committed: https://crrev.com/97906458b23205d57b35626955a33b67154d2e21
Cr-Commit-Position: refs/heads/master@{#378747}

[philipj_slow, 2016-02-24 18:08:07]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-24 18:10:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737443002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737443002/1

[commit-bot: I haz the power, 2016-02-24 19:26:59]

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

[commit-bot: I haz the power, 2016-02-24 19:27:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[philipj_slow, 2016-02-25 03:26:50]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-25 03:29:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737443002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737443002/20001

[commit-bot: I haz the power, 2016-02-25 05:26:24]

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

[commit-bot: I haz the power, 2016-02-25 05:26:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[philipj_slow, 2016-02-26 03:45:12]

Description was changed from

==========
Make DeviceOrientationEvent.prototype.absolute non-nullable

BUG=460722
==========

to

==========
Make DeviceOrientationEvent.prototype.absolute non-nullable

This is to match the spec and Gecko:
http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceori...
https://hg.mozilla.org/mozilla-central/file/85e218929a7a/dom/webidl/DeviceOri...

BUG=460722
==========

[philipj_slow, 2016-02-26 03:47:57]

philipj@opera.com changed reviewers:
+ timvolodine@chromium.org

[philipj_slow, 2016-02-26 03:47:58]

PTAL. I have done some ad-hoc testing on Android and found that there absolute
is already true even on the first event, so I think mainly Mac will be affected,
where currently I get an event where all the attributes are null, and it will be
null,null,null,false after this change.

Commenting inline for things I'd like your thoughts on.

[philipj_slow, 2016-02-26 03:54:01]

By the way, the reason I'm poking at this is so that I can add the constructors
for these events, and then deprecate the init*Event methods and remove support
for creating these events using document.createEvent. A long path ahead :)

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_mac.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_mac.cc:197:
orientation_buffer_->data.hasAbsolute = true;
This isn't the cause of the observable change on Mac, it's the below all-null
event that will change.

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_win.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_win.cc:133:
buffer_->data.hasAbsolute = has_alpha || has_beta || has_gamma;
Same as for Android, do you think I should make the above `has_alpha || has_beta
|| has_gamma` instead of true?

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:43:
buffer->data.absolute = absolute;
Should I make this absolute && ready, so that cases that were previously null
are now false instead? Does the spec offer any guidance and can you think of a
case where it matters?

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_chromeos.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/sensor_manager_chromeos.cc:72:
orientation_buffer_->data.hasAbsolute = true;
At least ChromeOS looks simple.

[philipj_slow, 2016-03-01 06:59:31]

Ping timvolodine@

[timvolodine, 2016-03-01 22:36:26]

lgtm % comments

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_mac.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_mac.cc:197:
orientation_buffer_->data.hasAbsolute = true;
On 2016/02/26 03:54:01, philipj_UTC7 wrote:
> This isn't the cause of the observable change on Mac, it's the below all-null
> event that will change.

right, the change is that it cannot be all-null anymore, but absolute=false with
the rest null, which is fine because sufficient for detection.

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_win.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_win.cc:133:
buffer_->data.hasAbsolute = has_alpha || has_beta || has_gamma;
On 2016/02/26 03:54:01, philipj_UTC7 wrote:
> Same as for Android, do you think I should make the above `has_alpha ||
has_beta
> || has_gamma` instead of true?

the spec doesn't really specify the value of absolute in case of a 'all-null'
event and I don't think it's important. However for consistency maybe do add
has_alpha || ... as you suggest to the absolute property above.

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:43:
buffer->data.absolute = absolute;
On 2016/02/26 03:54:01, philipj_UTC7 wrote:
> Should I make this absolute && ready, so that cases that were previously null
> are now false instead? Does the spec offer any guidance and can you think of a
> case where it matters?

the spec doesn't mention the value of absolute in case of all-null event for
example, however this is a helper method to set the values in the buffer so I
think we can keep it the way it is now.

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_chromeos.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/sensor_manager_chromeos.cc:72:
orientation_buffer_->data.hasAbsolute = true;
On 2016/02/26 03:54:01, philipj_UTC7 wrote:
> At least ChromeOS looks simple.

;)

[timvolodine, 2016-03-01 22:46:02]

https://codereview.chromium.org/1737443002/diff/20001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1737443002/diff/20001/components/test_runner/...
components/test_runner/test_runner.cc:1005: args->GetNext(&absolute);
oh hold on, actually don't think this is fixed in all layout tests, e.g.
WebKit/LayoutTests/fast/dom/DeviceOrientation/resources/basic-operation.js
still invokes testRunner.setMockDeviceOrientation with all parameters.
Interestingly the tests still seem to pass probably because we always use
absolute=true as mock value.

Basically all calls to testRunner.setMockDeviceOrientation in layout tests need
to be updated..

[philipj_slow, 2016-03-02 07:30:42]

https://codereview.chromium.org/1737443002/diff/20001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1737443002/diff/20001/components/test_runner/...
components/test_runner/test_runner.cc:1005: args->GetNext(&absolute);
On 2016/03/01 22:46:01, timvolodine wrote:
> oh hold on, actually don't think this is fixed in all layout tests, e.g.
> WebKit/LayoutTests/fast/dom/DeviceOrientation/resources/basic-operation.js
> still invokes testRunner.setMockDeviceOrientation with all parameters.
> Interestingly the tests still seem to pass probably because we always use
> absolute=true as mock value.
> 
> Basically all calls to testRunner.setMockDeviceOrientation in layout tests
need
> to be updated..

Oops. I've updated the other instances, and indeed they were all passing true as
the final two arguments.

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_win.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_win.cc:133:
buffer_->data.hasAbsolute = has_alpha || has_beta || has_gamma;
On 2016/03/01 22:36:25, timvolodine wrote:
> On 2016/02/26 03:54:01, philipj_UTC7 wrote:
> > Same as for Android, do you think I should make the above `has_alpha ||
> has_beta
> > || has_gamma` instead of true?
> 
> the spec doesn't really specify the value of absolute in case of a 'all-null'
> event and I don't think it's important. However for consistency maybe do add
> has_alpha || ... as you suggest to the absolute property above.

This data comes directly from ISensorDataReport and I can't tell if it makes any
guarantees, but it seems possible in principle for all of these to be null.
Since the old code was defensive enough to check for it, I'll do it too.

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (left):

https://codereview.chromium.org/1737443002/diff/20001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:43:
buffer->data.absolute = absolute;
On 2016/03/01 22:36:25, timvolodine wrote:
> On 2016/02/26 03:54:01, philipj_UTC7 wrote:
> > Should I make this absolute && ready, so that cases that were previously
null
> > are now false instead? Does the spec offer any guidance and can you think of
a
> > case where it matters?
> 
> the spec doesn't mention the value of absolute in case of all-null event for
> example, however this is a helper method to set the values in the buffer so I
> think we can keep it the way it is now.

I looked at all the call sites, and it turns out that making this absolute &&
ready wouldn't actually change the resulting value in any case. I'll leave it
as-is.

[philipj_slow, 2016-03-02 07:32:02]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-02 07:32:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737443002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737443002/40001

[philipj_slow, 2016-03-02 07:33:42]

philipj@opera.com changed reviewers:
+ mkwst@chromium.org

[philipj_slow, 2016-03-02 07:33:43]

Mike, can you rubberstamp components/test_runner/test_runner.* please?

[commit-bot: I haz the power, 2016-03-02 10:32:05]

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

[commit-bot: I haz the power, 2016-03-02 10:32:07]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-03-02 13:48:52]

components/test_runner LGTM!

[philipj_slow, 2016-03-02 14:40:31]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2016-03-02 14:40:31]

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/1737443002/#ps40001
(title: "address feedback")

[commit-bot: I haz the power, 2016-03-02 14:40:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737443002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737443002/40001

[commit-bot: I haz the power, 2016-03-02 14:46:29]

Description was changed from

==========
Make DeviceOrientationEvent.prototype.absolute non-nullable

This is to match the spec and Gecko:
http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceori...
https://hg.mozilla.org/mozilla-central/file/85e218929a7a/dom/webidl/DeviceOri...

BUG=460722
==========

to

==========
Make DeviceOrientationEvent.prototype.absolute non-nullable

This is to match the spec and Gecko:
http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceori...
https://hg.mozilla.org/mozilla-central/file/85e218929a7a/dom/webidl/DeviceOri...

BUG=460722
==========

[commit-bot: I haz the power, 2016-03-02 14:46:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-02 14:47:52]

Description was changed from

==========
Make DeviceOrientationEvent.prototype.absolute non-nullable

This is to match the spec and Gecko:
http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceori...
https://hg.mozilla.org/mozilla-central/file/85e218929a7a/dom/webidl/DeviceOri...

BUG=460722
==========

to

==========
Make DeviceOrientationEvent.prototype.absolute non-nullable

This is to match the spec and Gecko:
http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceori...
https://hg.mozilla.org/mozilla-central/file/85e218929a7a/dom/webidl/DeviceOri...

BUG=460722

Committed: https://crrev.com/97906458b23205d57b35626955a33b67154d2e21
Cr-Commit-Position: refs/heads/master@{#378747}
==========

[commit-bot: I haz the power, 2016-03-02 14:47:54]

Patchset 3 (id:??) landed as
https://crrev.com/97906458b23205d57b35626955a33b67154d2e21
Cr-Commit-Position: refs/heads/master@{#378747}

[timvolodine, 2016-03-02 19:48:34]

On 2016/03/02 14:47:54, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/97906458b23205d57b35626955a33b67154d2e21
> Cr-Commit-Position: refs/heads/master@{#378747}

Thanks for cleaning this up Philip! now looks more like the spec :)