Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2915993004:
[WebVR] Only count devices that have successfully connected. (Closed)
|
Created:
3 years, 6 months ago by amp Modified:
3 years, 6 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, feature-vr-reviews_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[WebVR] Only count devices that have successfully connected.
This allows the navigator.GetVRDisplays() promise to resolve when the
underlying VR libraries are not installed or out of date.
BUG=727969
Review-Url: https://codereview.chromium.org/2915993004
Cr-Commit-Position: refs/heads/master@{#476516}
Committed: https://chromium.googlesource.com/chromium/src/+/7a7288d743d475fd4f1a903a1d3d9503ba282792
Patch Set 1 #Patch Set 2 : track registered and connected devices and only callback into client once #Patch Set 3 : Ignore device manager, just track the connected devices directly from the callbacks #
Messages
Total messages: 24 (11 generated)
amp@chromium.org changed reviewers: + bajones@chromium.org, billorr@chromium.org
I'll also need separate approval for the mojom change, but holding off on that until we decide this is the right change.
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
I think what we should do instead, to simplify a lot of things here, is to have VrShellDelegate create the GvrDeviceProvider when it's ready to create GvrDevices, then the GvrDeviceProvider registers itself with the VrDeviceManager. I don't think it makes sense to have VRDeviceManager create and register all of the device providers for each platform, and I think doing this refactoring would be good overall. This would also not require any mojom changes.
I think this would also imply service_->SetClient triggers VrShellDelegate creation. Which I think will be a good thing, having all initialization happen at the same time with appropriate ownership directions.
mthiesse@chromium.org changed reviewers: + tiborg@chromium.org
+tibor for opinions, as he recently did some work in this area.
amp@chromium.org changed reviewers: - tiborg@chromium.org
After some discussion with Bill I uploaded patch set 2 which attempted to track things better (and also not require a mojom change), but I couldn't get it working quite yet (compiles, but doesn't resolve the promise for some reason) I'll look at the larger refactoring and see if I can make progress that way as I agree it makes more sense (I was just hoping for a smaller change to get things working in the mean time).
In the future though, we should also probably get rid of the numConnectedDevices return value in general. That feels riddled with race conditions of devices connecting or disconnecting after initially setting the client.
PTAL. Latest patch set trims down even further and only touches vr_service_impl. A refactor is still the best plan, but this brings the implementation into line with expectations until then.
lgtm
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...
Actually, hold on, not lgtm yet. Won't this break a bunch of sites that don't listen for vrdisplayconnect? They won't find displays when loading now, right?
On 2017/06/01 22:03:14, mthiesse wrote: > Actually, hold on, not lgtm yet. Won't this break a bunch of sites that don't > listen for vrdisplayconnect? They won't find displays when loading now, right? Why won't they find displays when loading? I was incorrect about the way the flow works in my description in the bug. It turns out that all the devices were being connected in manager->AddService and only after all that completes would we go back and call 'GetConnectedDevices' (line 44 above). The thing is that the manager never knew if the devices were actually connected or not (because the callback was only routed through to the ServiceImpl) so that value would be wrong if the device had a problem connecting. The refactor should fix all of this, but I don't think this change impacts current functionality (other than returning the correct value when no devices actually connected).
lgtm. Forgot the devices are created when VRDeviceManager is.
Description was changed from ========== [WebVR] Remove devices that can not be connected. This allows the navigator.GetVRDisplays() promise to resolve when the underlying VR libraries are not installed or out of date. BUG=727969 ========== to ========== [WebVR] Only count devices that have successfully connected. This allows the navigator.GetVRDisplays() promise to resolve when the underlying VR libraries are not installed or out of date. BUG=727969 ==========
The CQ bit was unchecked by amp@chromium.org
The CQ bit was checked by amp@chromium.org
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": 40001, "attempt_start_ts": 1496358054780390,
"parent_rev": "62433b1aac27d4652b1bff4a6e0e424b4bac9edf", "commit_rev":
"7a7288d743d475fd4f1a903a1d3d9503ba282792"}
Message was sent while issue was closed.
Description was changed from ========== [WebVR] Only count devices that have successfully connected. This allows the navigator.GetVRDisplays() promise to resolve when the underlying VR libraries are not installed or out of date. BUG=727969 ========== to ========== [WebVR] Only count devices that have successfully connected. This allows the navigator.GetVRDisplays() promise to resolve when the underlying VR libraries are not installed or out of date. BUG=727969 Review-Url: https://codereview.chromium.org/2915993004 Cr-Commit-Position: refs/heads/master@{#476516} Committed: https://chromium.googlesource.com/chromium/src/+/7a7288d743d475fd4f1a903a1d3d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7a7288d743d475fd4f1a903a1d3d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
