1013671 - Use SystemAppProxy to fire "system-messages-open-app" from SystemMessageInternal.js

Status: RESOLVED FIXED

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

Description

[Gene Lian (I already quit Mozilla)]

Bug 908191 inspires me we should use SystemAppProxy to fire "system-messages-open-app" from SystemMessageInternal.js, instead of calling the notifyObservers() to notify the shell.js.

Fabrice, if my understanding on the SystemAppProxy is correct, that's something we should polish. Right? Also, the shell.js is listening to lots of observer topics for sendCustomEvent too, which are all supposed to be done through the SystemAppProxy in the long-term. Right?

Sean, this might be another good first bug you can support.

Comment 1

[[:fabrice] Fabrice Desré]

We can do that on b2g, but I'm worried about other runtimes that may want to enable system messages, like Android.

We do all the buffering of messages in shell.js too, I'm not sure yet if we should move that somewhere else.

Comment 2

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #1)
> We can do that on b2g, but I'm worried about other runtimes that may want to
> enable system messages, like Android.

But the 'system-messages-open-app' is designed for asking the System App (window_manager) to open other apps, which implies there must be a System App listening to it. We haven't yet considered other paths (like Android) for non-system-app scenarios so I think it's OK to do this for 'system-messages-open-app' specifically.

> 
> We do all the buffering of messages in shell.js too, I'm not sure yet if we
> should move that somewhere else.

If we can use SystemAppProxy to fire "system-messages-open-app", another benefit is we might rely on the SystemAppProxy.setIsReady() to pend the chrome events until the system completely boots up, so that we can remove the ugly 10-sec timer for buffering 'system-messages-open-app' messages in shell.js.

Comment 3

[[:fabrice] Fabrice Desré]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #2)
> (In reply to Fabrice Desré [:fabrice] from comment #1)
> > We can do that on b2g, but I'm worried about other runtimes that may want to
> > enable system messages, like Android.
> 
> But the 'system-messages-open-app' is designed for asking the System App
> (window_manager) to open other apps, which implies there must be a System
> App listening to it. We haven't yet considered other paths (like Android)
> for non-system-app scenarios so I think it's OK to do this for
> 'system-messages-open-app' specifically.

It's designed to ask the "runtime UI" to do something. In b2g case it's the system app. That will be something else in other runtimes. 

What about just moving the observer in SystemAppProxy?

> > 
> > We do all the buffering of messages in shell.js too, I'm not sure yet if we
> > should move that somewhere else.
> 
> If we can use SystemAppProxy to fire "system-messages-open-app", another
> benefit is we might rely on the SystemAppProxy.setIsReady() to pend the
> chrome events until the system completely boots up, so that we can remove
> the ugly 10-sec timer for buffering 'system-messages-open-app' messages in
> shell.js.

Yes, we should do that.

Comment 4

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #2)
> > (In reply to Fabrice Desré [:fabrice] from comment #1)
> > > We can do that on b2g, but I'm worried about other runtimes that may want to
> > > enable system messages, like Android.
> > 
> > But the 'system-messages-open-app' is designed for asking the System App
> > (window_manager) to open other apps, which implies there must be a System
> > App listening to it. We haven't yet considered other paths (like Android)
> > for non-system-app scenarios so I think it's OK to do this for
> > 'system-messages-open-app' specifically.
> 
> It's designed to ask the "runtime UI" to do something. In b2g case it's the
> system app. That will be something else in other runtimes. 
> 
> What about just moving the observer in SystemAppProxy?

Just have a quick discussion on the IRC. It makes sense to us to move the observer topics from shell.js into SystemAppProxy. We need to keep the observer mechanism for now because other platforms (like Android) might also need to listen to those observer topics to launch its own runtime UIs in its own way in the future.

Comment 5

[Gene Lian (I already quit Mozilla)]

Another alternative is we can follow what we've done for:

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

to construct different components but with he same interface, then we can call SystemAppProxy's APIs to dispatch chrome events based on the instance of the b2g-specific component. In this way, we don't need to move the observer topic from shell.js to SystemAppProxy.

Comment 6

[Gene Lian (I already quit Mozilla)]

... just drop the observer topics instead.

Comment 7

[[:fabrice] Fabrice Desré]

We can do that too, it's just a bit more refactoring.

Comment 8

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

Attachment: Patch v1

The patch also fixes Bug 793420 since the logic relevant to these two bugs is interleaved to some extent.

Comment 9

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

Attachment: Patch v2

Further removing variable |timer| from shell.js since we no longer need it.

Comment 10

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8430494 [details] [diff] [review]
Patch v2

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

::: b2g/components/B2GComponents.manifest
@@ +27,5 @@
>  # InterAppCommUIGlue.js
>  component {879ee66c-e246-11e3-9910-74d02b97e723} InterAppCommUIGlue.js
>  contract @mozilla.org/dom/apps/inter-app-comm-ui-glue;1 {879ee66c-e246-11e3-9910-74d02b97e723}
>  
> +# SystemMessagesUIGlue.js

System message doesn't attempt to launch any "UI" stuff, so SystemMessagesUIGlue might not be a good name.

How about SystemMessagesOpenAppGlue or any others better?

::: b2g/components/SystemAppProxy.jsm
@@ +53,5 @@
>     *     event.details == 'bar'
>     *   });
>     */
> +  _sendCustomEvent: function systemApp_sendCustomEvent(type, details,
> +                                                       noPendingUntilReady) {

Would you add some comments about how to use |noPendingUntilReady|? Also, I think |noPending| is enough because you still need to check the codes to realize the meaning of "UntilReady" anyway, so just let comments cover it.

@@ +58,5 @@
>      let content = this._frame ? this._frame.contentWindow : null;
>  
>      // If the system app isn't ready yet,
>      // queue events until someone calls setIsLoaded
> +    if ((!noPendingUntilReady && !this._isReady) || !content) {

Why it's not:

if (!noPendingUntilReady && (!this._isReady || !content)) {

???

::: b2g/components/SystemMessagesUIGlue.js
@@ +9,5 @@
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");

You don't need Promise.

@@ +15,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "SystemAppProxy",
> +                                  "resource://gre/modules/SystemAppProxy.jsm");
> +
> +const DEBUG = false;
> +function debug(aMsg) {

You didn't use the debug in this file. Add it to one place at least or just remove the debug function.

@@ +34,5 @@
> +                    onlyShowApp: aOnlyShowApp,
> +                    expectingSystemMessage: true,
> +                    extra: aExtra };
> +
> +    // |SystemAppProxy| will queue non-activity events without actually sending

s/non-activity events/'open-app' events for non-activity system messages/

@@ +35,5 @@
> +                    expectingSystemMessage: true,
> +                    extra: aExtra };
> +
> +    // |SystemAppProxy| will queue non-activity events without actually sending
> +    // them until it's set ready.

s/it's/the system app is/

::: dom/messages/SystemMessageInternal.js
@@ +589,5 @@
> +          ", manifestURL: " + aPage.manifestURL + ", type: " + aPage.type +
> +          ", target: " + JSON.stringify(aMessage.target) +
> +          ", showApp: " + showApp + ", onlyShowApp: " + onlyShowApp +
> +          ", extra: " + JSON.stringify(aExtra));
> +    let glue = Cc["@mozilla.org/dom/messages/system-messages-ui-glue;1"]

Add one blank line above this line.

::: dom/messages/interfaces/nsISystemMessagesUIGlue.idl
@@ +10,5 @@
> +interface nsISystemMessagesUIGlue : nsISupports
> +{
> +    /* Notify the system app to open the target app.
> +     *
> +     * @param pageURL     The URI of the page that will be opened.

s/URI/URL/

URI is actually a special interface (nsIURI), whereas URL represents the string of URI.

@@ +11,5 @@
> +{
> +    /* Notify the system app to open the target app.
> +     *
> +     * @param pageURL     The URI of the page that will be opened.
> +     * @param manifestURL The webapp's manifest URI.

s/URI/URL/

@@ +23,5 @@
> +     */
> +    void openApp(in AString pageURL,
> +                 in AString manifestURL,
> +                 in AString type,
> +                 in jsval target,

I wonder we can have a more explicit interface than jsval for the target.

Comment 11

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #10)
> ::: b2g/components/B2GComponents.manifest
> @@ +27,5 @@
> >  # InterAppCommUIGlue.js
> >  component {879ee66c-e246-11e3-9910-74d02b97e723} InterAppCommUIGlue.js
> >  contract @mozilla.org/dom/apps/inter-app-comm-ui-glue;1 {879ee66c-e246-11e3-9910-74d02b97e723}
> >  
> > +# SystemMessagesUIGlue.js
> 
> System message doesn't attempt to launch any "UI" stuff, so
> SystemMessagesUIGlue might not be a good name.
> 
> How about SystemMessagesOpenAppGlue or any others better?
I'm thinking to simply use SystemMessagesGlue with the flexibility to enhace it with some other methods in the future.
> @@ +58,5 @@
> >      let content = this._frame ? this._frame.contentWindow : null;
> >  
> >      // If the system app isn't ready yet,
> >      // queue events until someone calls setIsLoaded
> > +    if ((!noPendingUntilReady && !this._isReady) || !content) {
> 
> Why it's not:
> 
> if (!noPendingUntilReady && (!this._isReady || !content)) {
> 
> ???
Since the next line below this section is

let event = content.document.createEvent('CustomEvent');

To ensure null-safe, I made a compromise to allow pending for the edge case where content hasn't been assigned with anything.

Thoughts?!

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Sean Lin [:seanlin] from comment #11)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #10)
> > ::: b2g/components/B2GComponents.manifest
> > @@ +27,5 @@
> > >  # InterAppCommUIGlue.js
> > >  component {879ee66c-e246-11e3-9910-74d02b97e723} InterAppCommUIGlue.js
> > >  contract @mozilla.org/dom/apps/inter-app-comm-ui-glue;1 {879ee66c-e246-11e3-9910-74d02b97e723}
> > >  
> > > +# SystemMessagesUIGlue.js
> > 
> > System message doesn't attempt to launch any "UI" stuff, so
> > SystemMessagesUIGlue might not be a good name.
> > 
> > How about SystemMessagesOpenAppGlue or any others better?
> I'm thinking to simply use SystemMessagesGlue with the flexibility to enhace
> it with some other methods in the future.

Yeap, sounds good. Btw, please s/SystemMessagesGlue/SystemMessageGlue. Don't need an extra "s", which is consistent with other system message related file names.

> > @@ +58,5 @@
> > >      let content = this._frame ? this._frame.contentWindow : null;
> > >  
> > >      // If the system app isn't ready yet,
> > >      // queue events until someone calls setIsLoaded
> > > +    if ((!noPendingUntilReady && !this._isReady) || !content) {
> > 
> > Why it's not:
> > 
> > if (!noPendingUntilReady && (!this._isReady || !content)) {
> > 
> > ???
> Since the next line below this section is
> 
> let event = content.document.createEvent('CustomEvent');
> 
> To ensure null-safe, I made a compromise to allow pending for the edge case
> where content hasn't been assigned with anything.
> 
> Thoughts?!

OK, I see. I'd like to have a a minor improvement on this condition:

  if (!content || (!this._isReady && !noPending))

which has a more reasonable order because the flow we expect is:

1. Check if the content window is constructed.
2. If the content window is constructed, check if the system app is ready to receive events.
3. If the system app is not ready to receive events, check if we need to pend the events.

Comment 13

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

Attachment: Patch v3

Updating based on Gene's comments.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8431418 [details] [diff] [review]
Patch v3

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

Sean, I won't have time to review during the next 2 weeks, but I'll do it when I'll be back.

Comment 15

[Gene Lian (I already quit Mozilla)]

(In reply to < away until June 17 > from comment #14)
> Comment on attachment 8431418 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8431418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sean, I won't have time to review during the next 2 weeks, but I'll do it
> when I'll be back.

Two weeks sounds a too long timeline to wait for when it comes to fixing a bug. I personally think having my review is enough because we used to have a very similar same change for inter-app-comm API UI glue and Fabrice has passed the review for it.

I think we can land this first and needinfo? to Fabrice. If Fabrice wouldn't be satisfied with the changes when he comes back, we can polish it as a follow-up.

Comment 16

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

The correspondent try run for this patch.
https://tbpl.mozilla.org/?tree=Try&rev=f273d916c71d

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10869fc4d7e7

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10869fc4d7e7