Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916423002:
[WebVR] Handle slow connecting devices. (Closed)
Description[WebVR] Handle slow connecting devices.
A race condition exists when entering WebVR while already in VR (from
VrShell) which slows down devices connecting. We keep track if we have
seen an immediate handling of the device without a connection (which
signifies that libraries aren't loaded) which allows us to return a 0
count in that case, but return non-zero in the case where a device will
eventually connect.
BUG=729614
Review-Url: https://codereview.chromium.org/2916423002
Cr-Commit-Position: refs/heads/master@{#477103}
Committed: https://chromium.googlesource.com/chromium/src/+/c0376a2d193d886e60e2e618be407e4a262446df
Patch Set 1 #
Total comments: 4
Patch Set 2 : add a check on the callstack when no libraries available #
Messages
Total messages: 19 (11 generated)
amp@chromium.org changed reviewers: + bajones@chromium.org, mthiesse@chromium.org
When trying to address the promise resolving issue we inadvertenly created a race condition in which entering WebVR from VrShell would report no devices (they were just slow). This change accounts for that, but feels like a really ugly way to handle it. Still I would like to merge it back to 60 and I think this is cleaner than trying to rearchticture the interaction between the browser and renderer (but we should do that soon).
https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:56: std::move(callback).Run(expected_devices); I'm confused. Doesn't this just re-introduce the bug where we report 1 device, but that device never connects?
On 2017/06/05 16:18:08, mthiesse wrote: > https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.cc > File device/vr/vr_service_impl.cc (right): > > https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.c... > device/vr/vr_service_impl.cc:56: std::move(callback).Run(expected_devices); > I'm confused. Doesn't this just re-introduce the bug where we report 1 device, > but that device never connects? It's subtle, but I verified it works in both scenario's. I tried to comment how it works, but really we just need to overhaul the whole mess. I think that will involve changing mojo interfaces around though, and therefore not easily mergable back to 60. If we really don't like it and want to wait until a proper overhaul we can revert my previous fix in the meantime. This will get the transitions in VrShell working again, but the promise will stop resolving again and the tests which were updated to catch that will break so they will need to be reverted as well, but those aren't major problems.
https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:56: std::move(callback).Run(expected_devices); On 2017/06/05 16:18:07, mthiesse wrote: > I'm confused. Doesn't this just re-introduce the bug where we report 1 device, > but that device never connects? Let me attempt a better explanation (let me know if you want this in the comments). If all the devices are immediately handled (probably only happens when we don't have to load libraries and instantiate vr displays) then we can assume that all devices that are going to connect have already connected and so we return that value (line 54). Usually this ends up being 0 and so the VRController on the rendered side immediately resolves and returns 0 devices to the javascript side. If we haven't handled all the devices, then something is still working on loading or instantiating and so we return the actual number of devices that were registered in the manager (from line 47). Then when the device connects and calls back (line 92) it will signal the rendered and resolve the promise when the number of connected devices which called back equals the number passed back here.
Description was changed from ========== [WebVR] Handle slow connecting devices. A race condition exists when entering WebVR while already in VR (from VrShell) which slows down devices connecting. We keep track if we have seen an immediate handling of the device without a connection (which signifies that libraries aren't loaded) which allows us to return a 0 count in that case, but return non-zero in the case where a device will eventually connect. BUG=727969 ========== to ========== [WebVR] Handle slow connecting devices. A race condition exists when entering WebVR while already in VR (from VrShell) which slows down devices connecting. We keep track if we have seen an immediate handling of the device without a connection (which signifies that libraries aren't loaded) which allows us to return a 0 count in that case, but return non-zero in the case where a device will eventually connect. BUG=729614 ==========
ugh this is so gross :P lgtm as a fix for M60, but as we've already agreed upon we desperately need some re-architecting here. https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:94: } Can we DCHECK on the else case there we're inside of a SetClient call?
The CQ bit was checked by amp@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...
https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2916423002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:94: } On 2017/06/05 17:36:27, mthiesse wrote: > Can we DCHECK on the else case there we're inside of a SetClient call? Done. Switched the if around (back to the original) and used an AutoReset to verify the stack.
The CQ bit was checked by amp@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 amp@chromium.org
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2916423002/#ps20001 (title: "add a check on the callstack when no libraries available")
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": 20001, "attempt_start_ts": 1496690265778650,
"parent_rev": "773f81b206a0e30c9742428389af3e1eeec90997", "commit_rev":
"c0376a2d193d886e60e2e618be407e4a262446df"}
Message was sent while issue was closed.
Description was changed from ========== [WebVR] Handle slow connecting devices. A race condition exists when entering WebVR while already in VR (from VrShell) which slows down devices connecting. We keep track if we have seen an immediate handling of the device without a connection (which signifies that libraries aren't loaded) which allows us to return a 0 count in that case, but return non-zero in the case where a device will eventually connect. BUG=729614 ========== to ========== [WebVR] Handle slow connecting devices. A race condition exists when entering WebVR while already in VR (from VrShell) which slows down devices connecting. We keep track if we have seen an immediate handling of the device without a connection (which signifies that libraries aren't loaded) which allows us to return a 0 count in that case, but return non-zero in the case where a device will eventually connect. BUG=729614 Review-Url: https://codereview.chromium.org/2916423002 Cr-Commit-Position: refs/heads/master@{#477103} Committed: https://chromium.googlesource.com/chromium/src/+/c0376a2d193d886e60e2e618be40... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c0376a2d193d886e60e2e618be40... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
