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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: airpingu, Assigned: selin)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Release this to anyone who is interested and I'm happy to be the mentor.
Assignee: gene.lian → nobody
No longer depends on: inter-app-comm-api
Assignee: nobody → selin
Posted patch Patch attempt 1 (obsolete) — Splinter Review
Patch attempt 1
Attachment #8424720 - Flags: review?(gene.lian)
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?
Attachment #8424720 - Flags: review?(gene.lian) → review-
(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.
Flags: needinfo?(gene.lian)
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.
Flags: needinfo?(gene.lian)
Posted patch Patch attempt 2 (obsolete) — Splinter Review
Attachment #8424720 - Attachment is obsolete: true
Attachment #8425317 - Flags: review?(gene.lian)
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.
Attachment #8425317 - Flags: review?(gene.lian) → review+
Please see bug 1013671, #c4. We need to keep the observer topic for other platforms.
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.
Attachment #8425317 - Flags: review+
Posted patch Patch attempt 3 (obsolete) — Splinter Review
Updated based on the latest comments.
Attachment #8425317 - Attachment is obsolete: true
Attachment #8426775 - Flags: review?(gene.lian)
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.
Attachment #8426775 - Flags: review?(gene.lian)
Posted patch Patch attempt 4 (obsolete) — Splinter Review
Attachment #8426775 - Attachment is obsolete: true
Attachment #8426939 - Flags: review?(gene.lian)
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/
Attachment #8426939 - Flags: review?(gene.lian) → review+
I believe my review is pretty much enough. Welcome inputs if you are available and interested. ;)
The previous comment is talking to Fabrice.
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 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.
Attachment #8426939 - Flags: review-
Posted patch Patch attempt 5 (obsolete) — Splinter Review
Updated based on Gene & Fabrice's comments.
Attachment #8426939 - Attachment is obsolete: true
Attachment #8427615 - Flags: review?(fabrice)
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.
Attachment #8427615 - Flags: review+
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
Attachment #8427615 - Flags: review?(fabrice) → review+
Posted patch Patch attempt 6 (obsolete) — Splinter Review
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.
Attachment #8427615 - Attachment is obsolete: true
Attachment #8428465 - Flags: review?(fabrice)
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.
Attachment #8428465 - Flags: review?(fabrice)
Applied the patch with the lastest code base to resolve detected merge conflicts.
Attachment #8428465 - Attachment is obsolete: true
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.
Hi Carsten, do I need to back it out manually?
Flags: needinfo?(cbook)
(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 :)
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/0cd69dc2e968
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.