Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(115)

Issue 2783993002: Send vrdisplayactivate to the most recently focused navigator listening for displayactivate. (Closed)

Created:
3 years, 8 months ago by mthiesse
Modified:
3 years, 8 months ago
Reviewers:
bajones
CC:
chromium-reviews, haraken, blink-reviews, feature-vr-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Send vrdisplayactivate to the most recently focused navigator listening for displayactivate. Since focus changes are reported late in the Android lifecycle, we can end up in a situation where we send vrdisplayactivate, but no webVR frames are considered focused yet. This change makes it so that even if no webVR frame is currently focused, we still send displayactivate to the most recently focused frame (which has the added benefit of ensuring only one frame can call requestPresent in response). This is safe because we only fire displayactivate at all if we are sure some frame was listening for displayactivate before the Android VR DON flow paused chrome and unfocused the frame. In the future we should track focus fully browser-side to more securely solve this issue. This CL also prevents a possible reconnect loop if the page mistakenly thinks it has focus when it doesn't by detecting repeated connection failures. BUG=706472 Review-Url: https://codereview.chromium.org/2783993002 Cr-Commit-Position: refs/heads/master@{#460558} Committed: https://chromium.googlesource.com/chromium/src/+/b6d4321b17cea01782056a317aeb781367921531

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -10 lines) Patch
M device/vr/android/gvr/gvr_device_provider.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M device/vr/vr_device_manager.h View 2 chunks +4 lines, -1 line 0 comments Download
M device/vr/vr_device_manager.cc View 3 chunks +14 lines, -2 lines 0 comments Download
M device/vr/vr_display_impl.cc View 2 chunks +4 lines, -0 lines 0 comments Download
M device/vr/vr_service_impl.cc View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/vr/VRDisplay.h View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/vr/VRDisplay.cpp View 3 chunks +4 lines, -2 lines 0 comments Download

Messages

Total messages: 14 (8 generated)
mthiesse
PTAL
3 years, 8 months ago (2017-03-29 18:10:33 UTC) #4
bajones
On 2017/03/29 18:10:33, mthiesse wrote: > PTAL LGTM
3 years, 8 months ago (2017-03-29 20:42:55 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2783993002/1
3 years, 8 months ago (2017-03-29 20:44:46 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/260594)
3 years, 8 months ago (2017-03-29 21:19:46 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2783993002/1
3 years, 8 months ago (2017-03-29 21:36:47 UTC) #11
commit-bot: I haz the power
3 years, 8 months ago (2017-03-29 22:38:34 UTC) #14
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/b6d4321b17cea01782056a317aeb...

Powered by Google App Engine
This is Rietveld 408576698