Closed Bug 1235123 Opened 4 years ago Closed 3 years ago

[Presentation WebAPI] Receive PresentationRequestUIGlue events from shell-remote

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.6+, firefox49 fixed, b2g-v2.6 fixed)

RESOLVED FIXED
blocking-b2g 2.6+
Tracking Status
firefox49 --- fixed
b2g-v2.6 --- fixed

People

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

References

(Blocks 1 open bug)

Details

(Whiteboard: [ft:conndevices])

Attachments

(3 files, 24 obsolete files)

16.84 KB, patch
kuoe0.tw
: review+
Details | Diff | Splinter Review
9.14 KB, patch
schien
: review+
Details | Diff | Splinter Review
5.09 KB, patch
kuoe0.tw
: review+
Details | Diff | Splinter Review
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.
Attached patch Bug-1235123-Part-1-hg.patch (obsolete) — Splinter Review
This part makes shell-remote be able to listen app launched event.
Assignee: nobody → kuoe0
Attached patch Bug-1235123-Part-2-hg.patch (obsolete) — Splinter Review
This part make PresentationRequestUIGlue.js observe the presentation-ui-response event from shell-remote.
Attachment #8701950 - Flags: review?(schien)
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.
Attachment #8701950 - Flags: review?(schien) → feedback?(fabrice)
(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.
Attachment #8701950 - Flags: feedback?(fabrice)
I modified SystemAppProxy.jsm to make it can handle multiple system(-remote) app.
Attachment #8701947 - Attachment is obsolete: true
Attachment #8701950 - Attachment is obsolete: true
Attachment #8731078 - Flags: feedback?(schien)
I modified system_remote.js to make it align shell.js.
Attachment #8731080 - Flags: feedback?(schien)
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.
Attachment #8731081 - Flags: feedback?(schien)
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.
Flags: needinfo?(schien)
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
Attachment #8731078 - Flags: feedback?(schien)
please see my feedback in comment #9.
Flags: needinfo?(schien)
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
Attachment #8731080 - Flags: feedback?(schien) → feedback+
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
Attachment #8731081 - Flags: feedback?(schien) → feedback-
Attachment #8731078 - Attachment is obsolete: true
Attachment #8733743 - Flags: feedback?(schien)
Attachment #8731080 - Attachment is obsolete: true
Attachment #8733744 - Flags: feedback?(schien)
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.
Flags: needinfo?(schien)
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 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.
Attachment #8733745 - Flags: feedback?(schien)
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.
Attachment #8733743 - Flags: feedback?(schien) → feedback+
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 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.
Attachment #8733744 - Flags: feedback?(schien)
Flags: needinfo?(schien)
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
Attachment #8733743 - Attachment is obsolete: true
Attachment #8735375 - Flags: feedback?(schien)
Attachment #8733744 - Attachment is obsolete: true
Attachment #8735376 - Flags: feedback?(schien)
Attachment #8735375 - Attachment is obsolete: true
Attachment #8735375 - Flags: feedback?(schien)
Attachment #8735393 - Flags: review?(fabrice)
Attachment #8735393 - Flags: feedback?(schien)
Attachment #8735376 - Attachment is obsolete: true
Attachment #8735376 - Flags: feedback?(schien)
Attachment #8735394 - Flags: review?(fabrice)
Attachment #8735394 - Flags: feedback?(schien)
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.
Flags: needinfo?(schien)
Attachment #8735385 - Attachment is obsolete: true
Attachment #8735385 - Flags: feedback?(schien)
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.
Attachment #8735395 - Flags: feedback?(schien)
(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' });
>                                           });
(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 });
>  });
>}
Flags: needinfo?(schien)
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 on attachment 8735393 [details] [diff] [review]
[Part 1] Make SystemAppProxy can handle multiple system app

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

lgtm.
Attachment #8735393 - Flags: feedback?(schien) → feedback+
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.
Attachment #8735394 - Flags: feedback?(schien) → feedback+
Use double quotation mark for strings.
Attachment #8735393 - Attachment is obsolete: true
Attachment #8735393 - Flags: review?(fabrice)
Attachment #8736270 - Flags: review?(fabrice)
Use double quotation mark for strings.
Attachment #8735394 - Attachment is obsolete: true
Attachment #8735394 - Flags: review?(fabrice)
Attachment #8736271 - Flags: review?(fabrice)
Update the architecture for PresentationRequestUIGlue.js. We don't need to use an object to store resolvers now.
Attachment #8735395 - Attachment is obsolete: true
Attachment #8735395 - Flags: review?(fabrice)
Attachment #8736273 - Flags: review?(fabrice)
Attachment #8736273 - Flags: feedback?(schien)
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.
Attachment #8736273 - Flags: feedback?(schien)
Hi S.C., I updated the patch. Use only one promise in sendTo1UA/sendTo2UA.
Attachment #8736273 - Attachment is obsolete: true
Attachment #8736273 - Flags: review?(fabrice)
Flags: needinfo?(schien)
Attachment #8739345 - Flags: review?(fabrice)
Attachment #8739345 - Flags: feedback?(schien)
Use 'let' instead of 'var' and remove useless code.
Attachment #8739345 - Attachment is obsolete: true
Attachment #8739345 - Flags: review?(fabrice)
Attachment #8739345 - Flags: feedback?(schien)
Attachment #8739887 - Flags: review?(fabrice)
Attachment #8739887 - Flags: feedback?(schien)
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.
Attachment #8739887 - Attachment is obsolete: true
Attachment #8739887 - Flags: review?(fabrice)
Attachment #8739887 - Flags: feedback?(schien)
Attachment #8740786 - Flags: feedback?(schien)
Update:

- Remove hanlder for both event "unload" and "mozPresentationContentEvent".
Attachment #8740786 - Attachment is obsolete: true
Attachment #8740786 - Flags: feedback?(schien)
Attachment #8740794 - Flags: feedback?(schien)
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.
Attachment #8740794 - Flags: feedback?(schien) → feedback+
Flags: needinfo?(schien)
(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!
Attachment #8740794 - Attachment is obsolete: true
Attachment #8740892 - Flags: feedback?(schien)
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.
Attachment #8736270 - Flags: review?(fabrice) → review?(schien)
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
Attachment #8736271 - Flags: review?(schien)
Attachment #8736271 - Flags: review?(fabrice)
Attachment #8736271 - Flags: feedback+
Attachment #8740892 - Flags: feedback?(schien) → feedback+
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.
Attachment #8736271 - Flags: review?(schien) → review+
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.
Attachment #8736270 - Flags: review?(schien) → review+
Update reviewer's name and use single quote for string. Carry r+ from comment #50.
Attachment #8736270 - Attachment is obsolete: true
Attachment #8741403 - Flags: review+
Update reviewer's name in commit message. Carry r+ from comment #49.
Attachment #8736271 - Attachment is obsolete: true
Attachment #8741405 - Flags: review+
Update reviewer's name in commit message.
Attachment #8740892 - Attachment is obsolete: true
Attachment #8741406 - Flags: review?(schien)
Fix typo in commit message. Carry r+ from comment #49.
Attachment #8741405 - Attachment is obsolete: true
Attachment #8741408 - Flags: review+
Attachment #8741406 - Flags: review?(schien) → review+
Depends on: 1208417
Keywords: checkin-needed
Whiteboard: Please check-in after Bug 1208417 landed.
Whiteboard: Please check-in after Bug 1208417 landed.
blocking-b2g: --- → 2.6+
Whiteboard: [ft:conndevices]
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
Flags: needinfo?(jocheng)
Attachment #8741408 - Flags: approval-mozilla-b2g48?
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
Attachment #8741406 - Flags: approval-mozilla-b2g48?
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
Attachment #8741403 - Flags: approval-mozilla-b2g48?
Comment on attachment 8741403 [details] [diff] [review]
[Part 1] (v3) Make SystemAppProxy can handle multiple system app

Approve for TV 2.6
Flags: needinfo?(jocheng)
Attachment #8741403 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
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
Attachment #8741406 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
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
Attachment #8741408 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
You need to log in before you can comment on or make changes to this bug.