Presentation API: don't fire availability events when page is foregrounded unless needed |
|||||||||||
Issue descriptionWhen a page uses Presentation API with an availability remote device, it will receive an availability change (to false) then another one (to true). We should not fire an event if a page is foregrounded and there was no change between the previous known value and the current one.
,
Mar 31 2017
That sounds reasonable. - Does that mean the initial getAvailability() could take up to 5s in the negative case as well? - If we are changing Media Router itself, we should make sure UI components that depend on it are not affected.
,
Mar 31 2017
Yes, we will wait for 5s to detect the negative case too. Most pages would not show the UI until the positive response anyway so it shouldn't have much impact I think. We could change the MRPs where needed (e.g. if Hangouts "discovery" is symmetrical in terms of positive/negative result, we don't need to impose the timeout on them).
,
Jan 10 2018
,
Jan 10 2018
,
Apr 5 2018
This is a lingering correctness/ergonomics issue with the API that would be good to address in Q2.
,
Apr 9 2018
,
Sep 4
,
Oct 2
,
Nov 7
,
Nov 16
,
Nov 26
+takumif@ I instrumented the code and verified that the analysis from Comment #1 by avayvod@ is essentially correct. When a PresentationAvailability goes in the background, it signals the Media Router to unregister its observer for the corresponding MediaSource. If that was the only observer, it removes the sink query and expunges the cache for that source. When the PresentationAvailability goes back in the foreground, if it's the first to register an observer for the MediaSource again, the observer is fired with a cached empty list here: https://cs.chromium.org/chromium/src/chrome/browser/media/router/mojo/media_router_mojo_impl.cc?rcl=0803a8bb25489af4db4a54542663f9fad87f2697&l=581 The quick fix is to not fire the observer until at least one MRP returns a sink for a new sink query (or all return none). This is correct and will eliminate most spurious state changes. However, this may add an arbitrary delay until a getAvailability() promise is resolved. There might be user experience issues as well depending on whether various bits of UX trigger actions based on the sink observer firing. If necessary, we can add a second code path to determine screen availability within a timeout. This is more involved and unclear if this is a Blink change, an MR change or both. I didn't analyze the Android media router code path. A similar fix may be needed there. takumif@, who has refactored a lot of the desktop MRP code, might have opinions here...
,
Nov 26
,
Nov 26
> The quick fix is to not fire the observer until at least one MRP returns a sink for a new sink query (or all return none). This sounds right to me. I don't think we currently keep track of which MRPs have already responded to a sink query, so we'd need to add that to MediaSinksQuery. We could also combine this with a timeout (within MediaSinksQuery), in case the MRPs don't respond in a timely manner.
,
Nov 26
Adding a global timeout to MediaSinkQuery would be most backwards compatible. I'm just not sure which observers need a timeout and whether it would be the same across observers. I'm leaning towards implementing any timeouts outside of the MR.
,
Dec 28
I have a WIP patch for this that fixes the positive case: when sinks are available, the availability object will no longer fire a spurious false event when the page is foregrounded. Need to verify that other cases are handled correctly w.r.t. changes in page visibility and device availability. It's tricky because the same code path is used to resolve the getAvailability() promise (which needs a definite true/false answer) and trigger the change event (which just needs to signal transitions). For a new URL (where we don't have any cached sinks to fall back on), it might be worth resolving the promise early with 'availability = false', then waiting for a future 'true' transition if a MRP finds a compatible sink, versus introducing some arbitrary timeout to resolve the promise with 'availability = false'. That might be something to work on in a separate issue.
,
Dec 28
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by avayvod@chromium.org
, Mar 31 2017