Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2859893002:
VR: Make work done during onResume in VrShellDelegate asynchronous. (Closed)
DescriptionVR: Make work done during onResume in VrShellDelegate asynchronous.
Makes registerDaydreamIntent run after onResume. Makes static checking of VR support level in onResume an async task.
Note that doing this work asynchronously is racy. In practice this shouldn't be an issue because the user would have to put their phone into their daydream headset within a few milliseconds of launching chrome. The failure mode is also not terrible, the user would return to Daydream home instead of Chrome.
BUG=718136
Review-Url: https://codereview.chromium.org/2859893002
Cr-Commit-Position: refs/heads/master@{#469141}
Committed: https://chromium.googlesource.com/chromium/src/+/2d3151bb4c3bf0346ceec9c4b43f895607fe8d2c
Patch Set 1 #
Total comments: 5
Patch Set 2 : Ensure we don't register the intent after pausing. #Patch Set 3 : ADdress comment #
Total comments: 2
Patch Set 4 : Add comments #Patch Set 5 : Future-proof comment #Messages
Total messages: 16 (8 generated)
mthiesse@chromium.org changed reviewers: + cjgrant@chromium.org, vollick@chromium.org
PTAL
The CQ bit was checked by mthiesse@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...
Description was changed from ========== VR: Make work done during onResume in VrShellDelegate asynchronous. Makes registerDaydreamIntent run after onResume. Makes static checking of VR support level in onResume an async task. BUG=718136 ========== to ========== VR: Make work done during onResume in VrShellDelegate asynchronous. Makes registerDaydreamIntent run after onResume. Makes static checking of VR support level in onResume an async task. Note that doing this work asynchronously is racy. In practice this shouldn't be an issue because the user would have to put their phone into their daydream headset within a few milliseconds of launching chrome. The failure mode is also not terrible, the user would return to Daydream home instead of Chrome. BUG=718136 ==========
Some extra explanation in the original description would be good here. https://codereview.chromium.org/2859893002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2859893002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:260: // call is slow, and takes ~10ms. I wouldn't include the time measurement here. Maybe in the bug... https://codereview.chromium.org/2859893002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:634: new Handler().postDelayed(new Runnable() { Why postDelayed(..., 0), instead of post()?
Updated CL description. https://codereview.chromium.org/2859893002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2859893002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:260: // call is slow, and takes ~10ms. On 2017/05/03 19:49:30, cjgrant wrote: > I wouldn't include the time measurement here. Maybe in the bug... I think it should be included here, so that readers of the code understand this is a slow call when that isn't immediately obvious. https://codereview.chromium.org/2859893002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:634: new Handler().postDelayed(new Runnable() { On 2017/05/03 19:49:30, cjgrant wrote: > Why postDelayed(..., 0), instead of post()? Done.
https://codereview.chromium.org/2859893002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2859893002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:239: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) return; Nit, but maybe add a quick comment about daydream not being supported on less than N as the reason for this check. I was wondering why we didn't just check the vr support level for daydream instead, but then realized this saves us from calling the daydream api at all.
https://codereview.chromium.org/2859893002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2859893002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:239: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) return; On 2017/05/03 20:07:07, amp wrote: > Nit, but maybe add a quick comment about daydream not being supported on less > than N as the reason for this check. > > I was wondering why we didn't just check the vr support level for daydream > instead, but then realized this saves us from calling the daydream api at all. Done.
lgtm https://codereview.chromium.org/2859893002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2859893002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:260: // call is slow, and takes ~10ms. On 2017/05/03 19:52:49, mthiesse wrote: > On 2017/05/03 19:49:30, cjgrant wrote: > > I wouldn't include the time measurement here. Maybe in the bug... > > I think it should be included here, so that readers of the code understand this > is a slow call when that isn't immediately obvious. Sure, but "slow call" is enough to justify async execution. Putting in 10ms is just asking for a future stale comment IMO. Up to you...
The CQ bit was checked by mthiesse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org Link to the patchset: https://codereview.chromium.org/2859893002/#ps80001 (title: "Future-proof comment")
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": 1493844835695330,
"parent_rev": "0da9a5fad5370f949189ba918c3d1d4934730d1c", "commit_rev":
"2d3151bb4c3bf0346ceec9c4b43f895607fe8d2c"}
Message was sent while issue was closed.
Description was changed from ========== VR: Make work done during onResume in VrShellDelegate asynchronous. Makes registerDaydreamIntent run after onResume. Makes static checking of VR support level in onResume an async task. Note that doing this work asynchronously is racy. In practice this shouldn't be an issue because the user would have to put their phone into their daydream headset within a few milliseconds of launching chrome. The failure mode is also not terrible, the user would return to Daydream home instead of Chrome. BUG=718136 ========== to ========== VR: Make work done during onResume in VrShellDelegate asynchronous. Makes registerDaydreamIntent run after onResume. Makes static checking of VR support level in onResume an async task. Note that doing this work asynchronously is racy. In practice this shouldn't be an issue because the user would have to put their phone into their daydream headset within a few milliseconds of launching chrome. The failure mode is also not terrible, the user would return to Daydream home instead of Chrome. BUG=718136 Review-Url: https://codereview.chromium.org/2859893002 Cr-Commit-Position: refs/heads/master@{#469141} Committed: https://chromium.googlesource.com/chromium/src/+/2d3151bb4c3bf0346ceec9c4b43f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2d3151bb4c3bf0346ceec9c4b43f... |
