Closed Bug 1295087 Opened 8 years ago Closed 8 years ago

[Presentation WebAPI] Implement PresentationDeviceProvider for Chromecast devices on Fennec.

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: kuoe0.tw, Assigned: kuoe0.tw)

References

Details

(Whiteboard: [ETA Fx52])

Attachments

(1 file)

Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.
58 bytes, text/x-review-board-request
schien
: review+
smaug
: review+
snorp
: review+
Details 
I'm working on Bug 1252788. We'll make Presentation API support Chromecast devices on Fennec. I'll implement the provider for that.
Tommy, I am inferring you are going to work on this in a few months, so set P2. Please feel free to change it.
Priority: -- → P2
Assignee: nobody → kuoe0
Summary: Implement PresentationDeviceProvider for Chromecast devices on Fennec → Implement PresentationDeviceProvider for Chromecast devices on Fennec.
Summary: Implement PresentationDeviceProvider for Chromecast devices on Fennec. → [Presentation WebAPI] Implement PresentationDeviceProvider for Chromecast devices on Fennec.
Whiteboard: [ETA 8/19]
As discussed with SC, we move ETA to 9/2. Thank you!
Whiteboard: [ETA 8/19] → [ETA 9/2]
Whiteboard: [ETA 9/2] → [ETA 9/16]
Depends on: 1305351
Attachment #8796112 - Flags: review?(bugs)
Blocks: 1305352
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

Perhaps schien could review this.


(I started to read the patch a bit. It could use some comments, like, what does _correspondingControlChannel  do?)
Attachment #8796112 - Flags: review?(bugs) → review?(schien)
I could do a second round review later.
No problem, I'll review it first.
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review82322

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Add mode line.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:19
(Diff revision 1)
> +// globals Messaging
> +Cu.import("resource://gre/modules/Messaging.jsm");
> +
> +const DEBUG = false;
> +function log(str) {
> +  dump("-*- AndroidPresentationDeviceProvider -*-: " + str + "\n");

disable the log before checked-in, or use a pref if you want to monitor it first.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:23
(Diff revision 1)
> +function log(str) {
> +  dump("-*- AndroidPresentationDeviceProvider -*-: " + str + "\n");
> +}
> +
> +// Helper function: transfer nsIPresentationChannelDescription to json
> +function descriptionAsJson(aDescription) {

hmm...we only use this for debug logging. Maybe we should directly convert it to string instead of to JSON.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:68
(Diff revision 1)
> +      this._listener.notifyConnected();
> +    }
> +  },
> +
> +  onOffer: function LCC_onOffer(aOffer) {
> +    if (this._direction == "sender") {

use `const DIRECTION_SENDER = 1` to prevent string comparison.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:93
(Diff revision 1)
> +  set listener(aListener) {
> +    this._listener = aListener;
> +    if (this._pendingConnected) {
> +      this._pendingConnected = false;
> +      this._listener.notifyConnected();
> +    }

I think you'll still need `_pendingOffer` and `_pendingCandidate`.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:178
(Diff revision 1)
> +  _ctrlChannel: null,
> +
> +  // nsIPresentationDevice
> +  get id() { return this._id; },
> +
> +  set id(aId) { this._id = aId; },

id, name, type are all readonly attribute, you should set those value in constructor and remove the setter function.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:184
(Diff revision 1)
> +
> +  get name() { return this._name; },
> +
> +  set name(aName) { this._name = aName; },
> +
> +  get type() { return this._type; },

You can return "chromecast" without using the `_type`.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:191
(Diff revision 1)
> +  set type(aType) { this._type = aType; },
> +
> +  establishControlChannel: function CRDD_establishControlChannel() {
> +    this._ctrlChannel = new LocalControlChannel(this._provider,
> +                                                this.id,
> +                                                'sender');

use " for string.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:195
(Diff revision 1)
> +                                                this.id,
> +                                                'sender');
> +
> +    // Connect to Chromecast.
> +    Messaging.sendRequestForResult({
> +      type: "PresentationDevice:Start",

I'd like to use a more precise string like "AndroidCastDevice:XXX"

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:218
(Diff revision 1)
> +    });
> +  },
> +
> +  isRequestedUrlSupported: function CRDD_isRequestedUrlSupported(aUrl) {
> +    var url = Cc["@mozilla.org/network/io-service;1"]
> +              .getService(Ci.nsIIOService)

align `.` to `[`

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:230
(Diff revision 1)
> +
> +  set windowId(aWindowId) { this._windowId = aWindowId; },
> +
> +  // nsIObserver
> +  observe: function CRDD_observe(aSubject, aTopic, aData) {
> +    if (aTopic == 'presentation-view-ready') {

use " for string

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:233
(Diff revision 1)
> +  // nsIObserver
> +  observe: function CRDD_observe(aSubject, aTopic, aData) {
> +    if (aTopic == 'presentation-view-ready') {
> +      DEBUG && log("ChromecastRemoteDisplayDevice - observe: aTopic="
> +                   + aTopic + ' data=' + aData);
> +      if (this._windowId == aData) {

I'll prefer using |===|

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:239
(Diff revision 1)
> +        this._ctrlChannel.notifyConnected();
> +      }
> +    }
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationLocalDevice,

`Ci.nsIPresentationDevice` is required.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:245
(Diff revision 1)
> +                                         Ci.nsIObserver]),
> +};
> +
> +function AndroidPresentationDeviceProvider() {
> +  // observer registration
> +  Services.obs.addObserver(this, "PresentationDevice:Added", false);

* use `const TOPIC_CAST_DEVICE_ADD = ...` to prevent typo error.
* "PresentationDevice:Added" seems to general. I'd like to have a more meaningful name, e.g. "AndroidCastDevice:Added".

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:246
(Diff revision 1)
> +};
> +
> +function AndroidPresentationDeviceProvider() {
> +  // observer registration
> +  Services.obs.addObserver(this, "PresentationDevice:Added", false);
> +  Services.obs.addObserver(this, "PresentationDevice:Removed", false);

Need to pull the full list of available devices so that the deviceList in provider have the synced state with Android.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:250
(Diff revision 1)
> +  Services.obs.addObserver(this, "PresentationDevice:Added", false);
> +  Services.obs.addObserver(this, "PresentationDevice:Removed", false);
> +}
> +
> +AndroidPresentationDeviceProvider.prototype = {
> +  _port: 0,

`_port` is not used

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:283
(Diff revision 1)
> +                                      aControlChannel,
> +                                      aIsFromReceiver);
> +  },
> +
> +  onDisconnect: function APDP_onDisconnect(aId, aReason) {
> +    if (!!aReason) {

* compare to `Cr.NS_OK`
* The disconnect logic looks pretty wrong to me because you'll enter infinite recursion if aReason is not NS_OK. In addition, we didn't call `ctrlChannelListener.notifyDisconnect` at all.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:290
(Diff revision 1)
> +    }
> +  },
> +
> +  // nsIPresentationDeviceProvider
> +  set listener(aListener) {
> +    this._listener = aListener;

* How do we ask Android to start monitoring Chromecast device? I'd like to see a logic that monitoring is only triggered while listener is set.
* If there is already device in the deviceList, you'll need to call `addDevice` for all of them.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:297
(Diff revision 1)
> +
> +  get listener() {
> +    return this._listener;
> +  },
> +
> +  forceDiscovery: function APDP_forceDiscovery() {},

Please put some comment if there is no API in Android SDK to do force discovery.

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:301
(Diff revision 1)
> +
> +  forceDiscovery: function APDP_forceDiscovery() {},
> +
> +  // nsIObserver
> +  observe: function APDP_observe(aSubject, aTopic, aData) {
> +    if (aTopic == "PresentationDevice:Added") {

use switch case here

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:311
(Diff revision 1)
> +      device.windowId = deviceInfo.uuid;
> +      this._deviceList.set(device.id, device);
> +
> +      let deviceManager = Cc["@mozilla.org/presentation-device/manager;1"]
> +                          .getService(Ci.nsIPresentationDeviceManager);
> +      deviceManager.addDevice(device);

* You should use `this._listener.addDevice`
* Previously there is an issue of duplicate device name in the selection prompt. Is this version of patch fixed that as well?

::: dom/presentation/provider/AndroidPresentationDeviceProvider.js:325
(Diff revision 1)
> +  },
> +
> +  classID: Components.ID("{7394f24c-dbc3-48c8-8a47-cd10169b7c6b}"),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
> +                                         Ci.nsIPresentationDeviceProvider,
> +                                         Ci.nsIPresentationControlServerListener]),

remove the `Ci.nsIPresentationControlServerListener`.
Attachment #8796112 - Flags: review?(schien)
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review82422

In addition, test case is required.
Attachment #8796112 - Flags: review-
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review82322

> * compare to `Cr.NS_OK`
> * The disconnect logic looks pretty wrong to me because you'll enter infinite recursion if aReason is not NS_OK. In addition, we didn't call `ctrlChannelListener.notifyDisconnect` at all.

Why does it enter infinite recursion? If `LocalControlChannel.disconnect()` is called with not `Cr.NS_OK`, it calls `AndroidPresentationDeviceProvider.onDisconnect()` and then go to `ChromeRemoteDisplayDevice.disconnect()` to send a message to do the disconnection in Java.

I think if the connection got something wrong like \[1\], `LocalControlChannel.disconnect()` would be called. Isn't it right?

\[1\]: https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationService.cpp#558

> * You should use `this._listener.addDevice`
> * Previously there is an issue of duplicate device name in the selection prompt. Is this version of patch fixed that as well?

> Previously there is an issue of duplicate device name in the selection prompt. Is this version of patch fixed that as well?

What is the issue? I didn't remember I tested that. If we want to test that, maybe I need two Chromecast to do that.
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review82322

> Why does it enter infinite recursion? If `LocalControlChannel.disconnect()` is called with not `Cr.NS_OK`, it calls `AndroidPresentationDeviceProvider.onDisconnect()` and then go to `ChromeRemoteDisplayDevice.disconnect()` to send a message to do the disconnection in Java.
> 
> I think if the connection got something wrong like \[1\], `LocalControlChannel.disconnect()` would be called. Isn't it right?
> 
> \[1\]: https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationService.cpp#558

1. You are right about the infinite recursion part, that's my misunderstanding about the patch.
2. ControlChannel.disconnect() is not only call while error happened, but also called when session-request/reconnect-request/terminate-request is done. NotifyDisconnect callback is the way we know the transaction is finished.

> > Previously there is an issue of duplicate device name in the selection prompt. Is this version of patch fixed that as well?
> 
> What is the issue? I didn't remember I tested that. If we want to test that, maybe I need two Chromecast to do that.

Previously when I tried your WIP, I'll sometimes see duplicate name on deveice selection menu after cancel the device selection several times. It happened with one Chromecast environment so you don't need second one to reproduce it.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #10)
> Comment on attachment 8796112 [details]
> Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on
> Fennec.
> 
> https://reviewboard.mozilla.org/r/82068/#review82322
> 
> 1. You are right about the infinite recursion part, that's my
> misunderstanding about the patch.
> 2. ControlChannel.disconnect() is not only call while error happened, but
> also called when session-request/reconnect-request/terminate-request is
> done. NotifyDisconnect callback is the way we know the transaction is
> finished.

For the second point, I'll do more investigation on that in next week.

> Previously when I tried your WIP, I'll sometimes see duplicate name on
> deveice selection menu after cancel the device selection several times. It
> happened with one Chromecast environment so you don't need second one to
> reproduce it.

Oh, I know what you talking about. That issue is caused by the MediaPlayerManager.java, the video casting support already in Gecko. In bug 1305351, I put the display device management into MediaPlayerManager.java to fix the problem and the race condition of devices.
Whiteboard: [ETA 9/16] → [ETA Fx52]
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review82322

> use `const DIRECTION_SENDER = 1` to prevent string comparison.

I referenced to [1] and used that same way to assign the direction. If we won't use the same way, I think we can use the definition we already have in [2] to avoid duplicate definitions.

[1]: http://searchfox.org/mozilla-central/source/dom/presentation/provider/PresentationControlService.js#473
[2]: http://searchfox.org/mozilla-central/source/dom/presentation/interfaces/nsIPresentationService.idl#51-52
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

Hi smaug, could you review this patch first? And I'll add a test case later.
Flags: needinfo?(bugs)
Attachment #8796112 - Flags: review- → review?(schien)
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review87974

Still found major issue on control channel establishment, especially in the case of initiating on receiver side.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:259
(Diff revision 2)
> +  this._provider = aProvider;
> +  this._id       = aId;
> +  this._name     = aName;
> +  this._windowId = aWindowId;
> +  this._role     = aRole;
> +  Services.obs.addObserver(this, TOPIC_PRESENTATION_VIEW_READY, false);

I'll prefer only add observer while establishControlChannel, in addition you can remove the observer so that device object can be GCed after after device removal.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:286
(Diff revision 2)
> +
> +  establishControlChannel: function CRDD_establishControlChannel() {
> +    this._ctrlChannel = new LocalControlChannel(this._provider,
> +                                                this._id,
> +                                                this._role);
> +

You should not trigger DEVICE_START procedure if establishControlChannel is called on receiver side. In addition, control channel need to call notifyConnected explicitly since no TOPIC_PRESENTATION_VIEW_READY notification in this case.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:296
(Diff revision 2)
> +    }).then(result => {
> +      log("Chromecast is connected.");
> +    }).catch(error => {
> +      log("Can not connect to Chromecast.");
> +      this._ctrlChannel.disconnect(Cr.NS_ERROR_FAILURE);
> +      throw Cr.NS_ERROR_FAILURE;

throw exception in this promise chain doesn't have any effect. should remove it.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:411
(Diff revision 2)
> +
> +        if (!this._deviceList.has(deviceId)) {
> +          let device = new ChromecastRemoteDisplayDevice(this,
> +                                                         deviceInfo.uuid,
> +                                                         deviceInfo.friendlyName,
> +                                                         deviceInfo.uuid,

Here you pass the uuid as the `windowId` parameter.
nsIPresentationLocalDevice.windowId looks useless to me since it's always the same as device.id. Do you recall why we need Id/windowId in the first place?
Attachment #8796112 - Flags: review?(schien) → review-
schien gave r- so I assume I don't need to look at the patch yet, right?
Flags: needinfo?(bugs)
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review88320

r+ with my comment addressed.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:264
(Diff revisions 2 - 3)
>  };
>  
> -function ChromecastRemoteDisplayDevice(aProvider, aId, aName, aWindowId, aRole) {
> +function ChromecastRemoteDisplayDevice(aProvider, aId, aName, aRole) {
>    this._provider = aProvider;
>    this._id       = aId;
> +  this._windowId = aId; // Currently, we use the same value for id and windowId.

don't duplicate the same value into two memeber variable, simply make windowId getter function return `this._id` and all `this._windowId` reference should change to `this.windowId`.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:336
(Diff revisions 2 - 3)
>    },
>  
>    // nsIPresentationLocalDevice
>    get windowId() { return this._windowId; },
>  
>    set windowId(aWindowId) { this._windowId = aWindowId; },

By IDL definition, windowId is readonly. Should remove this setter function.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:343
(Diff revisions 2 - 3)
>    // nsIObserver
>    observe: function CRDD_observe(aSubject, aTopic, aData) {
>      if (aTopic == TOPIC_PRESENTATION_VIEW_READY) {
>        log("ChromecastRemoteDisplayDevice - observe: aTopic="
>            + aTopic + " data=" + aData);
> +      Services.obs.removeObserver(this, TOPIC_PRESENTATION_VIEW_READY);

should move this line in to the following `if` block.
Attachment #8796112 - Flags: review?(schien) → review+
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

ask @smaug for second review.
Attachment #8796112 - Flags: review?(bugs)
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review88514

I'm not sure I'm the right person to review this.
schien's review for AndroidCastDeviceProvider.js is probably enough, and someone familiar with Android stuff should take a look at  MediaPlayerManager.java
Perhaps same reviewers as in Bug 1285870?

But please explain why addObserver usage doesn't cause leaks, or fix it.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:353
(Diff revision 4)
> +};
> +
> +function AndroidCastDeviceProvider() {
> +  // observer registration
> +  Services.obs.addObserver(this, TOPIC_ANDROID_CAST_DEVICE_ADDED, false);
> +  Services.obs.addObserver(this, TOPIC_ANDROID_CAST_DEVICE_REMOVED, false);

So, we keep AndroidCastDeviceProvider instances alive forever. Is that expected? Or should someone call removeObserver?
Or, could we pass true as the last param to addObserver?
Attachment #8796112 - Flags: review?(bugs)
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review88520

::: dom/presentation/provider/AndroidCastDeviceProvider.js:294
(Diff revision 4)
> +                                                this._role);
> +
> +    if (this._role == Ci.nsIPresentationService.ROLE_CONTROLLER) {
> +      // Only connect to Chromecast for controller.
> +      // Monitor the receiver being ready.
> +      Services.obs.addObserver(this, TOPIC_PRESENTATION_VIEW_READY, false);

Is it guaranteed that TOPIC_PRESENTATION_VIEW_READY will happen at some point, or will we leak here?
(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8796112 [details]
> Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on
> Fennec.
> 
> https://reviewboard.mozilla.org/r/82068/#review88514
> 
> I'm not sure I'm the right person to review this.
> schien's review for AndroidCastDeviceProvider.js is probably enough, and
> someone familiar with Android stuff should take a look at 
> MediaPlayerManager.java
> Perhaps same reviewers as in Bug 1285870?
> 
I added @snorp for reviewing the Java changes.
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review87974

> You should not trigger DEVICE_START procedure if establishControlChannel is called on receiver side. In addition, control channel need to call notifyConnected explicitly since no TOPIC_PRESENTATION_VIEW_READY notification in this case.

Can we know `establishControlChannel` is called for termination? For receiver, I would call `notifyConnected` here. And for controller, I think Chromecast would be already connected, and it will send `TOPIC_PRESENTATION_VIEW_READY` directly.

> Here you pass the uuid as the `windowId` parameter.
> nsIPresentationLocalDevice.windowId looks useless to me since it's always the same as device.id. Do you recall why we need Id/windowId in the first place?

For 1-UA, we need an top-level window as the receiver. So we need a window ID to identify which window belongs to this device. In current implementation, we just use the same value with ID.
Please ignore comment 26.
Did review?(snorp@snorp.net) → review?(bugs@pettay.fi) happen by accident? Or do you want me to review the .js part?
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review89270

::: dom/presentation/provider/AndroidCastDeviceProvider.js:83
(Diff revision 5)
> +  get correspondingControlChannel() {
> +    return this._correspondingControlChannel;
> +  },
> +
> +  notifyConnected: function LCC_notifyConnected() {
> +    this._pendingDisconnect = null;

I don't understand _pendingDisconnect usage. Here it is null, and it is also initially set to null.

::: dom/presentation/provider/AndroidCastDeviceProvider.js:243
(Diff revision 5)
> +        this._pendingDisconnect != Cr.NS_OK) {
> +      aReason = this._pendingDisconnect;
> +    }
> +
> +    if (!this._listener) {
> +      this._pendingDisconnect = aReason;

but here an nsresult value is assigned to it.

It is unclear what its meaning is. Is it a boolean or nsresult or what? (nsresult is a number).

Should all the _pendingDisconnect = null; be
_pendingDisconnect = Cr.NS_OK ?

::: dom/presentation/provider/AndroidCastDeviceProvider.js:294
(Diff revision 5)
> +                                                this._role);
> +
> +    if (this._role == Ci.nsIPresentationService.ROLE_CONTROLLER) {
> +      // Only connect to Chromecast for controller.
> +      // Monitor the receiver being ready.
> +      Services.obs.addObserver(this, TOPIC_PRESENTATION_VIEW_READY, false);

Is it guaranteed that TOPIC_PRESENTATION_VIEW_READY notification will fire, or that the promise below will be rejected?
Could you explain why, and add a comment here.

If it is not guaranteed, observer should use weakref.

We don't want leaks. They not only cause more memory usage but make browser slower since CC/GC times increase.
Attachment #8796112 - Flags: review?(bugs) → review-
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review88520

> Is it guaranteed that TOPIC_PRESENTATION_VIEW_READY will happen at some point, or will we leak here?

I add the code to remove this observer if Chromecast is fail to launch. If Chromecast is successful to launch, `TOPIC_PRESENTATION_VIEW_READY` will happened at some point.
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review89270

> I don't understand _pendingDisconnect usage. Here it is null, and it is also initially set to null.

I just think I should clear the pending disconnect if it is ready to connect.

> but here an nsresult value is assigned to it.
> 
> It is unclear what its meaning is. Is it a boolean or nsresult or what? (nsresult is a number).
> 
> Should all the _pendingDisconnect = null; be
> _pendingDisconnect = Cr.NS_OK ?

It is a nsresult or null. If it is null, it means no pending disconnect. If it is NS_OK, it means control channel disconnects normally. And other nsresult values mean control channel disconnects abnormally.

> Is it guaranteed that TOPIC_PRESENTATION_VIEW_READY notification will fire, or that the promise below will be rejected?
> Could you explain why, and add a comment here.
> 
> If it is not guaranteed, observer should use weakref.
> 
> We don't want leaks. They not only cause more memory usage but make browser slower since CC/GC times increase.

In bug 1305352, `TOPIC_PRESENTATION_VIEW_READY` will be fired when PresentationView.(xul/js) is loaded. In [1], if remote display failed to launch, callback.sendError will make the promise below be rejected. And I think using weakref is also a safe way to prevent some abnormal behavior on Chromecast. Because it is still the experimental function.

[1] http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java#102
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review90104

Did you test this patch works without nsISupportsWeakReference in QI?

::: dom/presentation/provider/AndroidCastDeviceProvider.js:294
(Diff revision 6)
> +                                                this._role);
> +
> +    if (this._role == Ci.nsIPresentationService.ROLE_CONTROLLER) {
> +      // Only connect to Chromecast for controller.
> +      // Monitor the receiver being ready.
> +      Services.obs.addObserver(this, TOPIC_PRESENTATION_VIEW_READY, true);

Why does this work when ChromecastRemoteDisplayDevice misses nsISupportsWeakReference in its QI implementation?
Attachment #8796112 - Flags: review?(bugs) → review-
(In reply to Tommy Kuo [:KuoE0] from comment #31)

> It is a nsresult or null. If it is null, it means no pending disconnect. If
> it is NS_OK, it means control channel disconnects normally. And other
> nsresult values mean control channel disconnects abnormally.
Please document this in the code.
Hi Jim, in AndroidCastDeviceProvider.js, I use Messaging.jsm to send some request. And I'm writing the test case for this provider, is it possible to mock Messaging.jsm on xpcshell test? I think we can't use the original Messaging.jsm on xpcshell test, because Messaging.sendRequest will call a JNI function. I tried to mock nsIAndroidBridge, but there is [implicit_jscontext] mark on handleGeckoMessage, I can't mock it with JS.
Flags: needinfo?(nchen)
(In reply to Tommy Kuo [:KuoE0] from comment #36)
> Hi Jim, in AndroidCastDeviceProvider.js, I use Messaging.jsm to send some
> request. And I'm writing the test case for this provider, is it possible to
> mock Messaging.jsm on xpcshell test? I think we can't use the original
> Messaging.jsm on xpcshell test, because Messaging.sendRequest will call a
> JNI function. I tried to mock nsIAndroidBridge, but there is
> [implicit_jscontext] mark on handleGeckoMessage, I can't mock it with JS.

Can you set `Messaging.sendRequest` to your own stub function? If nothing works, you can always convert the test to a Robocop JS test, which will have Java access.
Flags: needinfo?(nchen)
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review90352
Attachment #8796112 - Flags: review?(bugs) → review+
Comment on attachment 8796112 [details]
Bug 1295087 - Implement PresentationDeviceProvider for Chromecast devices on Fennec.

https://reviewboard.mozilla.org/r/82068/#review90372
Attachment #8796112 - Flags: review?(snorp) → review+
See Also: → 1316538
The robocop test will be implemented in Bug 1316538. It is more complicated than what I thought.
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a2c98b982b9
Implement PresentationDeviceProvider for Chromecast devices on Fennec. r=schien,smaug,snorp
https://hg.mozilla.org/mozilla-central/rev/0a2c98b982b9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: