New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 723032 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 517102



Sign in to add a comment

Merge WebRemotePlaybackAvailability and ScreenAvailability, propagate the enum to WebAvailabilityObservers

Project Member Reported by avayvod@chromium.org, May 16 2017

Issue description

Looking 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?
 

Comment 1 by mfo...@chromium.org, May 16 2017

What does this new state map onto for Presentation API?
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

Comment 3 by mfo...@chromium.org, 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?

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.

Comment 5 by mfo...@chromium.org, 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.




Status: Available (was: Untriaged)
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.
Blocking: 517102
Owner: avayvod@chromium.org
Status: Started (was: Available)
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.
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.
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)?
Project Member

Comment 12 by bugdroid1@chromium.org, 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

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.



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.
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?
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.
Project Member

Comment 17 by bugdroid1@chromium.org, 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

Project Member

Comment 18 by bugdroid1@chromium.org, 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

Owner: mlamouri@chromium.org
Status: Untriaged (was: Started)
Assigning all my bugs to Mounir for him to triage and close/reassign later.
Cc: mlamouri@chromium.org
Owner: mfo...@chromium.org
mfoltz@, is this something you can triage/reassign?
Owner: ----
Status: Fixed (was: Untriaged)
Per discussion with mfoltz@, we believe this is done.

Sign in to add a comment