1235123 - [Presentation WebAPI] Receive PresentationRequestUIGlue events from shell-remote

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Tommy Kuo (away forever...)]

PresentationRequestUIGlue.js cannot add an event listener in shell-remote. So we need to use observer pattern to communicate between shell-remote and PresentationRequestUIGlue.js.

Comment 1

[Tommy Kuo (away forever...)]

Attachment: Bug-1235123-Part-1-hg.patch

This part makes shell-remote be able to listen app launched event.

Comment 2

[Tommy Kuo (away forever...)]

Attachment: Bug-1235123-Part-2-hg.patch

This part make PresentationRequestUIGlue.js observe the presentation-ui-response event from shell-remote.

Comment 3

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

Comment on attachment 8701950 [details] [diff] [review]
Bug-1235123-Part-2-hg.patch

Review of attachment 8701950 [details] [diff] [review]:
-----------------------------------------------------------------

I'm thinking that we should make SystemAppProxy.jsm be able to listen the mozContentEvent from other top level window.

@fabrice, gecko will launch the content on another top-level window and Gecko need to know the <iframe> that loaded the receiver page/app for Presentation API. Currently in this patch we use shell-remote.js to translate the mozContentEvent to observer notification, but I'm thinking we should instead create some API in SystemAppProxy for dispatching the event. How do you think?

::: b2g/components/PresentationRequestUIGlue.js
@@ +6,5 @@
>  
>  "use strict"
>  
>  function debug(aMsg) {
> +  // dump("-*- PresentationRequestUIGlue: " + aMsg + "\n");

no need to make change in this line.

@@ +52,5 @@
>                                          id: aSessionId });
> +    });
> +  },
> +
> +  continueCallback: function(aDetail) {

name it "appLaunchCallback" should be more clear.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #3)
> Comment on attachment 8701950 [details] [diff] [review]
> Bug-1235123-Part-2-hg.patch
> 
> Review of attachment 8701950 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm thinking that we should make SystemAppProxy.jsm be able to listen the
> mozContentEvent from other top level window.
> 
> @fabrice, gecko will launch the content on another top-level window and
> Gecko need to know the <iframe> that loaded the receiver page/app for
> Presentation API. Currently in this patch we use shell-remote.js to
> translate the mozContentEvent to observer notification, but I'm thinking we
> should instead create some API in SystemAppProxy for dispatching the event.
> How do you think?

I agree, that would be better.

Comment 5

[Tommy Kuo (away forever...)]

Attachment: Part 1 - Make SystemAppProxy can handle multiple system app

I modified SystemAppProxy.jsm to make it can handle multiple system(-remote) app.

Comment 6

[Tommy Kuo (away forever...)]

Attachment: Part 2 - Update architecture of shell_remote.js to align shell.js

I modified system_remote.js to make it align shell.js.

Comment 7

[Tommy Kuo (away forever...)]

Attachment: Part 3 - Update interfaces and implantations of nsIPresentationDevic and nsIPresentationUIGlue to support 1-UA use case

There is some modifications:

- Add a property named "isLocal" in nsIPresentationDevice. This property can be used to check this presentation is 1-UA or 2-UA.
- Add a method name "sendLocalRequest" in nsIPresentationUIGlue. It is almost like "sendRequest", but it add the event listener to other system(-remote) app not main system app.

Comment 8

[Tommy Kuo (away forever...)]

Hi S.C, could you give some feedback on these patches? If you think this architecture is okay, I'll send the review request to :frabice.

Comment 9

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

Comment on attachment 8731078 [details] [diff] [review]
Part 1 - Make SystemAppProxy can handle multiple system app

Review of attachment 8731078 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/SystemAppProxy.jsm
@@ +20,5 @@
>    _pendingListeners: [],
>  
> +  // Get frameInfo by ID of system(-remote) app.
> +  // - ID of main system app is undefined.
> +  // - ID of system-remote app is defined themselves.

You can use a map directly by giving a predefined ID for main system app. This can save some code.
BTW, ID is too vague for people to understand the source, maybe use windowId/frameId/screenId?

@@ +182,4 @@
>          throw new Error("no content window");
>        }
>  
> +      let content = frameInfo.frame.contentWindow;

you'll need to check if content is null or not, to preserve the meaning of original code.
maybe you can add a getter function for main system app frame, so that you don't need to change code that much. And it'll be more readable because invoking getFrameInfoById() without parameter has too much implication.

@@ +252,5 @@
>      }
>    },
>  
> +  // Get all frame in system app
> +  getFrames: function systemApp_getFrames(id) {

getFrames is used in various scenario and it probably wants to generate the list form all system apps. You'll need to double check with all current use cases [1].

[1] https://dxr.mozilla.org/mozilla-central/source/b2g/components/Frames.jsm

Comment 10

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

please see my feedback in comment #9.

Comment 11

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

Comment on attachment 8731080 [details] [diff] [review]
Part 2 - Update architecture of shell_remote.js to align shell.js

Review of attachment 8731080 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/shell_remote.js
@@ +66,5 @@
>  
>    stop: function () {
> +    this.contentBrowser.removeEventListener('mozbrowserloadstart', this);
> +    this.contentBrowser.removeEventListener('mozbrowserlocationchange', this, true);
> +  },

You'll need to monitor unload event and call stop like shell.js, see https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#430

Comment 12

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

Comment on attachment 8731081 [details] [diff] [review]
Part 3 - Update interfaces and implantations of nsIPresentationDevic and nsIPresentationUIGlue to support 1-UA use case

Review of attachment 8731081 [details] [diff] [review]:
-----------------------------------------------------------------

no need to change UUID for IDL interface change anymore, see https://groups.google.com/forum/#!searchin/mozilla.dev.platform/stop$20revving$20UUIDs/mozilla.dev.platform/HE1_qZhPj1I/rElS7KjXAQAJ .

::: b2g/components/PresentationRequestUIGlue.js
@@ +1,2 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- /
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */

Use the same mode line as in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

@@ +57,3 @@
>  
> +      // Listen to the result for the opened iframe from front-end.
> +      SystemAppProxy.addEventListenerById(aDeviceId,

can we have explicit mapping between device Id and display Id?

@@ +81,5 @@
> +                                                 this);
> +        }
> +        else {
> +          SystemAppProxy.removeEventListener('mozPresentationContentEvent', this);
> +        }

Definitely need explicit mapping among device Id, display Id, and display type, sending out ChromeEvent with device Id and get back a display Id from ContentEvent is not readable. In addition, we should store the deviceId/displayId with sessionId without depending Gaia piggyback the information.

::: dom/presentation/PresentationService.cpp
@@ +341,5 @@
>    mSessionInfoAtReceiver.Put(sessionId, info);
>  
> +  // Check this presentation is 1-UA or 2-UA.
> +  bool isLocalPresentation;
> +  device->GetIsLocal(&isLocalPresentation);

matching the local variable name with the getter function.

@@ +356,5 @@
> +    nsAutoCString deviceId;
> +    device->GetId(deviceId);
> +    rv = glue->SendLocalRequest(url,
> +                                sessionId,
> +                                NS_ConvertASCIItoUTF16(deviceId),

NS_ConvertUTF8toUTF16 here

::: dom/presentation/interfaces/nsIPresentationDevice.idl
@@ +22,5 @@
>    // The category of this device, could be "wifi", "bluetooth", "hdmi", etc.
>    readonly attribute AUTF8String type;
>  
> +  // The property is used to identify this device is for 1-UA or 2-UA.
> +  readonly attribute boolean isLocal;

s/isLocal/isLocalRendering

::: dom/presentation/interfaces/nsIPresentationRequestUIGlue.idl
@@ +13,5 @@
>  interface nsIPresentationRequestUIGlue : nsISupports
>  {
>    /*
> +   * (2-UA) This method is called to open the responding app/page when
> +   * a presentation request comes in at receiver side.

For 2-UAs scenario, this...

@@ +23,5 @@
>     */
>    nsISupports sendRequest(in DOMString url, in DOMString sessionId);
> +  /*
> +   * (1-UA) This method is called to open the responding app/page when
> +   * a presentation request comes in at receiver side.

For 1-UA scenario, this method is called to open the responding app/page in separate top level window.

nit: add new line before comment.

@@ +31,5 @@
> +   * @param deviceId  The ID of selected device.
> +   *
> +   * @return A promise that resolves to the opening frame.
> +   */
> +  nsISupports sendLocalRequest(in DOMString url, in DOMString sessionId,

s/sendLocalRequest/sendLocalRenderingRequest

::: dom/presentation/provider/HDMIDisplayProvider.cpp
@@ +87,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +HDMIDisplayProvider::HDMIDisplayDevice::GetIsLocal(bool* aRet)

s/aRet/aRetVal

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +993,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +MulticastDNSDeviceProvider::Device::GetIsLocal(bool* aRet)

s/aRet/aRetVal

Comment 13

[Tommy Kuo (away forever...)]

Attachment: [Part 1] Make SystemAppProxy can handle multiple system app

Comment 14

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Update architecture of shell_remote.js to align shell.js

Comment 15

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Update interfaces and implantations of nsIPresentationDevic and nsIPresentationUIGlue to support 1-UA use case

Comment 16

[Tommy Kuo (away forever...)]

Hi S.C., I updated the patches. Could you review it again?

Part 1.
I decide to create a new function for each original function for the main system app.

Part 2.
AddEventListener/RemoveEventListener for 'unload' event.

Part 3.
I add a parameter "device" for sendRequest. If we have the device, we can know the info of 1-UA/2-UA and device ID. So, we don't need get the ID from Gaia side.

Comment 17

[Tommy Kuo (away forever...)]

BTW, I named the method "sendCustomEventFromId" instead of "_sendCustomEventFromId". I think this method is a public method can be used by other components. And I also think we should change the method named "_sendCustomEvent" to "sendCustomEvent" with the same reason. I found Bug 1199025 is about that. I intend to fix it after this bug fixed.

Comment 18

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

Comment on attachment 8733745 [details] [diff] [review]
[Part 3] Update interfaces and implantations of nsIPresentationDevic and nsIPresentationUIGlue to support 1-UA use case

Review of attachment 8733745 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/PresentationRequestUIGlue.js
@@ +3,3 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
> + *  * License, v. 2.0. If a copy of the MPL was not distributed with this
> + *   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: unnecessary modification.

@@ +53,5 @@
> +        SystemAppProxy.sendCustomEventFromId(aDevice.id,
> +                                            "mozPresentationChromeEvent",
> +                                            { type: "presentation-launch-receiver",
> +                                              url: aUrl,
> +                                              id: aSessionId });

In this patch you change form one global listener to listener per request. You can leverage function scope to bind sessionId, resolver, and device without storing it in a map.

@@ +55,5 @@
> +                                            { type: "presentation-launch-receiver",
> +                                              url: aUrl,
> +                                              id: aSessionId });
> +      }
> +      else {

nit: no need to put 'else {' in new line.

@@ +72,5 @@
> +    debug('Got a ' + evt.type + ' event.');
> +    switch(evt.type) {
> +      case 'mozPresentationContentEvent':
> +        let sessionId = evt.detail.id;
> +        let device = this._devices[sessionId];

need delete this._devices[sessionId]; at some point.

Comment 19

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

Comment on attachment 8733743 [details] [diff] [review]
[Part 1] Make SystemAppProxy can handle multiple system app

Review of attachment 8733743 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with some minor modification. I'll need you to perform some manual test to see if anything breaks.

::: b2g/components/SystemAppProxy.jsm
@@ +12,5 @@
>  Cu.import('resource://gre/modules/Services.jsm');
>  
>  this.EXPORTED_SYMBOLS = ['SystemAppProxy'];
>  
> +const mainSystemAppId = 'main';

s/mainSystemAppId/kMainSystemAppId

@@ +45,5 @@
> +                      frame: frame };
> +
> +    this._frameInfoMap.set(frameId, frameInfo);
> +
> +    // Register all DOM event listeners added before we got a ref to 

nit: trailing space here and elsewhere.

@@ +85,3 @@
>    },
>  
> +  // To call when the load event of the MAIN system app document is triggered.

s/MAIN/main here and elsewhere.

@@ +272,4 @@
>          throw new Error("no content window");
>        }
>  
> +      let content = frameInfo.frame.contentWindow;

Your code doesn't preserve the original logic. Please consider using following example:

let frameInfo = this._getMainSystemAppInfo();

let content = (frameInfo && frameInfo.frame) ? frameInfo.frame.contentWindow
                                             : null;

if (!content) {
  ...
}

@@ +348,5 @@
>      }
>    },
>  
> +  // Get all frame in system app
> +  getFrames: function systemApp_getFrames(frameId) {

You'll need to check if WebIDE can correctly list all the browser tabs after applying your patch.

Comment 20

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

Comment on attachment 8733743 [details] [diff] [review]
[Part 1] Make SystemAppProxy can handle multiple system app

Review of attachment 8733743 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/SystemAppProxy.jsm
@@ +29,3 @@
>  
> +  // To call when a new system(-remote) app iframe is created with ID
> +  registerFrameWithId: function systemApp_registerFrameWithId(frameId,

BTW, all function names that end with "WithId" seems opaque to me. I can live with it but I'll prefer a better name that identifies what kind of "Id" is used.

I'd like @fabrice to make the final call about the naming.

Comment 21

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

Comment on attachment 8733744 [details] [diff] [review]
[Part 2] Update architecture of shell_remote.js to align shell.js

Review of attachment 8733744 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't match with your description. I guess you put the wrong one here.

Comment 22

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

Comment on attachment 8733745 [details] [diff] [review]
[Part 3] Update interfaces and implantations of nsIPresentationDevic and nsIPresentationUIGlue to support 1-UA use case

Review of attachment 8733745 [details] [diff] [review]:
-----------------------------------------------------------------

Abnormal scenario should be take into consider and I think you'll need some error handling in this patch:

1. HDMI plugged in.
2. presentation request is fired on HDMI device
3. HDMI unplugged while 1) before ChromeEvent is sent
                        2) before ContentEvent is received
                        3) before page loading complete

Comment 23

[Tommy Kuo (away forever...)]

Attachment: [Part 1] Make SystemAppProxy can handle multiple system app

Comment 24

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Update architecture of shell_remote.js to align shell.js

Comment 25

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Comment 26

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Comment 27

[Tommy Kuo (away forever...)]

Attachment: [Part 1] Make SystemAppProxy can handle multiple system app

Comment 28

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Update architecture of shell_remote.js to align shell.js

Comment 29

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Comment 30

[Tommy Kuo (away forever...)]

Hi S.C., I update the patch to prevent the abnormal behavior you mentioned. 

Part 1.
Add a function "unregisterFrameWithId" to remove system(-remote) app when top level window closed.

Part 2.
Call "unregisterFrameWithId" when stop.

Part 3.
When HDMI cable is unplugged, systme-remote app is closed and "unload" event is emitted.

Listen "unload" event in PresentationUIGlue. When "unload" is received, performs "reject" action. I think no matter when the "unload" event comming, it can ensure that the "reject" action performed normally.

---

> @@ +348,5 @@
> >      }
> >    },
> >  
> > +  // Get all frame in system app
> > +  getFrames: function systemApp_getFrames(frameId) {
> 
> You'll need to check if WebIDE can correctly list all the browser tabs after applying your patch.

In addition, I checked the issue you mentioned in comment 19. And WebIDE can list all frames include the system-remote app correctly.

Comment 31

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

Comment on attachment 8735395 [details] [diff] [review]
[Part 3] Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Review of attachment 8735395 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/PresentationRequestUIGlue.js
@@ +60,5 @@
> +      SystemAppProxy.removeEventListenerWithId(aDevice.windowId,
> +                                               "unload",
> +                                               handle_unload);
> +      this.appLaunchCallback({ id: aSessionId,
> +                               type: 'presentation-device-disconnected' });

nit: use double quotation mark (") here and elsewhere.

@@ +61,5 @@
> +                                               "unload",
> +                                               handle_unload);
> +      this.appLaunchCallback({ id: aSessionId,
> +                               type: 'presentation-device-disconnected' });
> +    }

Why not use single callback function for both event? You'll need to remove both listener if one of the event is fired.

Comment 32

[Tommy Kuo (away forever...)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #31)
> @@ +61,5 @@
> > +                                               "unload",
> > +                                               handle_unload);
> > +      this.appLaunchCallback({ id: aSessionId,
> > +                               type: 'presentation-device-disconnected' });
> > +    }
> 
> Why not use single callback function for both event? You'll need to remove
> both listener if one of the event is fired.

Because I think it will make the lines too long (see the following code). But if you think it's okay, I'll revise to that. I don't know which way is better. Or, is there another suggestion for indentation?

>     // If system(-remote) app is closed.
>     SystemAppProxy.addEventListenerWithId(aDevice.windowId,
>                                           "unload",
>                                           function handle_unload() {
>                                             SystemAppProxy.removeEventListenerWithId(aDevice.windowId,
>                                               "unload",
>                                               handle_unload);
>                                             this.appLaunchCallback({ id: aSessionId,
>                                                                      type: 'presentation-device-disconnected' });
>                                           });

Comment 33

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

(In reply to Tommy Kuo [:KuoE0] from comment #32)

No, what I mean is like following code:

>sendTo1UA: function(aUrl, aSessionId, aDevice) {
>  return new Promise((resolve, reject) => {
>    function handler(evt) {
>      SystemAppProxy.removeEventListenerWithId(aDevice.windowId, "unload", handler);
>      SystemAppProxy.removeEventListenerWithId(aDevice.windowId,
>                                               "mozPresentationContentEvent",
>                                               handler);
>      if (evt.type === "unload") {
>        reject();
>      }
>      if (evt.type !== "mozPresentationContentEvent" ||
>          evt.detail.id !== aSessionId) {
>        return;
>      }
>      appLauncheCallback(evt.detail, resolve, reject);
>    }
>    SystemAppProxy.addEventListenerWithId(aDevice.windowId,
>                                          "unload",
>                                          handler);
>    // Listen to the result for the opened iframe from front-end.
>    SystemAppProxy.addEventListenerWithId(aDevice.windowId,
>                                          "mozPresentationContentEvent",
>                                          handler);
>    SystemAppProxy.sendCustomEventWithId(aDevice.windowId,	
>                                         "mozPresentationChromeEvent",
>                                         { type: "presentation-launch-receiver",
>                                           url: aUrl, id: aSessionId });
>  });
>}

Comment 34

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

Comment on attachment 8735395 [details] [diff] [review]
[Part 3] Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Review of attachment 8735395 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/PresentationRequestUIGlue.js
@@ +89,5 @@
> +      }
> +    }
> +    // Listen to the result for the opened iframe from front-end.
> +    SystemAppProxy.addEventListener("mozPresentationContentEvent",
> +                                    handle_mozPresentationContentEvent.bind(this));

|bind| will create a new function object. Use |handle_mozPresentationContentEvent| to remove listener will have no effect.

Comment 35

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

Comment on attachment 8735393 [details] [diff] [review]
[Part 1] Make SystemAppProxy can handle multiple system app

Review of attachment 8735393 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

Comment 36

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

Comment on attachment 8735394 [details] [diff] [review]
[Part 2] Update architecture of shell_remote.js to align shell.js

Review of attachment 8735394 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm!

::: b2g/chrome/content/shell_remote.js
@@ +60,5 @@
>      let container = document.getElementById("container");
>      this.contentBrowser = container.appendChild(systemAppFrame);
>      this.contentBrowser.src = homeURL + window.location.hash;
> +
> +    window.addEventListener('unload', this);

nit: use double quotation mark here and elsewhere.

Comment 37

[Tommy Kuo (away forever...)]

Attachment: [Part 1] (v2) Make SystemAppProxy can handle multiple system app

Use double quotation mark for strings.

Comment 38

[Tommy Kuo (away forever...)]

Attachment: [Part 2] (v2) Update architecture of shell_remote.js to align shell.js

Use double quotation mark for strings.

Comment 39

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v2) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Update the architecture for PresentationRequestUIGlue.js. We don't need to use an object to store resolvers now.

Comment 40

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

Comment on attachment 8736273 [details] [diff] [review]
[Part 3] (v2) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Review of attachment 8736273 [details] [diff] [review]:
-----------------------------------------------------------------

Three level of Promise seems overkill. You only need create one Promise at sendTo1UA/sendTo2UA, other function can be sync function call.

Comment 41

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v3) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Hi S.C., I updated the patch. Use only one promise in sendTo1UA/sendTo2UA.

Comment 42

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v4) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Use 'let' instead of 'var' and remove useless code.

Comment 43

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v5) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Hi S.C., I made some updates of the timing to remove listener.

Update:

- Remove handler when "unload" event received no matter who sent it.
- Remove handler when "mozPresentationContentEvent" received and the session ID is same.

Comment 44

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v6) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Update:

- Remove hanlder for both event "unload" and "mozPresentationContentEvent".

Comment 45

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

Comment on attachment 8740794 [details] [diff] [review]
[Part 3] (v6) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Review of attachment 8740794 [details] [diff] [review]:
-----------------------------------------------------------------

f+ with following comments addressed.

::: b2g/components/PresentationRequestUIGlue.js
@@ +55,5 @@
> +          this.appLaunchCallback(evt.detail, aResolve, aReject);
> +        }
> +      };
> +      // If system(-remote) app is closed.
> +      SystemAppProxy.addEventListenerWithId(aDevice.windowId,

Pass the windowId in parameter directly instead of passing device object, so that we don't depend on "aDevice.windowId is never changed before callback".

@@ +73,3 @@
>  
> +  // For 2-UA scenario
> +  sendTo2UA: function(aUrl, aSessionId, aDevice) {

You don't need |aDevice| since it's not used in this function.

Comment 46

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v7) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #45)
> Comment on attachment 8740794 [details] [diff] [review]
> [Part 3] (v6) Update interfaces and implantations of nsIPresentationDevice
> and nsIPresentationUIGlue to support 1-UA use case
> 
> Review of attachment 8740794 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ with following comments addressed.
> 
> ::: b2g/components/PresentationRequestUIGlue.js
> @@ +55,5 @@
> > +          this.appLaunchCallback(evt.detail, aResolve, aReject);
> > +        }
> > +      };
> > +      // If system(-remote) app is closed.
> > +      SystemAppProxy.addEventListenerWithId(aDevice.windowId,
> 
> Pass the windowId in parameter directly instead of passing device object, so
> that we don't depend on "aDevice.windowId is never changed before callback".
> 
> @@ +73,3 @@
> >  
> > +  // For 2-UA scenario
> > +  sendTo2UA: function(aUrl, aSessionId, aDevice) {
> 
> You don't need |aDevice| since it's not used in this function.

Fixed!

Comment 47

[[:fabrice] Fabrice Desré]

Comment on attachment 8736270 [details] [diff] [review]
[Part 1] (v2) Make SystemAppProxy can handle multiple system app

Review of attachment 8736270 [details] [diff] [review]:
-----------------------------------------------------------------

redirecting to S.C, but please revert your changes to the string quotes style.

Comment 48

[[:fabrice] Fabrice Desré]

Comment on attachment 8736271 [details] [diff] [review]
[Part 2] (v2) Update architecture of shell_remote.js to align shell.js

Review of attachment 8736271 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, but let's get r+ from S.C

Comment 49

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

Comment on attachment 8736271 [details] [diff] [review]
[Part 2] (v2) Update architecture of shell_remote.js to align shell.js

Review of attachment 8736271 [details] [diff] [review]:
-----------------------------------------------------------------

remember to update the commit comment for the change of reviewer.

Comment 50

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

Comment on attachment 8736270 [details] [diff] [review]
[Part 1] (v2) Make SystemAppProxy can handle multiple system app

Review of attachment 8736270 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with @fabrice's comment addressed.

Comment 51

[Tommy Kuo (away forever...)]

Attachment: [Part 1] (v3) Make SystemAppProxy can handle multiple system app

Update reviewer's name and use single quote for string. Carry r+ from comment #50.

Comment 52

[Tommy Kuo (away forever...)]

Attachment: [Part 2] (v3) Update architecture of shell_remote.js to align shell.js

Update reviewer's name in commit message. Carry r+ from comment #49.

Comment 53

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v8) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Update reviewer's name in commit message.

Comment 54

[Tommy Kuo (away forever...)]

Attachment: [Part 2] (v4) Update architecture of shell_remote.js to align shell.js

Fix typo in commit message. Carry r+ from comment #49.

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3245a8dfacd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2389fb0248db
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ec1d8e5504

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3245a8dfacd
https://hg.mozilla.org/mozilla-central/rev/2389fb0248db
https://hg.mozilla.org/mozilla-central/rev/50ec1d8e5504

Comment 57

[Tommy Kuo (away forever...)]

Comment on attachment 8741408 [details] [diff] [review]
[Part 2] (v4) Update architecture of shell_remote.js to align shell.js

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1235123
User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g.
Testing completed: manual testing passed
Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch.
String or UUID changes made by this patch: uuid updated

Comment 58

[Tommy Kuo (away forever...)]

Comment on attachment 8741406 [details] [diff] [review]
[Part 3] (v8) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1235123
User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g.
Testing completed: manual testing passed
Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch.
String or UUID changes made by this patch: uuid updated

Comment 59

[Tommy Kuo (away forever...)]

Comment on attachment 8741403 [details] [diff] [review]
[Part 1] (v3) Make SystemAppProxy can handle multiple system app

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1235123
User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g.
Testing completed: manual testing passed
Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch.
String or UUID changes made by this patch: uuid updated

Comment 60

[Josh Cheng [:josh]]

Comment on attachment 8741403 [details] [diff] [review]
[Part 1] (v3) Make SystemAppProxy can handle multiple system app

Approve for TV 2.6

Comment 61

[Josh Cheng [:josh]]

Comment on attachment 8741406 [details] [diff] [review]
[Part 3] (v8) Update interfaces and implantations of nsIPresentationDevice and nsIPresentationUIGlue to support 1-UA use case

Approve for TV 2.6

Comment 62

[Josh Cheng [:josh]]

Comment on attachment 8741408 [details] [diff] [review]
[Part 2] (v4) Update architecture of shell_remote.js to align shell.js

Approve for TV 2.6

Comment 63

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/8b500eab52c5e223dccc4ac04aac4589696a694f