Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1947963003:
Make deviceorientationabsolute confrom with spec on Chromium OS (Closed)
|
Created:
4 years, 7 months ago by jonross Modified:
4 years, 7 months ago Reviewers:
timvolodine CC:
chromium-reviews, riju_, Michael van Ouwerkerk, jam, darin-cc_chromium.org, mlamouri+watch-sensors_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake deviceorientationabsolute confrom with spec on Chromium OS
Absolute device orientation cannot be provided on Chromium OS machines. This update sends a one-off null event when deviceorientationabsolute is requesteed. This conforms with the spec: http://w3c.github.io/deviceorientation/spec-source-orientation.html
TEST=http://timvolodine.github.io/deviceorientation-test/
BUG=609131
Committed: https://crrev.com/123238ab16647ff1fd69a664844522793eae69b6
Cr-Commit-Position: refs/heads/master@{#392674}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Rebase to include 1954753002 #Patch Set 4 : #Messages
Total messages: 19 (6 generated)
jonross@chromium.org changed reviewers: + timvolodine@chromium.org
Hey Tim, Could you review this change to send null-event on CrOS for deviceorientationabsolute? Thanks, Jon
On 2016/05/04 17:46:48, jonross wrote: > Hey Tim, > > Could you review this change to send null-event on CrOS for > deviceorientationabsolute? > > Thanks, > Jon Thanks Jon, I'll take a look tomorrow
Sorry my original mac patch was incomplete, I've added some more bits to it https://codereview.chromium.org/1915273003/. Comments below relate to these changes. https://codereview.chromium.org/1947963003/diff/1/content/browser/device_sens... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/1947963003/diff/1/content/browser/device_sens... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:36: orientation_absolute_buffer->seqlock.WriteBegin(); think it would be good to also set data.absolute = true here, that way implementation would explicitly know what kind of event it is, plus also more in line with the spec. https://codereview.chromium.org/1947963003/diff/1/content/browser/device_sens... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:55: case CONSUMER_TYPE_ORIENTATION_ABSOLUTE: best to have some 'clean-up' code here as well
https://codereview.chromium.org/1947963003/diff/1/content/browser/device_sens... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/1947963003/diff/1/content/browser/device_sens... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:36: orientation_absolute_buffer->seqlock.WriteBegin(); On 2016/05/06 15:10:41, timvolodine wrote: > think it would be good to also set data.absolute = true here, that way > implementation would explicitly know what kind of event it is, plus also more in > line with the spec. Done. https://codereview.chromium.org/1947963003/diff/1/content/browser/device_sens... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:55: case CONSUMER_TYPE_ORIENTATION_ABSOLUTE: On 2016/05/06 15:10:41, timvolodine wrote: > best to have some 'clean-up' code here as well Good call. Done.
lgtm % comments below also in description: "Asbolute" -> Absolute ;) if not too much work, before you land it, could you try this on ChromeOS with the https://codereview.chromium.org/1954753002/ patch, just to be sure. Thanks! https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:58: // properly cleanup, and fire an all-null event to blink. there is no "fire event" here, maybe just remove the whole comment altogether, it seems self-explanatory.
On 2016/05/09 18:20:41, timvolodine wrote: > lgtm % comments below > > also in description: "Asbolute" -> Absolute ;) > > if not too much work, before you land it, could you try this on ChromeOS with > the https://codereview.chromium.org/1954753002/ patch, just to be sure. Thanks! > > https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... > File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc > (right): > > https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... > content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:58: // > properly cleanup, and fire an all-null event to blink. > there is no "fire event" here, maybe just remove the whole comment altogether, > it seems self-explanatory. I'm remote for a week, so I'll try this with your patch next monday, and then land.
On 2016/05/09 18:39:37, jonross wrote: > On 2016/05/09 18:20:41, timvolodine wrote: > > lgtm % comments below > > > > also in description: "Asbolute" -> Absolute ;) > > > > if not too much work, before you land it, could you try this on ChromeOS with > > the https://codereview.chromium.org/1954753002/ patch, just to be sure. > Thanks! > > > > > https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... > > File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc > > (right): > > > > > https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... > > content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:58: // > > properly cleanup, and fire an all-null event to blink. > > there is no "fire event" here, maybe just remove the whole comment altogether, > > it seems self-explanatory. > > I'm remote for a week, so I'll try this with your patch next monday, and then > land. ok thanks, preferably before the M52 branch point ;)
Oh, yeah. That should be fine to hit. On Mon, May 9, 2016 at 11:43 AM <timvolodine@chromium.org> wrote: > On 2016/05/09 18:39:37, jonross wrote: > > On 2016/05/09 18:20:41, timvolodine wrote: > > > lgtm % comments below > > > > > > also in description: "Asbolute" -> Absolute ;) > > > > > > if not too much work, before you land it, could you try this on > ChromeOS > with > > > the https://codereview.chromium.org/1954753002/ patch, just to be > sure. > > Thanks! > > > > > > > > > > https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... > > > File > content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc > > > (right): > > > > > > > > > > https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... > > > > content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:58: // > > > properly cleanup, and fire an all-null event to blink. > > > there is no "fire event" here, maybe just remove the whole comment > altogether, > > > it seems self-explanatory. > > > > I'm remote for a week, so I'll try this with your patch next monday, and > then > > land. > > ok thanks, preferably before the M52 branch point ;) > > https://codereview.chromium.org/1947963003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Make deviceorientationabsolute confrom with spec on Chromium OS Asbolute device orientation cannot be provided on Chromium OS machines. This update sends a one-off null event when deviceorientationabsolute is requesteed. This conforms with the spec: http://w3c.github.io/deviceorientation/spec-source-orientation.html TEST=http://timvolodine.github.io/deviceorientation-test/ BUG=609131 ========== to ========== Make deviceorientationabsolute confrom with spec on Chromium OS Absolute device orientation cannot be provided on Chromium OS machines. This update sends a one-off null event when deviceorientationabsolute is requesteed. This conforms with the spec: http://w3c.github.io/deviceorientation/spec-source-orientation.html TEST=http://timvolodine.github.io/deviceorientation-test/ BUG=609131 ==========
https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:58: // properly cleanup, and fire an all-null event to blink. On 2016/05/09 18:20:41, timvolodine wrote: > there is no "fire event" here, maybe just remove the whole comment altogether, > it seems self-explanatory. Done.
On 2016/05/10 18:01:08, jonross wrote: > https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... > File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc > (right): > > https://codereview.chromium.org/1947963003/diff/20001/content/browser/device_... > content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:58: // > properly cleanup, and fire an all-null event to blink. > On 2016/05/09 18:20:41, timvolodine wrote: > > there is no "fire event" here, maybe just remove the whole comment altogether, > > it seems self-explanatory. > > Done. Ended up with some spare time in the end. Everything is still fine after rebasing on your patch.
The CQ bit was checked by jonross@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/1947963003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947963003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947963003/60001
Message was sent while issue was closed.
Description was changed from ========== Make deviceorientationabsolute confrom with spec on Chromium OS Absolute device orientation cannot be provided on Chromium OS machines. This update sends a one-off null event when deviceorientationabsolute is requesteed. This conforms with the spec: http://w3c.github.io/deviceorientation/spec-source-orientation.html TEST=http://timvolodine.github.io/deviceorientation-test/ BUG=609131 ========== to ========== Make deviceorientationabsolute confrom with spec on Chromium OS Absolute device orientation cannot be provided on Chromium OS machines. This update sends a one-off null event when deviceorientationabsolute is requesteed. This conforms with the spec: http://w3c.github.io/deviceorientation/spec-source-orientation.html TEST=http://timvolodine.github.io/deviceorientation-test/ BUG=609131 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make deviceorientationabsolute confrom with spec on Chromium OS Absolute device orientation cannot be provided on Chromium OS machines. This update sends a one-off null event when deviceorientationabsolute is requesteed. This conforms with the spec: http://w3c.github.io/deviceorientation/spec-source-orientation.html TEST=http://timvolodine.github.io/deviceorientation-test/ BUG=609131 ========== to ========== Make deviceorientationabsolute confrom with spec on Chromium OS Absolute device orientation cannot be provided on Chromium OS machines. This update sends a one-off null event when deviceorientationabsolute is requesteed. This conforms with the spec: http://w3c.github.io/deviceorientation/spec-source-orientation.html TEST=http://timvolodine.github.io/deviceorientation-test/ BUG=609131 Committed: https://crrev.com/123238ab16647ff1fd69a664844522793eae69b6 Cr-Commit-Position: refs/heads/master@{#392674} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/123238ab16647ff1fd69a664844522793eae69b6 Cr-Commit-Position: refs/heads/master@{#392674} |
