Merge WebRemotePlaybackAvailability and ScreenAvailability, propagate the enum to WebAvailabilityObservers |
|||||||
Issue descriptionLooking into making RemotePlayback an WebPresentationAvailabilityObserver. Currently RemotePlayback gets 5 different values (defined in the WebRemotePlaybackAvailability enum) back from the backend to throw the right DOMException from watchAvailability(). WebPresentationAvailabilityObserver only gets a boolean from PresentationDispatcher, while dispatcher has four values defined in the ScreenAvailability enum. I think we could do the following to allow RemotePlayback to be implemented better: 1. move ScreenAvailability enum to third_party/WebKit/public/platform/WebScreenAvailability 2. pass the enum via AvailabilityChanged to WebPresentationAvailabilityObservers instead of the bool (rename them to WebScreenAvailabilityObservers?) 3. add the fifth state to the enum (kSourceNotSupported - when the devices are available but the current source is not compatible with any of those). WDYT?
,
May 16 2017
It might be just unused by the Presentation API. In theory, it could be used if the page uses a single Cast presentation URL and there're no Cast devices available but there're some other screens. In RemotePlayback API the NotSupported exception is meant to be a hint that a different source (e.g. .webm instead of .mp4) could work if the remote playback devices available support one but not the other. See step 6 of the prompt() algorithm: https://w3c.github.io/remote-playback/#prompt-user-for-changing-remote-playback-state
,
May 16 2017
There is already ScreenAvailability::UNSUPPORTED used to mean that availability is not supported for that request. It is used to reject the getAvailability() promise and seems to mean the same for Remote Playback API. Can you do step 1 and pass UNSUPPORTED via WebPresentationAvailabilityCallbacks?
,
May 16 2017
RemotePlayback API distinguishes kSourceNotSupported and kSourceNotCompatible: the former for when the UA will never be able to play the current source remotely, the latter is for when the UA knows how to play the current source remotely _AND_ there're devices but they don't match the kind needed for the current source. We had this changed as a result of https://github.com/w3c/remote-playback/issues/34, before we'd just reject prompt() with NotFoundError even if there're devices but none compatible with the current source. Thus ScreenAvailability can't be used as is, because RemotePlayback API currently needs 5 availability states, not four.
,
May 17 2017
OK. Thanks for clarifying. The PresentationService uses UNSUPPORTED to denote when availability monitoring is not supported for a given source. In reality this is only the case when it is disabled for all sources (i.e. background availability monitoring is currently disabled). This is the only case where we reject the getAvailability() promise. It uses UNAVAILABLE which is all cases where the user agent believes (at the current instant) that there is no screen for a given source. However it is *not* a terminal state. Your semantics are different from both of these cases. In the PresentationService I would recommend renaming UNSUPPORTED -> DISABLED and adding SOURCE_NOT_SUPPORTED as you suggested earlier. This new state would map onto availability = false for the Presentation API. We don't expose detailed information about source/device compatibility for privacy reasons and didn't see a need for developers to care about the difference. I think the Privacy section of the RP API should be updated to reflect this - the "one bit" statement is not accurate.
,
May 17 2017
,
May 31 2017
Upon further looking at this today, I think reusing one availability enum is a small change to the Presentation API / MediaRouter stack - it seems like NotSupported is only propagated to the callbacks (as a response to GetAvailability()) while the availability listeners get true or false from the MediaRouter. Changing all this plumbing to pass the enum value instead will probably touch even media route providers. It seems easier/faster for RemotePlayback to actually request a PresentationAvailability object (a new one each time the source is changed) to get NotSupported event via the callback and true/false via the listener. SourceNotSupported would be the last thing which could be plumbed later.
,
Jun 8 2017
,
Jun 9 2017
A couple of questions: 1. Can I remove OnScreenAvailabilityNotSupported() in favor of OnScreenAvailabilityChanged(ScreenAvailability::UNSUPPORTED) ? The only reason to keep the separate method I see is to more explicitly prevent changes from UNSUPPORTED to AVAILABLE and others but maybe it's just the historical mimicking of how getAvailability() works? 2. MR could reuse SOURCE_NOT_COMPATIBLE for when the presentation url is not valid instead of reporting availability as false which is what it currently does. It may not be used by the Presentation API though.
,
Jun 9 2017
1. That seems fine. As part of this change, I would like UNSUPPORTED to be renamed DISABLED per c#5. 2. That seems logical, and might enable a small optimization: we can remove the MR listener for incompatible URLs since they will never become AVAILABLE.
,
Jun 9 2017
I have UNSUPPORTED and SOURCE_NOT_COMPATIBLE atm. Just to confirm, you'd like: UNKNOWN DISABLED SOURCE_NOT_SUPPORTED UNAVAILABLE AVAILABLE (in this order wrt determining the availability for multiple URLs)?
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1275d6645acf8c4575a53f395353d70c87f70f72 commit 1275d6645acf8c4575a53f395353d70c87f70f72 Author: Anton Vayvod <avayvod@google.com> Date: Fri Jun 09 23:48:03 2017 [RemotePlayback, Presentation] ScreenAvailability - part 1 Adds blink::mojom::ScreenAvailability in presentation.mojom to be used by both RemotePlayback and Presentation APIs. Replace PresentationDispatcher::ScreenAvailability with it. Switches the plumbing from PresentationAvailability to PresentationServiceImpl to use the enums instead of bool. Updates the tests. (part 2 will update the plumbing from PresentationServiceImpl to MediaRouter). BUG= 723032 TEST=existing/mofidied tests Change-Id: I818e46507ffb02ca6f1d1a526832b734b8011891 Reviewed-on: https://chromium-review.googlesource.com/527376 Commit-Queue: Anton Vayvod <avayvod@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Derek Cheng <imcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#478460} [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/content/browser/presentation/presentation_service_impl.cc [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/content/browser/presentation/presentation_service_impl_unittest.cc [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/content/renderer/presentation/presentation_dispatcher.cc [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/content/renderer/presentation/presentation_dispatcher.h [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/content/renderer/presentation/presentation_dispatcher_unittest.cc [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/third_party/WebKit/Source/modules/presentation/PresentationAvailability.h [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/third_party/WebKit/public/platform/modules/presentation/OWNERS [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/third_party/WebKit/public/platform/modules/presentation/WebPresentationAvailabilityObserver.h [modify] https://crrev.com/1275d6645acf8c4575a53f395353d70c87f70f72/third_party/WebKit/public/platform/modules/presentation/presentation.mojom
,
Jun 12 2017
That should be fine - as long as DISABLED remains a terminal state, i.e. it cannot go to AVAILABLE in the future for a given URL. That is because we reject the getAvailability promise if any URL is DISABLED.
,
Jun 12 2017
When DISABLED is the current availability, it indicates background monitoring is disabled. However, later on when the page calls start()/prompt() the availability monitoring algorithm will run and therefore the internal availability may change for the same URL, no? getAvailability() then should still throw though.
,
Jun 12 2017
Yes, the availability returned by start()/prompt() may be different. However there won't be any PresentationAvailability objects to fire event since the getAvailability() promise rejected. I assume that the remote playback API works the same way with watchAvailability?
,
Jun 12 2017
Yes. As long as PresentationDispatcher is not updated with new availability results upon prompt()/start() we should be fine. Mental note to myself: need to check that the native cast button recognizes disabled state properly.
,
Jun 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e3c39863bf551e31b2ffb556e7ae0b85c803e8db commit e3c39863bf551e31b2ffb556e7ae0b85c803e8db Author: Anton Vayvod <avayvod@google.com> Date: Tue Jun 20 23:47:05 2017 [MediaRouter] ScreenAvailability - part 2 Using the ScreenAvailability enum added in https://chromium-review.googlesource.com/c/527376/ in content/browser and chrome/browser/ BUG= 723032 TEST=existing/updated tests Change-Id: I6667f4d6237c242dfe9446cbf9e82dd30f0858ec Reviewed-on: https://chromium-review.googlesource.com/530284 Commit-Queue: Anton Vayvod <avayvod@chromium.org> Reviewed-by: mark a. foltz <mfoltz@chromium.org> Reviewed-by: Derek Cheng <imcheng@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#481030} [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/chrome/browser/DEPS [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/chrome/browser/media/router/mock_screen_availability_listener.h [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/chrome/browser/media/router/presentation_media_sinks_observer.cc [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/chrome/browser/media/router/presentation_media_sinks_observer.h [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/chrome/browser/media/router/presentation_media_sinks_observer_unittest.cc [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/chrome/browser/media/router/presentation_service_delegate_impl.cc [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/content/browser/presentation/presentation_service_impl.cc [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/content/browser/presentation/presentation_service_impl.h [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/content/browser/presentation/presentation_service_impl_unittest.cc [modify] https://crrev.com/e3c39863bf551e31b2ffb556e7ae0b85c803e8db/content/public/browser/presentation_screen_availability_listener.h
,
Jun 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ddd1c408d37adc96182b9ed1cce769c160bdaabe commit ddd1c408d37adc96182b9ed1cce769c160bdaabe Author: Anton Vayvod <avayvod@google.com> Date: Wed Jun 21 02:15:11 2017 [MediaRouter,PresentationAPI] ScreenAvailability - part 3 Remove OnScreenAvailabilityNotSupported in favor of ScreenAvailability::DISABLED. BUG= 723032 TEST=existing/updated tests. Change-Id: I824e832ad6454adfb4b221fd101b9eb4f65b46a3 Reviewed-on: https://chromium-review.googlesource.com/530324 Commit-Queue: Anton Vayvod <avayvod@chromium.org> Reviewed-by: mark a. foltz <mfoltz@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#481084} [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/chrome/browser/media/router/mock_screen_availability_listener.h [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/chrome/browser/media/router/presentation_service_delegate_impl.cc [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/content/browser/presentation/presentation_service_impl.cc [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/content/browser/presentation/presentation_service_impl.h [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/content/browser/presentation/presentation_service_impl_unittest.cc [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/content/public/browser/presentation_screen_availability_listener.h [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/content/renderer/presentation/presentation_dispatcher.cc [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/content/renderer/presentation/presentation_dispatcher.h [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/content/renderer/presentation/presentation_dispatcher_unittest.cc [modify] https://crrev.com/ddd1c408d37adc96182b9ed1cce769c160bdaabe/third_party/WebKit/public/platform/modules/presentation/presentation.mojom
,
Oct 18 2017
Assigning all my bugs to Mounir for him to triage and close/reassign later.
,
Oct 19 2017
mfoltz@, is this something you can triage/reassign?
,
Oct 19 2017
,
Oct 23 2017
Per discussion with mfoltz@, we believe this is done. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by mfo...@chromium.org
, May 16 2017