Closed Bug 1329902 Opened 7 years ago Closed 7 years ago

[Presentation WebAPI] fix device changed/removed event in AndroidCastDeviceProvider

Categories

(Firefox for Android Graveyard :: Screencasting, defect)

Product:
Firefox for Android Graveyard
Component:
Screencasting
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: schien, Assigned: schien)

Details

Attachments

(3 files)

Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.
59 bytes, text/x-review-board-request
kuoe0.tw
: review+
Details
Bug 1329902 - Part 2, handle onRouteChanged without onRouteAdded.
59 bytes, text/x-review-board-request
kuoe0.tw
: review+
Details
Bug 1329902 - Part 3, don't notify Gecko if removed route is not a presentation display.
59 bytes, text/x-review-board-request
kuoe0.tw
: review+
Details 
1. AndroidCastDeviceProvider doesn't register "AndroidCastDevice:Changed" observer.
2. AndroidCastDeviceProvider try remove device that doesn't existed in its device list.
I also observe that in some not-well-implemented Android firmware `onRouteChanged` might be invoked without `onRouteAdded` beforehand.
Comment on attachment 8825309 [details]
Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.

https://reviewboard.mozilla.org/r/103486/#review104096

LGTM!

::: dom/presentation/provider/AndroidCastDeviceProvider.js:456
(Diff revision 1)
>        case TOPIC_ANDROID_CAST_DEVICE_REMOVED: {
>          let deviceId = aData;
> +        if (!this._deviceList.has(deviceId)) {
> +          break;
> +        }
> +

Maybe we should add some comments here to note that is caused by the instability of Chromecast discovery on Android.
Attachment #8825309 - Flags: review?(kuoe0) → review+
Comment on attachment 8825310 [details]
Bug 1329902 - Part 2, handle onRouteChanged without onRouteAdded.

https://reviewboard.mozilla.org/r/103488/#review104098

LGTM!
Attachment #8825310 - Flags: review?(kuoe0) → review+
Comment on attachment 8825309 [details]
Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.

https://reviewboard.mozilla.org/r/103486/#review104118
Comment on attachment 8825309 [details]
Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.

https://reviewboard.mozilla.org/r/103486/#review104096

> Maybe we should add some comments here to note that is caused by the instability of Chromecast discovery on Android.

This is not caused by the instability of Android SDK. `OnRouteRemoved` can also be called for a non-chromecast device. I can also do a null check in https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java#190 to reduce messaging from Java to Gecko JS if necessary.

```
if (null != displays.remove(route.getId())) {
  GeckoAppShell.notifyObservers("AndroidCastDevice:Removed", route.getId());
}
```
see comment #7.
Flags: needinfo?(kuoe0)
Comment on attachment 8825309 [details]
Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.

https://reviewboard.mozilla.org/r/103486/#review104096

> This is not caused by the instability of Android SDK. `OnRouteRemoved` can also be called for a non-chromecast device. I can also do a null check in https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java#190 to reduce messaging from Java to Gecko JS if necessary.
> 
> ```
> if (null != displays.remove(route.getId())) {
>   GeckoAppShell.notifyObservers("AndroidCastDevice:Removed", route.getId());
> }
> ```

I mean `this._deviceList.has(deviceId)` is impossible to be `false` when `TOPIC_ANDROID_CAST_DEVICE_REMOVED` event is emitted. It means we didn't receivce add/change event to add this device into device list.
Comment on attachment 8825707 [details]
Bug 1329902 - Part 3, don't notify Gecko if removed route is not a presentation display.

https://reviewboard.mozilla.org/r/103780/#review104518
Attachment #8825707 - Flags: review?(kuoe0) → review+
Comment on attachment 8825309 [details]
Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.

https://reviewboard.mozilla.org/r/103486/#review104096

> I mean `this._deviceList.has(deviceId)` is impossible to be `false` when `TOPIC_ANDROID_CAST_DEVICE_REMOVED` event is emitted. It means we didn't receivce add/change event to add this device into device list.

after part-3 landed, `this._deviceList.has(deviceId)` should not return false. Just add it as a safty check.
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f5731369ae3
Part 1, fix device changed/removed event in AndroidCastDeviceProvider. r=KuoE0
https://hg.mozilla.org/integration/autoland/rev/26a07768718d
Part 2, handle onRouteChanged without onRouteAdded. r=KuoE0
https://hg.mozilla.org/integration/autoland/rev/09aca1c16440
Part 3, don't notify Gecko if removed route is not a presentation display. r=KuoE0
Flags: needinfo?(kuoe0)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: