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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

1. AndroidCastDeviceProvider doesn't register "AndroidCastDevice:Changed" observer.
2. AndroidCastDeviceProvider try remove device that doesn't existed in its device list.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I also observe that in some not-well-implemented Android firmware `onRouteChanged` might be invoked without `onRouteAdded` beforehand.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.

Review commit: https://reviewboard.mozilla.org/r/103486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103486/

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1329902 - Part 2, handle onRouteChanged without onRouteAdded.

Review commit: https://reviewboard.mozilla.org/r/103488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103488/

Comment 4

[Chih-Hsuan Kuo [:tommykuo]]

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.

Comment 5

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8825310 [details]
Bug 1329902 - Part 2, handle onRouteChanged without onRouteAdded.

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

LGTM!

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8825309 [details]
Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.

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

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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());
}
```

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

see comment #7.

Comment 9

[Chih-Hsuan Kuo [:tommykuo]]

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 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1329902 - Part 3, don't notify Gecko if removed route is not a presentation display.

Review commit: https://reviewboard.mozilla.org/r/103780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103780/

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8825309 [details]
Bug 1329902 - Part 1, fix device changed/removed event in AndroidCastDeviceProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103486/diff/1-2/

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8825310 [details]
Bug 1329902 - Part 2, handle onRouteChanged without onRouteAdded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103488/diff/1-2/

Comment 13

[Chih-Hsuan Kuo [:tommykuo]]

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

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f5731369ae3
https://hg.mozilla.org/mozilla-central/rev/26a07768718d
https://hg.mozilla.org/mozilla-central/rev/09aca1c16440