908191 - Use xpcom-interface caller to do interaction between Inter-App Communication API and run-time prompt

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

As Fabrice suggested at bug 876397, comment #86, we need to change the way of interaction between API and run-time prompt from observer notification to xpcom-interface caller. ActivitiesGlue.js is a good example.

Comment 1

[Gene Lian (I already quit Mozilla)]

Release this to anyone who is interested and I'm happy to be the mentor.

Comment 2

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch attempt 1

Patch attempt 1

Comment 3

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8424720 [details] [diff] [review]
Patch attempt 1

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

::: dom/apps/src/InterAppCommService.jsm
@@ +540,5 @@
>        requestID: requestID,
>        target: aTarget
>      };
>  
> +    var handlerCallback = this._handleSelectedApps.bind(this);

Remove this and see below.

@@ +541,5 @@
>        target: aTarget
>      };
>  
> +    var handlerCallback = this._handleSelectedApps.bind(this);
> +    // Listen to the result of selected apps from front-end

A minor nit. A formal comment format in Gecko is supposed to be:

//{a-space}{upper-case-letter}{...}{a-period}

So, add a period at the end of the comment.

@@ +542,5 @@
>      };
>  
> +    var handlerCallback = this._handleSelectedApps.bind(this);
> +    // Listen to the result of selected apps from front-end
> +    SystemAppProxy.addEventListener("mozContentEvent", function act_getSelected(evt) {

Don't need to add a name for an anonymous function unless really needed, so remove act_getSelected.

Also, s/evt/aEvent/ because this file use "a" as prefix for parameters and "aEvt" looks weird to me, so...

@@ +551,2 @@
>  
> +      SystemAppProxy.removeEventListener("mozContentEvent", act_getSelected);

This attempt looks weird to me. What's happening if we fire two consecutive mozContentEvents? After the user respond to the first one and the Gecko handles it, the second one seems to be dropped.

Please educate me if I'm wrong.

@@ +551,4 @@
>  
> +      SystemAppProxy.removeEventListener("mozContentEvent", act_getSelected);
> +      if (DEBUG) {
> +        debug("inter-app-comm-permission: " + detail);

Don't you need to JSON.stringify(detail) if detail is an object?

@@ +552,5 @@
> +      SystemAppProxy.removeEventListener("mozContentEvent", act_getSelected);
> +      if (DEBUG) {
> +        debug("inter-app-comm-permission: " + detail);
> +      }
> +      handlerCallback({ callerID: detail.chromeEventID,

Just call this._handleSelectedApp(...).

And can we keep detail.callerID?

@@ +556,5 @@
> +      handlerCallback({ callerID: detail.chromeEventID,
> +                        keyword: detail.keyword,
> +                        manifestURL: detail.manifestURL,
> +                        selectedApps: detail.peers });
> +    });

and add "}.bind(this));" for the callback of SystemAppProxy.addEventListener.

@@ +562,5 @@
> +    if (DEBUG) {
> +      debug("appsToSelect: " + appsToSelect);
> +    }
> +    SystemAppProxy.dispatchEvent({ type: "inter-app-comm-permission",
> +                                   chromeEventID: callerID,

Why not do you change callerID to chromeEventID?

@@ +571,5 @@
>      // the prompt, which always allows the connection. This dummy codes
>      // will be removed when the UX/UI for the prompt is ready.
> +    SystemAppProxy._sendCustomEvent("mozContentEvent",
> +                                    { type: "inter-app-comm-permission",
> +                                      chromeEventID: callerID,

Ditto. chromeEventID?

Comment 4

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #3)
> Comment on attachment 8424720 [details] [diff] [review]
> Patch attempt 1
> 
> Review of attachment 8424720 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +551,2 @@
> >  
> > +      SystemAppProxy.removeEventListener("mozContentEvent", act_getSelected);
> 
> This attempt looks weird to me. What's happening if we fire two consecutive
> mozContentEvents? After the user respond to the first one and the Gecko
> handles it, the second one seems to be dropped.
> 
> Please educate me if I'm wrong.
Yeah. I was intended to do so and was inspired from ActivitiesGlue.js.
http://dxr.mozilla.org/mozilla-central/source/b2g/components/ActivitiesGlue.js#46
It looks right to me to only take the first event/response since the app has passed user's initial decisions. (And in most expected cases, there appears only one event.) But if Gecko plans to accept a series of content events from the same app for the same app selecting action, we might need some more specs for this.

Btw, if we plan to keep this line, then the callback function may probably not become anonymous.
> 
> @@ +556,5 @@
> > +      handlerCallback({ callerID: detail.chromeEventID,
> > +                        keyword: detail.keyword,
> > +                        manifestURL: detail.manifestURL,
> > +                        selectedApps: detail.peers });
> > +    });
> 
> and add "}.bind(this));" for the callback of SystemAppProxy.addEventListener.
Good tip. Thanks.
> 
> @@ +552,5 @@
> > +      SystemAppProxy.removeEventListener("mozContentEvent", act_getSelected);
> > +      if (DEBUG) {
> > +        debug("inter-app-comm-permission: " + detail);
> > +      }
> > +      handlerCallback({ callerID: detail.chromeEventID,
> 
> Just call this._handleSelectedApp(...).
> 
> And can we keep detail.callerID?
Actually I tried to keep the original field names of the outgoing and incoming events from the original implementation, where it took chromeEventID. So this change would be transparent to the event listener at Gaia if there's any.
http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#700
http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#774

Yet we may still consider to rename the fields since it doesn't seem many apps, either already existent or under development, use IAC so far. And the change only happens to app-select logic, for which no UI/UX has been ready yet. So the pain introdiced by backward incompatibility should be really limited.

Thoughts?!

> 
> @@ +562,5 @@
> > +    if (DEBUG) {
> > +      debug("appsToSelect: " + appsToSelect);
> > +    }
> > +    SystemAppProxy.dispatchEvent({ type: "inter-app-comm-permission",
> > +                                   chromeEventID: callerID,
> 
> Why not do you change callerID to chromeEventID?
Ditto
> 
> @@ +571,5 @@
> >      // the prompt, which always allows the connection. This dummy codes
> >      // will be removed when the UX/UI for the prompt is ready.
> > +    SystemAppProxy._sendCustomEvent("mozContentEvent",
> > +                                    { type: "inter-app-comm-permission",
> > +                                      chromeEventID: callerID,
> 
> Ditto. chromeEventID?
Ditto.

Comment 5

[Sean Lin [:seanlin] (inactive from Mar 2016)]

A brief summary after short in-person discussion:

1. It's necessary to remove the line
   SystemAppProxy.removeEventListener("mozContentEvent", act_getSelected);
   to better handle the scenario where multiple apps may have to select apps at the same time. Besides, the logic
   SystemAppProxy.addEventListener("mozContentEvent", ...)
   should move outside function _connect to ensure the listener is only added once even with multiple connection calls.

2. Keeping the original event field name (chromeEventID) is alright especially in consideration of backward compatibility.

Comment 6

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch attempt 2

Comment 7

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8425317 [details] [diff] [review]
Patch attempt 2

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

Nice! r=gene

Btw, this occurs to me we can use SystemAppProxy to fire the "system-messages-open-app" event in SystemMessageInternal.js, which might be another bug you can pick up. ;)

::: dom/apps/src/InterAppCommService.jsm
@@ +221,5 @@
>      // }
>      this._messagePortPairs = {};
> +
> +    // Listen to the result of selected apps from front-end.
> +    SystemAppProxy.addEventListener("mozContentEvent", function (aEvent) {

nit: drop the space between "function" and (...).

@@ +223,5 @@
> +
> +    // Listen to the result of selected apps from front-end.
> +    SystemAppProxy.addEventListener("mozContentEvent", function (aEvent) {
> +      let detail = aEvent.detail;
> +      if (detail.type != 'inter-app-comm-permission') {

nit: use double quotes in this file as a consistent coding style.

@@ +228,5 @@
> +        return;
> +      }
> +
> +      if (DEBUG) {
> +        debug("inter-app-comm-permission: " + JSON.stringify(detail));

s/"inter-app-comm-permission"/"mozContentEvent" because "inter-app-comm-permission" is already included and printed by JSON.stringify(detail) (i.e. detail.type).

@@ -871,5 @@
>    observe: function(aSubject, aTopic, aData) {
>      switch (aTopic) {
>        case "xpcom-shutdown":
>          Services.obs.removeObserver(this, "xpcom-shutdown");
> -        Services.obs.removeObserver(this, "inter-app-comm-select-app-result");

Might be good to remove the "mozContentEvent" event listener here.

Comment 8

[Gene Lian (I already quit Mozilla)]

Please see bug 1013671, #c4. We need to keep the observer topic for other platforms.

Comment 9

[Gene Lian (I already quit Mozilla)]

Now I think we should follow what we've done for:

  b2g/components/ActivitiesGlue.js
  mobile/android/components/ActivitiesGlue.js

The reason why we can use SystemAppProxy in the b2g/components/ActivitiesGlue.js is because we're sure it's for b2g specific which implies there must be a system app.

Comment 10

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch attempt 3

Updated based on the latest comments.

Comment 11

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8426775 [details] [diff] [review]
Patch attempt 3

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

::: b2g/components/B2GComponents.manifest
@@ +93,5 @@
>  #endif
>  
> +# InterAppCommUIGlue.js
> +component {4c8b0520-e0b5-11e3-9c1d-74d02b97e723} InterAppCommUIGlue.js
> +contract @mozilla.org/dom/apps/inter-app-comm-ui-glue;1 {4c8b0520-e0b5-11e3-9c1d-74d02b97e723}

Would you move this below ActivitiesGlue.js to easily track of because they are similar features?

::: b2g/components/InterAppCommUIGlue.js
@@ +19,5 @@
> +  dump("-- InterAppCommUIGlue: " + Date.now() + ": " + aMsg + "\n");
> +}
> +
> +function InterAppCommUIGlue() {
> +  // This matrix is to store the callerId (a random UUID) / callback binding.

nit: I'd prefer s/Id/ID for all the codes.

@@ +58,5 @@
> +}
> +
> +InterAppCommUIGlue.prototype = {
> +  selectApps: function(aCallerId, aPubAppManifestURL, aKeyword, aAppsToSelect, aCallback) {
> +    this._handlers[aCallerId] = aCallback

nit: add ";".

@@ +65,5 @@
> +                                   chromeEventID: aCallerId,
> +                                   manifestURL: aPubAppManifestURL,
> +                                   keyword: aKeyword,
> +                                   peers: aAppsToSelect });
> +    // TODO Bug 897169 Simulate the return of the app-selected result by

Add a blank line above this comment.

@@ +78,5 @@
> +  },
> +
> +  classID: Components.ID("{4c8b0520-e0b5-11e3-9c1d-74d02b97e723}"),
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIInterAppCommUIGlue, Ci.nsIRunnable])

Remove Ci.nsIRunnable since you don't use it.

@@ +81,5 @@
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIInterAppCommUIGlue, Ci.nsIRunnable])
> +};
> +
> +this.NSGetFactory = XPCOMUtils.generateNSGetFactory([InterAppCommUIGlue]);

Add a blank line at the end of file.

::: dom/apps/src/InterAppCommService.jsm
@@ +218,5 @@
>      // }
>      this._messagePortPairs = {};
> +
> +    this._glue = Cc["@mozilla.org/dom/apps/inter-app-comm-ui-glue;1"]
> +                   .createInstance(Ci.nsIInterAppCommUIGlue);

You should put this before really instantiate it. We might not need to use it for certified apps, so it doesn't make sense to put it in the constructor.

This is the main reason for not giving review+ for now.

@@ +540,5 @@
>        requestID: requestID,
>        target: aTarget
>      };
>  
> +    if (this._glue) {

let glue = Cc["@mozilla.org/dom/apps/inter-app-comm-ui-glue;1"]
             .createInstance(Ci.nsIInterAppCommUIGlue);

before this.

::: dom/interfaces/apps/nsIInterAppCommUIGlue.idl
@@ +8,5 @@
> +interface nsIInterAppCommUIGlueCallback : nsISupports
> +{
> +    /**
> +      * Called after the user selected apps.
> +      * @param data  The data containing the info of the selected apps and some

s/containg/contains/

@@ +33,5 @@
> +      * @param callback           The callback to handle the selected apps.
> +      */
> +    void selectApps(in jsval callerId, in jsval pubAppManifestURL,
> +        in jsval keyword, in jsval appsToSelect,
> +        in nsIInterAppCommUIGlueCallback callback);

Please align all the arguments:

void selectApps(in AString callerId,
                in AString pubAppManifestURL,
                in AString keyword,
                in jsval appsToSelect,
                in nsIInterAppCommUIGlueCallback callback);

Also, use AString type for string which is more explicit.

Comment 12

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch attempt 4

Comment 13

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8426939 [details] [diff] [review]
Patch attempt 4

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

Nice! Thank you! r=gene

::: b2g/components/InterAppCommUIGlue.js
@@ +49,5 @@
> +      return;
> +    }
> +
> +    delete this._handlers[callerID];
> +    handler.handleApps({ callerID: callerID,

s/handleApps/handleSelectedApps/

@@ +57,5 @@
> +  }.bind(this));
> +}
> +
> +InterAppCommUIGlue.prototype = {
> +  selectApps: function(aCallerID, aPubAppManifestURL, aKeyword, aAppsToSelect, aCallback) {

Coding convention nit: this is too long (beyond 80 chars). Do:

selectApps: function(aCallerID, aPubAppManifestURL, aKeyword, aAppsToSelect,
                     aCallback) {

::: dom/apps/src/InterAppCommService.jsm
@@ +542,5 @@
> +                 .createInstance(Ci.nsIInterAppCommUIGlue);
> +    if (glue) {
> +      glue.selectApps(callerID, pubAppManifestURL, keyword, appsToSelect,
> +                      this._handleSelectedApps.bind(this));
> +    }

} else {
  if (DEBUG) {
    debug("Error! The UI glue component is not implemented. Rollback!")
  }
  delete this._promptUICallers[callerID];
}

::: dom/interfaces/apps/nsIInterAppCommUIGlue.idl
@@ +8,5 @@
> +interface nsIInterAppCommUIGlueCallback : nsISupports
> +{
> +    /**
> +      * Called after the user selected apps.
> +      * @param data  The data contain the info of the selected apps and some

s/contain/contains/

@@ +11,5 @@
> +      * Called after the user selected apps.
> +      * @param data  The data contain the info of the selected apps and some
> +      *              IAC attributes.
> +      */
> +    void handleApps(in jsval data);

s/handleApps/handleSelectedApps/

Comment 14

[Gene Lian (I already quit Mozilla)]

I believe my review is pretty much enough. Welcome inputs if you are available and interested. ;)

Comment 15

[Gene Lian (I already quit Mozilla)]

The previous comment is talking to Fabrice.

Comment 16

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8426939 [details] [diff] [review]
Patch attempt 4

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

::: dom/apps/src/InterAppCommService.jsm
@@ +542,5 @@
> +                 .createInstance(Ci.nsIInterAppCommUIGlue);
> +    if (glue) {
> +      glue.selectApps(callerID, pubAppManifestURL, keyword, appsToSelect,
> +                      this._handleSelectedApps.bind(this));
> +    }

After some thoughts, I think we should do:

} else {
  if (DEBUG) {
    debug("Error! The UI glue component is not implemented.")
  }

  // Handle the selected apps as if they are empty to resolve the caller.
  this._handleSelectcedApps({ callerID: callerID,
                              manifestURL: pubAppManifestURL,
                              keyword: keyword,
                              selectedApps: [] });
}

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8426939 [details] [diff] [review]
Patch attempt 4

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

I'd like that to be done a bit differently.

::: b2g/components/InterAppCommUIGlue.js
@@ +74,5 @@
> +                                    { type: "inter-app-comm-permission",
> +                                      chromeEventID: aCallerID,
> +                                      manifestURL: aPubAppManifestURL,
> +                                      keyword: aKeyword,
> +                                      peers: aAppsToSelect });

Please don't use the generic mozChomeEvent / mozContentEvent. Use SystemAppProxy._sendCustomEvent() with your own event name.

::: dom/interfaces/apps/nsIInterAppCommUIGlue.idl
@@ +35,5 @@
> +    void selectApps(in AString callerID,
> +                    in AString pubAppManifestURL,
> +                    in AString keyword,
> +                    in jsval appsToSelect,
> +                    in nsIInterAppCommUIGlueCallback callback);

Instead of passing a callback, I would have selectApps return a Promise instead, and resolve/reject depending on the result of the user selection.

Comment 18

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch attempt 5

Updated based on Gene & Fabrice's comments.

Comment 19

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8427615 [details] [diff] [review]
Patch attempt 5

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

Nice! Need to get Fabrice's review+ as well.

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8427615 [details] [diff] [review]
Patch attempt 5

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

r=me with the very minor nit fixed. Thanks!

::: b2g/installer/package-manifest.in
@@ +535,5 @@
>  @BINPATH@/components/InterAppComm.manifest
>  @BINPATH@/components/InterAppCommService.js
>  @BINPATH@/components/InterAppConnection.js
>  @BINPATH@/components/InterAppMessagePort.js
> +@BINPATH@/components/InterAppCommUIGlue.js

nit: I would move that to the "b2g specific components" section

Comment 21

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch attempt 6

Moved @BINPATH@/components/InterAppCommUIGlue.js down to the place for b2g specific components in b2g/installer/package-manifest.in based on Fabrice's latest comment.

Comment 22

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8428465 [details] [diff] [review]
Patch attempt 6

Since the code change has got review+ from Gene and Fabrice, and the last update is really minor, I'd directly mark "checkin-needed" for it.

Comment 23

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch attempt 7

Applied the patch with the lastest code base to resolve detected merge conflicts.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd69dc2e968

Comment 25

[Gene Lian (I already quit Mozilla)]

Oh! Sadly we land this at the same time on different branches...

https://hg.mozilla.org/integration/b2g-inbound/rev/eab1c325d9fb

Sorry, Sheriff, hope this won't confuse you too much.

Comment 26

[Gene Lian (I already quit Mozilla)]

Hi Carsten, do I need to back it out manually?

Comment 27

[Carsten Book [:Tomcat]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #26)
> Hi Carsten, do I need to back it out manually?

no problem, backedout from b2g-i in https://hg.mozilla.org/integration/b2g-inbound/rev/e08f8d3af303 :)

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cd69dc2e968