Last Comment Bug 755245 - (system-message-api) Implement System Message Handler
(system-message-api)
: Implement System Message Handler
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: [:fabrice] Fabrice Desré
: Jason Smith [:jsmith]
Mentors:
Depends on: 800431 818000 874353 883362 888609 895689 898307 931339 988142 1001278 751025 768228 770491 775032 782524 783149 787064 788466 793420 795209 795782 797803 801257 803490 805655 812080 821607 828395 830616 866366 873348 874339 875694 876614 878395 883215 887650 888926 892708 893703 895104 896945 899260 906164 909001 915611 964769 988129 988787 1013671 1035074
Blocks: webapi 715814 alarm-api 776027 781127 inter-app-comm-api 900734
  Show dependency treegraph
 
Reported: 2012-05-15 05:34 PDT by Mounir Lamouri (:mounir)
Modified: 2014-07-07 00:12 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Part 1 : IDL (2.54 KB, patch)
2012-05-22 15:30 PDT, [:fabrice] Fabrice Desré
mounir: feedback+
Details | Diff | Splinter Review
Part 2 : changes to have a getter/setter on isApp and appManifest (4.12 KB, patch)
2012-05-22 15:32 PDT, [:fabrice] Fabrice Desré
mounir: feedback-
Details | Diff | Splinter Review
Part 3 : DOM and internal parts (13.62 KB, patch)
2012-05-22 15:33 PDT, [:fabrice] Fabrice Desré
21: feedback+
Details | Diff | Splinter Review
Part 4 : b2g specific code (2.58 KB, patch)
2012-05-22 15:35 PDT, [:fabrice] Fabrice Desré
21: feedback+
Details | Diff | Splinter Review
Part 1 : IDL (2.93 KB, patch)
2012-05-24 14:18 PDT, [:fabrice] Fabrice Desré
mounir: review+
jonas: superreview+
Details | Diff | Splinter Review
Part 2 : adding getApp() (2.68 KB, patch)
2012-05-24 14:27 PDT, [:fabrice] Fabrice Desré
mounir: review-
Details | Diff | Splinter Review
Part 3 : DOM and internal parts (14.05 KB, patch)
2012-05-24 14:30 PDT, [:fabrice] Fabrice Desré
mounir: review-
21: review-
Details | Diff | Splinter Review
Part 2 : adding getApp() v2 (4.73 KB, patch)
2012-05-31 12:45 PDT, [:fabrice] Fabrice Desré
mounir: review+
Ms2ger: review-
Details | Diff | Splinter Review
Part 3 : DOM and internal parts v2 (15.39 KB, patch)
2012-05-31 12:47 PDT, [:fabrice] Fabrice Desré
21: review-
mounir: review-
Details | Diff | Splinter Review
Part 4 : b2g specific code v2 (5.84 KB, patch)
2012-05-31 12:49 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
Part 3 : DOM and internal parts (16.03 KB, patch)
2012-06-12 18:00 PDT, [:fabrice] Fabrice Desré
21: review+
mounir: review-
Details | Diff | Splinter Review
Part 4 : b2g specific code v3 (2.38 KB, patch)
2012-06-12 18:02 PDT, [:fabrice] Fabrice Desré
21: review-
Details | Diff | Splinter Review
Part 5 : webapps manifest support (4.10 KB, patch)
2012-06-12 18:04 PDT, [:fabrice] Fabrice Desré
21: review+
Details | Diff | Splinter Review
Part 4 : b2g specific code v4 (1.67 KB, patch)
2012-06-15 17:58 PDT, [:fabrice] Fabrice Desré
21: review+
Details | Diff | Splinter Review
Part 3 : DOM and internal parts v3 (16.75 KB, patch)
2012-06-19 14:38 PDT, [:fabrice] Fabrice Desré
mounir: review-
Details | Diff | Splinter Review
Part 3 : DOM and internal parts v4 (17.20 KB, patch)
2012-06-25 15:54 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
Part 1 : IDL v2 (4.07 KB, patch)
2012-06-26 02:26 PDT, [:fabrice] Fabrice Desré
khuey: review-
Details | Diff | Splinter Review
Part 3 : DOM and internal parts v5 (17.59 KB, patch)
2012-06-26 02:27 PDT, [:fabrice] Fabrice Desré
mounir: review+
Details | Diff | Splinter Review
Part 1 : IDL v3 (3.85 KB, patch)
2012-07-02 15:49 PDT, [:fabrice] Fabrice Desré
khuey: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-05-15 05:34:46 PDT
See the discussion in the webapi mailing list:
https://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/a3c6e4c31d04b663
Comment 1 [:fabrice] Fabrice Desré 2012-05-21 18:04:51 PDT
I should have a wip ready tomorrow
Comment 2 [:fabrice] Fabrice Desré 2012-05-22 15:30:59 PDT
Created attachment 626212 [details] [diff] [review]
Part 1 : IDL

IDL for the DOM part, to be exposed on navigator
Comment 3 [:fabrice] Fabrice Desré 2012-05-22 15:32:36 PDT
Created attachment 626216 [details] [diff] [review]
Part 2 : changes to have a getter/setter on isApp and appManifest
Comment 4 [:fabrice] Fabrice Desré 2012-05-22 15:33:58 PDT
Created attachment 626217 [details] [diff] [review]
Part 3 : DOM and internal parts

Mounir for the c++, vingtetun the js parts.
Comment 5 [:fabrice] Fabrice Desré 2012-05-22 15:35:19 PDT
Created attachment 626218 [details] [diff] [review]
Part 4 : b2g specific code

There is a bit of test code, that works with https://github.com/fabricedesre/gaia/tree/activities
Comment 6 Mounir Lamouri (:mounir) 2012-05-23 07:25:09 PDT
Comment on attachment 626212 [details] [diff] [review]
Part 1 : IDL

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

You have to update toolkit/toolkit-makefiles.sh
You alse need to add dom_messages.xpt to package-manifest.in's.

::: dom/messages/interfaces/nsIDOMNavigatorMessages.idl
@@ +4,5 @@
> +
> +#include "domstubs.idl"
> +
> +[scriptable, function, uuid(42692976-57fd-4bb4-ab95-2b97ebdc5056)]
> +interface nsIDOMMessageCallback : nsISupports

Could you name this |nsIDOMSystemMessageCallback|?

@@ +10,5 @@
> +    void handleMessage(in jsval message);
> +};
> +
> +[scriptable, uuid(091e90dd-0e8b-463d-8cdc-9225d3a6ff90)]
> +interface nsIDOMNavigatorMessages : nsISupports

nsIDOMNavigatorSystemMessages
Comment 7 Mounir Lamouri (:mounir) 2012-05-23 07:38:42 PDT
Comment on attachment 626217 [details] [diff] [review]
Part 3 : DOM and internal parts

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

::: dom/base/Navigator.cpp
@@ +1168,5 @@
> +//    nsNavigator::nsIDOMMessagesManager
> +//*****************************************************************************
> +NS_IMETHODIMP
> +Navigator::GetMessagesManager()
> +{

if (mMessagesManager) {
  return NS_OK;
}

That way you save a useless initialization and an identation.

::: dom/base/Navigator.h
@@ +112,4 @@
>     */
>    void SetWindow(nsPIDOMWindow *aInnerWindow);
>  
> +  // helper to set mActivities

mActivities?

@@ +112,5 @@
>     */
>    void SetWindow(nsPIDOMWindow *aInnerWindow);
>  
> +  // helper to set mActivities
> +  nsresult GetMessagesManager();

Given what the method is doing could you call that InitalizeMessagesManager() or something like that. "Get" let the caller assume something is going to be returned.

::: dom/messages/Makefile.in
@@ +14,5 @@
>  PARALLEL_DIRS = interfaces
>  
> +EXTRA_COMPONENTS = \
> +  SystemMessageManager.js       \
> +  SystemMessageInternal.js          \

This syntax would work well (you have too many spaces):
EXTRA_COMPONENTS = \
  Foo.js \
  Bar.js \
  Foobar.js \
  $(NULL)

::: dom/messages/SystemMessageManager.js
@@ +55,5 @@
> +    //Send a sync message to the parent to check if we have a pending message
> +    this._pending = cpmm.sendSyncMessage("SystemMessageManager:GetPending", { uri: this._uri, manifest: this._manifest })[0];
> +    this._hasPending = this._pending !== null;
> +    delete this.hasPendingMessage;
> +    this.hasPendingMessage = false;

That means it will *always* return false from now on. Is that really what we want?
(I might very likely misunderstood what you are trying to do, I'm far from a JS expert)

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +19,5 @@
> +    // Registration of a page that wants to be notified of a message type.
> +    // @param type          The message type.
> +    // @param pageURI       The URI of the page that will be opened.
> +    // @param manifestURI   The webapp's manifest URI.
> +    void registerPage(in DOMString type, in nsIURI pageURI, [optional] in nsIURI manifestURI);

How would that be used? and by whom?
Comment 8 Mounir Lamouri (:mounir) 2012-05-23 08:27:17 PDT
Comment on attachment 626216 [details] [diff] [review]
Part 2 : changes to have a getter/setter on isApp and appManifest

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

::: dom/base/nsGlobalWindow.cpp
@@ +10023,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +nsGlobalWindow::GetApp(nsAString& aManifestURL)

::GetApp returning a manifest URL is weird

@@ +10030,5 @@
> +    aManifestURL = NS_LITERAL_STRING("");
> +    return NS_OK;
> +  }
> +
> +  // we should has a mApp

and I can has cheeseburger :)

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1125,4 @@
>     * children) to have a different value for IsPartOfApp than the frame's
>     * parent.
>     */
> +  attribute boolean isApp;

According to your other patches, this is never used. I would prefer to avoid this chance if it's not needed.

@@ +1134,4 @@
>     * This method will throw an exception if the manifest URL isn't a valid URL
>     * or isn't the manifest URL of an installed application.
>     */
> +  attribute DOMString appManifest;

I don't really like that.

Why not:
void               setApp(in DOMString manifestURL);
mozIDOMApplication getApp();

or:
void               setApp(in DOMString manifestURL);
readonly attribute mozIDOMApplication app;
Comment 9 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-05-24 07:15:35 PDT
Comment on attachment 626217 [details] [diff] [review]
Part 3 : DOM and internal parts

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

My main concern sounds to be the expectation of a window object. From what I have understand System Message should be able to target a worker too. I won't mind not having support for them in a first draft but I would like to make sure the current approach won't make it impossible without rewriting the component.

::: dom/messages/SystemMessageManager.js
@@ +38,5 @@
> +    }
> +    this._handlers[aType].push(aCallback);
> +
> +    // if we have a pending message, send it asynchronously
> +    if (this._hasPending) {

You likely want to make a call to hasPendingMessage because the message could have been added before the call to setMessageHandler or do we expect the front-end to always do hasPendingMessage before?

@@ +55,5 @@
> +    //Send a sync message to the parent to check if we have a pending message
> +    this._pending = cpmm.sendSyncMessage("SystemMessageManager:GetPending", { uri: this._uri, manifest: this._manifest })[0];
> +    this._hasPending = this._pending !== null;
> +    delete this.hasPendingMessage;
> +    this.hasPendingMessage = false;

There is a lot of issues in this hasPendingMessage method.

For example if you call it twice, it will return true, then false.
If you don't call it, setMessageHandler will never do anything.
If you have a few message of a different type, it will ignore them.
Comment 10 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-05-24 07:17:13 PDT
Comment on attachment 626218 [details] [diff] [review]
Part 4 : b2g specific code

Sigh. The more I want to kill mozChromeEvent/mozContentEvent, the more I see it eerywhere. I don't have any other solution for now so f+.
Comment 11 [:fabrice] Fabrice Desré 2012-05-24 14:18:45 PDT
Created attachment 626963 [details] [diff] [review]
Part 1 : IDL

Addressed comments from comment #6
Comment 12 [:fabrice] Fabrice Desré 2012-05-24 14:27:55 PDT
Created attachment 626968 [details] [diff] [review]
Part 2 : adding getApp()

I went with this option:
mozIDOMApplication getApp()
Comment 13 [:fabrice] Fabrice Desré 2012-05-24 14:30:27 PDT
Created attachment 626971 [details] [diff] [review]
Part 3 : DOM and internal parts

Addressing comments from Mounir and Vivien.

About the usage in a worker, we only rely on the window to retrieve the manifest URL is any. We'll need something similar from workers.
Comment 14 Gene Lian [:gene] (I already quit Mozilla) 2012-05-29 01:01:21 PDT
Comment on attachment 626218 [details] [diff] [review]
Part 4 : b2g specific code

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

::: b2g/chrome/content/shell.js
@@ +672,5 @@
> +
> +let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +timer.initWithCallback(function() {
> +  dump("Firing alarm\n");
> +  messenger.sendMessage("alarm", { time: Date.now(), reason: "get up!" });

Fabrice,

Firstly, thanks for your big support on the system message part, making the Alarm API to achieve it goal much earlier :)

One thing that I don't quite understand when I try to integrate the system message in the Alarm API by following your testing codes here. The keyword "alarm" here seems to be assigned internally. Right? If so, how the App knows to call setMessageHandler(in DOMString type, ...) or hasPendingMessage(in DOMString type) function with the correct type (i.e. "alarm")? Or on the contrary, how the messenger here knows which message type it should send corresponding to the type specified by setMessageHandler(in DOMString type, ...)?

I'll invite you to have a review when I have a WIP patch for the system message integration in Alarm API.

Thanks,
Gene
Comment 15 Gene Lian [:gene] (I already quit Mozilla) 2012-05-29 01:39:06 PDT
Fabrice,

Btw, I had another question similar to the above-mentioned one: how to decide the pageURI and manifestURI in messenger.registerPage() if these two URIs could vary by different Apps?

Thanks,
Gene
Comment 16 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-05-29 03:26:33 PDT
Comment on attachment 626971 [details] [diff] [review]
Part 3 : DOM and internal parts

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

r- because of the hasPendingMessage question. The rest seems fine to me.

::: dom/messages/SystemMessageInternal.js
@@ +1,5 @@
> +/* 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/. */
> +
> +"use strict"

"use strict";

@@ +16,5 @@
> +});
> +
> +function debug(aMsg) { 
> +  dump("-- SystemMessageInternal " + Date.now() + " : " + aMsg + "\n"); 
> +}

nit: make it silent by default

@@ +70,5 @@
> +    });
> +
> +    // remove the returned pending message
> +    if (pending)
> +      this._pending.splice(index, 1);

If systemMessageService.hasPendingMessage is called twice, it will return true, then false, because of this line. It seems strange to remove the pending message before it has been consumed.

::: dom/messages/SystemMessageManager.js
@@ +13,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "cpmm", function() {
> +  return Cc["@mozilla.org/childprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager).QueryInterface(Ci.nsISyncMessageSender);
> +});

Does it needs to be cleaned at some point, like how you're doing it with ppmm in SystemMessageInternal.js?

@@ +17,5 @@
> +});
> +
> +function debug(aMsg) { 
> +  dump("-- SystemMessageManager " + Date.now() + " : " + aMsg + "\n"); 
> +}

nit: make it silent by default

@@ +30,5 @@
> +
> +SystemMessageManager.prototype = {
> +  __proto__: DOMRequestIpcHelper.prototype,
> +
> +  setMessageHandler: function(aType, aCallback) {

nit: sometimes in the code aCalback is called aHandler (in receiveMessage). Which name do you prefer?

@@ +35,5 @@
> +    debug("setMessage handler for [" + aType + "]");
> +    if (!(aType in this._handlers)) {
> +      this._handlers[aType] = [];
> +    }
> +    this._handlers[aType].push(aCallback);

With this code it will be possible to register twice the same callback function, is it expected?

@@ +94,5 @@
> +    this._manifest = null;
> +    try {
> +      let app = utils.getApp();
> +      this._manifest = app.manifestURL;
> +    } catch(e) { }

I assume this is for workers? If so let's add a small message that will help understand what happen when it fails.
Comment 17 Mounir Lamouri (:mounir) 2012-05-29 05:56:28 PDT
Comment on attachment 626963 [details] [diff] [review]
Part 1 : IDL

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

Jonas needs to sr this.

::: toolkit/toolkit-makefiles.sh
@@ +40,4 @@
>    dom/indexedDB/Makefile
>    dom/ipc/Makefile
>    dom/locales/Makefile
> +  dom/messages/Makefile

You are missing:
dom/messages/interfaces/Makefile
Comment 18 Mounir Lamouri (:mounir) 2012-05-29 06:07:10 PDT
Comment on attachment 626968 [details] [diff] [review]
Part 2 : adding getApp()

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

::: dom/base/nsGlobalWindow.cpp
@@ +10124,5 @@
> +nsresult
> +nsGlobalWindow::GetApp(mozIDOMApplication** aApplication)
> +{
> +  if (mIsApp != TriState_True || !mApp) {
> +    return NS_ERROR_FAILURE;

You shouldn't throw if |mApp| is null. This is something that could happen. I mean, we don't know in which situation this will be called.

Though, you might want to replace those two lines by this:
MOZ_ASSERT((mIsApp == TriState_True && mApp) || (mIsApp != TriState_True && !mApp));

Oh, actually, this needs to work if called on a child, right? In that case, you need to follow the window parents chain as done in ::IsPartOfApp(). You could actually move the logic of ::IsPartOfApp() in ::GetApp() and have IsPartOfApp() checking if ::GetApp() returns something or not.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1143,5 @@
>     * This method will throw an exception if the manifest URL isn't a valid URL
>     * or isn't the manifest URL of an installed application.
>     */
> +  void               setApp(in DOMString manifestURL);
> +  mozIDOMApplication getApp();

Add a comment saying it would be null if the window isn't associated with an application.
Comment 19 Mounir Lamouri (:mounir) 2012-05-29 06:21:36 PDT
Comment on attachment 626971 [details] [diff] [review]
Part 3 : DOM and internal parts

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

Just had a quick look at the JS files but a few things need to be fixed.

::: dom/base/Navigator.cpp
@@ +1204,5 @@
> +  mMessagesManager = do_CreateInstance("@mozilla.org/system-message-manager;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIDOMGlobalPropertyInitializer> gpi(do_QueryInterface(mMessagesManager));
> +  if (gpi) {

What should we do with |mMessagesManager| if |!gpi|? Maybe you should have a temporary value holder and, at the end, set |mMessagesManager| to the correct value. Otherwise, it will be incorrectly seen as set.

Also, you should do:
if (!gpi) {
  return NS_OK;
}

But do you really want NS_OK here? I guess Init() has to be called to have the manager working, right?

@@ +1208,5 @@
> +  if (gpi) {
> +    // we don't do anything with the return value.
> +    jsval prop_val = JSVAL_VOID;
> +    rv = gpi->Init(window, &prop_val);
> +    NS_ENSURE_SUCCESS(rv, rv);

Same as above regarding a failure.

::: dom/messages/Makefile.in
@@ +16,5 @@
> +EXTRA_COMPONENTS = \
> +  SystemMessageManager.js \
> +  SystemMessageInternal.js \
> +  SystemMessageManager.manifest \
> +  $(NULL)

I would have preferred a src/ directory.

::: dom/messages/SystemMessageManager.js
@@ +23,5 @@
> +// Implementation of the DOM API for system messages
> +
> +function SystemMessageManager() {
> +  this._handlers = {};
> +  this._hasPending = false;

Do you use that?

@@ +40,5 @@
> +
> +    // if we have a pending message, send it asynchronously
> +    if (this.hasPendingMessage(aType)) {
> +      let thread = Services.tm.mainThread;
> +      let pending = this._pending[aType];

So you can't handle more than one pending message?

@@ +65,5 @@
> +    return this._pending[aType] !== null;
> +  },
> +
> +  uninit: function()  {
> +    this._handlers = null;

Might be nice to reset |_pending| too.
Comment 20 [:fabrice] Fabrice Desré 2012-05-29 09:32:53 PDT
(In reply to Gene Lian [:gene] from comment #14)

> One thing that I don't quite understand when I try to integrate the system
> message in the Alarm API by following your testing codes here. The keyword
> "alarm" here seems to be assigned internally. Right? If so, how the App
> knows to call setMessageHandler(in DOMString type, ...) or
> hasPendingMessage(in DOMString type) function with the correct type (i.e.
> "alarm")? Or on the contrary, how the messenger here knows which message
> type it should send corresponding to the type specified by
> setMessageHandler(in DOMString type, ...)?

The messenger decides which message name it wants to use. We'll have to maintain this list to prevent name collisions, much like for event names.
 
> Btw, I had another question similar to the above-mentioned one: how to
> decide the pageURI and manifestURI in messenger.registerPage() if these two
> URIs could vary by different Apps?

registerPage() is an internal chrome-only call designed to be used when we'll allow apps to declare message handlers in their manifest. When a web page calls setMessageHandler(), we use the page's url and eventually the linked app manifest url.
Comment 21 [:fabrice] Fabrice Desré 2012-05-29 09:47:29 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #19)
 
> @@ +40,5 @@
> > +
> > +    // if we have a pending message, send it asynchronously
> > +    if (this.hasPendingMessage(aType)) {
> > +      let thread = Services.tm.mainThread;
> > +      let pending = this._pending[aType];
> 
> So you can't handle more than one pending message?

One of each type, yes. I implemented it this way since this was coherent we the singular form of hasPendingMessage(). If we want to support pending lists of messages, we'll have to rename to hasPendingMessages().
Comment 22 Mounir Lamouri (:mounir) 2012-05-29 12:40:08 PDT
(In reply to Fabrice Desré [:fabrice] from comment #21)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #19)
>  
> > @@ +40,5 @@
> > > +
> > > +    // if we have a pending message, send it asynchronously
> > > +    if (this.hasPendingMessage(aType)) {
> > > +      let thread = Services.tm.mainThread;
> > > +      let pending = this._pending[aType];
> > 
> > So you can't handle more than one pending message?
> 
> One of each type, yes. I implemented it this way since this was coherent we
> the singular form of hasPendingMessage(). If we want to support pending
> lists of messages, we'll have to rename to hasPendingMessages().

Suppose you have an application that receive two push notifications while running but doesn't have a handler for them. In that, we should make sure both notifications will be sent when the handler will be registered.
The name is correct here: if you have one message pending that doesn't mean you have only one but at least one; in the other hand, if you have pending message*s*, that means you have more than one and that should return false if you have only one, right?
Comment 23 [:fabrice] Fabrice Desré 2012-05-29 13:08:21 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #22)

> Suppose you have an application that receive two push notifications while
> running but doesn't have a handler for them. In that, we should make sure
> both notifications will be sent when the handler will be registered.

So if the application has a bug and never sets the message handler, we'll buffer potentially and endless list of messages? That could lead to OOM in extreme cases. We need a way to mitigate that.

> The name is correct here: if you have one message pending that doesn't mean
> you have only one but at least one; in the other hand, if you have pending
> message*s*, that means you have more than one and that should return false
> if you have only one, right?

ha yes, good point.
Comment 24 Mounir Lamouri (:mounir) 2012-05-30 02:25:48 PDT
(In reply to Fabrice Desré [:fabrice] from comment #23)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #22)
> 
> > Suppose you have an application that receive two push notifications while
> > running but doesn't have a handler for them. In that, we should make sure
> > both notifications will be sent when the handler will be registered.
> 
> So if the application has a bug and never sets the message handler, we'll
> buffer potentially and endless list of messages? That could lead to OOM in
> extreme cases. We need a way to mitigate that.

I would be okay to have specifications saying something that would only require implementations to keep a stack of pending messages but with some being dropped if there are too many in queue. Though, a couple of pending messages seem reasonable enough.
Comment 25 [:fabrice] Fabrice Desré 2012-05-31 12:45:51 PDT
Created attachment 628862 [details] [diff] [review]
Part 2 : adding getApp() v2
Comment 26 [:fabrice] Fabrice Desré 2012-05-31 12:47:57 PDT
Created attachment 628863 [details] [diff] [review]
Part 3 : DOM and internal parts v2

I changed the way pending message lists are maintained, this should be clearer now.
Comment 27 [:fabrice] Fabrice Desré 2012-05-31 12:49:07 PDT
Created attachment 628865 [details] [diff] [review]
Part 4 : b2g specific code v2

Vivien,

Minor changes to matches changes in the message format. I'll remove the test code before pushing.
Comment 28 [:fabrice] Fabrice Desré 2012-05-31 12:50:06 PDT
I have a test page and associated chrome code to generate events, but I don't know how to turn that into a proper test. Any help appreciated!
Comment 29 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-31 23:45:18 PDT
Comment on attachment 626963 [details] [diff] [review]
Part 1 : IDL

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

This interface will be implemented by the Navigator object, right?
Comment 30 [:fabrice] Fabrice Desré 2012-05-31 23:49:50 PDT
(In reply to Jonas Sicking (:sicking) from comment #29)
> 
> This interface will be implemented by the Navigator object, right?

Yes!
Comment 31 Mounir Lamouri (:mounir) 2012-06-01 02:00:11 PDT
Comment on attachment 628863 [details] [diff] [review]
Part 3 : DOM and internal parts v2

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

r+ for the dom/ code with the requested changes.
r- for the SystemMessageManager that seems to have some major bugs.

::: dom/base/Navigator.cpp
@@ +1200,5 @@
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> +  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
> +
> +  nsresult rv;
> +

nit: remove that empty line (after |nsresult rv;|).

@@ +1208,5 @@
> +  if (!gpi) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // we don't do anything with the return value.

nit: uppercase for the first letter ;)

@@ +1213,5 @@
> +  jsval prop_val = JSVAL_VOID;
> +  rv = gpi->Init(window, &prop_val);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mMessagesManager = messageManager;

You could even do:
mMessagesManager = messageManager.forget();
to save a few cycles ;)

@@ +1214,5 @@
> +  rv = gpi->Init(window, &prop_val);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mMessagesManager = messageManager;
> +  NS_ENSURE_SUCCESS(rv, rv);

I guess that NS_ENSURE_SUCCESS() got lost ;)

::: dom/base/Navigator.h
@@ +114,4 @@
>     */
>    void SetWindow(nsPIDOMWindow *aInnerWindow);
>  
> +  // helper to initialize mMessagesManager

nit:
// Helper to initialize mMessagesManager.

::: dom/messages/SystemMessageInternal.js
@@ +72,5 @@
> +    let page;
> +    this._pages.some(function(aPage) {
> +      if (aPage.uri == msg.uri &&
> +                 aPage.type == msg.type &&
> +                 aPage.manifest == msg.manifest) {

nit: identation issues here

::: dom/messages/SystemMessageManager.js
@@ +43,5 @@
> +      this._handlers[aType] = [];
> +    }
> +
> +    // don't register twice the same handler
> +    if (this._handlers[aType].indexOf(aHandler) == -1) {

Hmm, I think Jonas' idea was to have only *one* handler per type. I think we should replace the current handler if another is set.
In other words, this._handlers should contain be indexed per type and contain one handler for each type instead of an array.

@@ +65,5 @@
> +
> +  hasPendingMessage: function(aType) {
> +    debug("hasPendingMessage");
> +    if (this._pending[aType]) {
> +      return true;

So, imagine we do that:
hasPendingMessage('foo') returns false;
<- message coming
hasPendingMessage('foo') returns true;
<- message coming
hasPendingMessage('foo') returns true;
setMessageHandler('foo', handler);

I believe in that case, only the first message will be sent to the handler because the other ones will not be registered.

More generally, what would happen if I do:
<- message coming
<- message coming
<- message coming
setMessageHandler('foo', handler);
Will I get the handler called for the three messages?
Comment 32 Mounir Lamouri (:mounir) 2012-06-01 02:00:36 PDT
Comment on attachment 628862 [details] [diff] [review]
Part 2 : adding getApp() v2

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

r=me with the requested changes applied.

::: dom/base/nsGlobalWindow.cpp
@@ +10128,5 @@
>  bool
>  nsGlobalWindow::IsPartOfApp()
>  {
> +  mozIDOMApplication* app;
> +  nsresult rv = GetApp(&app);

if (NS_FAILED(GetApp(&app)) {
  return false;
}

@@ +10167,5 @@
> +
> +nsresult
> +nsGlobalWindow::GetApp(mozIDOMApplication** aApplication)
> +{
> +  FORWARD_TO_OUTER(GetApp, (aApplication), NS_OK);

|*aApplication = nsnull;| before calling FORWARD_TO_OUTER.

@@ +10177,4 @@
>      if (w->mIsApp == TriState_True) {
>        // The window should be part of an application.
>        MOZ_ASSERT(w->mApp);
> +      NS_IF_ADDREF(*aApplication = w->mApp);

You forgot:
return NS_OK;

@@ +10181,2 @@
>      } else if (w->mIsApp == TriState_False) {
> +      *aApplication = nsnull;

No need to set *aApplication given that it has a default value set at the beginning. Instead, call |return NS_OK;|.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1143,4 @@
>     * This method will throw an exception if the manifest URL isn't a valid URL
>     * or isn't the manifest URL of an installed application.
>     */
> +  void               setApp(in DOMString manifestURL);

nit: Don't change the alignment here. No method name is aligned in the interface and it's not a convention.
Comment 33 [:fabrice] Fabrice Desré 2012-06-04 12:38:18 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #31)
 
> ::: dom/messages/SystemMessageManager.js
> @@ +43,5 @@
> > +      this._handlers[aType] = [];
> > +    }
> > +
> > +    // don't register twice the same handler
> > +    if (this._handlers[aType].indexOf(aHandler) == -1) {
> 
> Hmm, I think Jonas' idea was to have only *one* handler per type. I think we
> should replace the current handler if another is set.
> In other words, this._handlers should contain be indexed per type and
> contain one handler for each type instead of an array.

Since many web pages are build by including different .js files, having only one handler for a given type on a page is inconvenient. Anyway, this could be reverted easily.
 
> @@ +65,5 @@
> > +
> > +  hasPendingMessage: function(aType) {
> > +    debug("hasPendingMessage");
> > +    if (this._pending[aType]) {
> > +      return true;
> 
> So, imagine we do that:
> hasPendingMessage('foo') returns false;
> <- message coming
> hasPendingMessage('foo') returns true;
> <- message coming
> hasPendingMessage('foo') returns true;
> setMessageHandler('foo', handler);
> 
> I believe in that case, only the first message will be sent to the handler
> because the other ones will not be registered.

If this is called from a web page (not an installed app that registered a handler in the manifest), we don't set a pending queue since we don't know which message it could want to listen to.

> More generally, what would happen if I do:
> <- message coming
> <- message coming
> <- message coming
> setMessageHandler('foo', handler);
> Will I get the handler called for the three messages?

Yes, if it's a page we open because it's registered.

Overall, I'm happy to make changes, but I feel we have not defined the behavior well enough yet.
Comment 34 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-05 04:21:31 PDT
Comment on attachment 628863 [details] [diff] [review]
Part 3 : DOM and internal parts v2

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

::: dom/messages/SystemMessageInternal.js
@@ +18,5 @@
> +// limit the number of pending messages for a given page
> +const kMaxPendingMessages = 5;
> +
> +function debug(aMsg) { 
> +  dump("-- SystemMessageInternal " + Date.now() + " : " + aMsg + "\n"); 

Make it silent by default.

@@ +37,5 @@
> +    debug("Broadcasting " + aType + " " + JSON.stringify(aMessage));
> +    ppmm.sendAsyncMessage("SystemMessageManager:Message" , { type: aType, msg: aMessage });
> +
> +    // trying to open pages that may not be open yet
> +    this._pages.forEach(function(aPage) {

nit: add a name to the anonymous callback

@@ +44,5 @@
> +
> +      aPage.pending.push(aMessage);
> +      if (aPage.pending.length > kMaxPendingMessages) {
> +        aPage.pending.splice(0, 1);
> +      }

I'm not really sure to understand the role of kMaxPendingMessages? Is it in the case of a webpage that never consumes the messages?
I wonder if the messages should be automatically dismissed once a page has been opened to react to them.

@@ +65,5 @@
> +    debug("received SystemMessageManager:GetPending");
> +    // this is a sync call, use to return the pending message for a page
> +    let msg = aMessage.json;
> +    debug(JSON.stringify(msg));
> +    let pending;

Declare 'pending' next to 'pending = page.pending', it is unused before.

::: dom/messages/SystemMessageManager.js
@@ +16,5 @@
> +  return Cc["@mozilla.org/childprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager).QueryInterface(Ci.nsISyncMessageSender);
> +});
> +
> +// limit the number of pending messages for a given type
> +const kMaxPendingMessages = 5;

I wish this const is not needed (see comment for the same line in SystemMessageInternal.js). But if you prove that it is needed you likely want to make it a preference to avoid the risk of having those values not synced.

@@ +19,5 @@
> +// limit the number of pending messages for a given type
> +const kMaxPendingMessages = 5;
> +
> +function debug(aMsg) { 
> +  dump("-- SystemMessageManager " + Date.now() + " : " + aMsg + "\n"); 

Make it silent by default.

@@ +30,5 @@
> +  // We can have several handlers for the same message type.
> +  this._handlers = {};
> +
> +  // Pending messages for this page, keyed by message type.
> +  this._pending = {};

pending -> pendings

@@ +36,5 @@
> +
> +SystemMessageManager.prototype = {
> +  __proto__: DOMRequestIpcHelper.prototype,
> +
> +  setMessageHandler: function(aType, aHandler) {

Since this code will come from a webpage it should be safe to check for aType and aHandler before doing anything else in the code.

@@ +40,5 @@
> +  setMessageHandler: function(aType, aHandler) {
> +    debug("setMessage handler for [" + aType + "]");
> +    if (!(aType in this._handlers)) {
> +      this._handlers[aType] = [];
> +    }

let handlers = this._handlers;
if (aType in handlers) {
  handlers[aType] = [];
}

@@ +62,5 @@
> +      });
> +    }
> +  },
> +
> +  hasPendingMessage: function(aType) {

Add names to your anonymous functions

@@ +65,5 @@
> +
> +  hasPendingMessage: function(aType) {
> +    debug("hasPendingMessage");
> +    if (this._pending[aType]) {
> +      return true;

let pendings = this._pendings;
if (aType in pendings)
  return true;

and then use pendings instead of accessing this._pendings all the time.

@@ +73,5 @@
> +    let messages = cpmm.sendSyncMessage("SystemMessageManager:GetPending", 
> +                                        { type: aType, 
> +                                          uri: this._uri, 
> +                                          manifest: this._manifest })[0];
> +    if (messages) {

if (!messages)
  return false;

@@ +74,5 @@
> +                                        { type: aType, 
> +                                          uri: this._uri, 
> +                                          manifest: this._manifest })[0];
> +    if (messages) {
> +      if (!this._pending[aType])

It seems like this condition is always |true| since you're checking it at the beginning of the method.

@@ +78,5 @@
> +      if (!this._pending[aType])
> +        this._pending[aType] = [];
> +
> +      // Doing that instead of pending.concat() to avoid array copy
> +      messages.forEach(function(aMessage) {

Add a name to your anonymous callback

@@ +104,5 @@
> +    if (!(msgType in this._handlers))
> +      return;
> +
> +    let message = aMessage.json.msg;
> +    this._handlers[msgType].forEach(function(aHandler) {

nit: add a name to your anonymous callback

@@ +123,5 @@
> +    try {
> +      let app = utils.getApp();
> +      if (app)
> +        this._manifest = app.manifestURL;
> +    } catch(e) { }

Why do you need a try/catch here? Does getApp throw instead of returning null?
Comment 35 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-05 04:23:10 PDT
Comment on attachment 628865 [details] [diff] [review]
Part 4 : b2g specific code v2

Seems like this is the wrong patch :)
Comment 36 Gene Lian [:gene] (I already quit Mozilla) 2012-06-06 21:18:09 PDT
(In reply to Fabrice Desré [:fabrice] from comment #20)
> (In reply to Gene Lian [:gene] from comment #14)
> 
> > Btw, I had another question similar to the above-mentioned one: how to
> > decide the pageURI and manifestURI in messenger.registerPage() if these two
> > URIs could vary by different Apps?
> 
> registerPage() is an internal chrome-only call designed to be used when
> we'll allow apps to declare message handlers in their manifest. When a web
> page calls setMessageHandler(), we use the page's url and eventually the
> linked app manifest url.

Fabrice,

I'd like to follow up with your previous response regarding the use of system message handler, hopping to make sure my understanding is correct :) 

So in theory, the Alarm API should never have chance to call the registerPage(), since it is internally used when an App calls setMessageHandler() to register the current ageURI and manifestURI where the setMessageHandler() is being called. Right?

If yes, supposing I need to send a system message from Alarm API, what I only need to do is to call the following 2 functions, where the "alarm" is a pre-defined keyword:

let messenger = Cc["@mozilla.org/system-message-internal;1"].getService(Ci.nsISystemMessagesInternal);
messenger.sendMessage("alarm", { ... });

Does this sound correct to you?

Thanks for your response in advance,
Gene
Comment 37 [:fabrice] Fabrice Desré 2012-06-06 21:32:32 PDT
(In reply to Gene Lian [:gene] from comment #36)
>
> let messenger =
> Cc["@mozilla.org/system-message-internal;1"].getService(Ci.
> nsISystemMessagesInternal);
> messenger.sendMessage("alarm", { ... });
> 
> Does this sound correct to you?

Yes, that's correct, you just need to do that. Note that this must happen in the parent process.
Comment 38 Gene Lian [:gene] (I already quit Mozilla) 2012-06-07 01:48:48 PDT
(In reply to Fabrice Desré [:fabrice] from comment #37)
> (In reply to Gene Lian [:gene] from comment #36)
> >
> > let messenger =
> > Cc["@mozilla.org/system-message-internal;1"].getService(Ci.
> > nsISystemMessagesInternal);
> > messenger.sendMessage("alarm", { ... });
> > 
> > Does this sound correct to you?
> 
> Yes, that's correct, you just need to do that. Note that this must happen in
> the parent process.

Fabrice,

Please see the discussion at https://groups.google.com/d/msg/mozilla.dev.webapi/o8bkwx0EtmM/Tocydr7_3gAJ when you have a chance.

It seems we still need a way to specify the specific app (i.e. ManifestURI) to send the system message to it. If both app A and app B use the same navigator.setMessageHandler("alarm", ...), an "alarm" system message later will be delivered to both app A and B, which is not correct because an alarm should only be set by one app.

Please let me know if I misunderstood anything :)

Thanks,
Gene
Comment 39 [:fabrice] Fabrice Desré 2012-06-12 18:00:51 PDT
Created attachment 632480 [details] [diff] [review]
Part 3 : DOM and internal parts

Addressing previous comments.
Comment 40 [:fabrice] Fabrice Desré 2012-06-12 18:02:07 PDT
Created attachment 632481 [details] [diff] [review]
Part 4 : b2g specific code v3

With the good patch this time ;)
Comment 41 [:fabrice] Fabrice Desré 2012-06-12 18:04:10 PDT
Created attachment 632483 [details] [diff] [review]
Part 5 : webapps manifest support

In this patch we check if webapps have a "messages" property in their manifest and use it to register handlers at startup or when installing an app.

My activities branch at https://github.com/fabricedesre/gaia/tree/activities adds one to the clock app.
Comment 42 Jose Manuel Cantera 2012-06-14 12:58:27 PDT
I don't understand the example on your branch. You have the messages property on the manifest and you call setMessageHandler function, thus it seems to be redundant ...
Comment 43 [:fabrice] Fabrice Desré 2012-06-14 13:36:45 PDT
(In reply to Jose M. Cantera from comment #42)
> I don't understand the example on your branch. You have the messages
> property on the manifest and you call setMessageHandler function, thus it
> seems to be redundant ...

No it's not. The messages property is used to register the application as a potential target for these messages, while setMessageHandler() actually set up the callback that receives them. If you just call setMessageHandler() without registering before, you won't get the messages.
Comment 44 Jose Manuel Cantera 2012-06-14 14:17:25 PDT
Thus If I add an alarm by using the Alarm API but I don't specify the messages field on the manifest, my app won't be awoken. That seems a bit strange. Why that registration on the manifest is needed in that case?
Comment 45 [:fabrice] Fabrice Desré 2012-06-14 14:26:42 PDT
(In reply to Jose M. Cantera from comment #44)
> Thus If I add an alarm by using the Alarm API but I don't specify the
> messages field on the manifest, my app won't be awoken. That seems a bit
> strange. Why that registration on the manifest is needed in that case?

Because different apps can use the alarm API, and the calendar app must not receive alarms that have been set up by the clock app for instance.
Comment 46 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-14 23:29:26 PDT
Comment on attachment 632480 [details] [diff] [review]
Part 3 : DOM and internal parts

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

r+ with the debug code turned off.

::: dom/messages/SystemMessageInternal.js
@@ +21,5 @@
> +  kMaxPendingMessages = Services.prefs.getIntPref("dom.messages.maxPendingMessages");
> +} catch(e) { }
> +
> +function debug(aMsg) { 
> +  dump("-- SystemMessageInternal " + Date.now() + " : " + aMsg + "\n"); 

Turn it off by default (you can just // the line)

::: dom/messages/SystemMessageManager.js
@@ +22,5 @@
> +  kMaxPendingMessages = Services.prefs.getIntPref("dom.messages.maxPendingMessages");
> +} catch(e) { }
> +
> +function debug(aMsg) { 
> +  dump("-- SystemMessageManager " + Date.now() + " : " + aMsg + "\n"); 

Turn it off by default (you can just // the line)

@@ +40,5 @@
> +SystemMessageManager.prototype = {
> +  __proto__: DOMRequestIpcHelper.prototype,
> +
> +  setMessageHandler: function sysMessMgr_setMessageHandler(aType, aHandler) {
> +    if (!aType || !aHandler) {

Make the check harder: !aHandler -> (typeof aHandler !== 'function')

@@ +61,5 @@
> +    if (this.hasPendingMessage(aType)) {
> +      let thread = Services.tm.mainThread;
> +      let pending = this._pendings[aType];
> +      this._pendings[aType] = null;
> +      pending.forEach(function(aPending) {

nit: add a name

@@ +93,5 @@
> +        pendings[aType].splice(pendings.length, 0, aMessage);
> +        if (pendings.length > kMaxPendingMessages) {
> +          pendings.splice(0, 1);
> +        }
> +      }.bind(this));

nit: you don't need to call bind

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +10,5 @@
> +
> +[scriptable, uuid(fdc1ba03-5d8f-4de9-894a-333c7a136c5f)]
> +interface nsISystemMessagesInternal : nsISupports
> +{
> +	/*

nit: the indentation seems wrong.
Comment 47 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-14 23:31:34 PDT
Comment on attachment 632481 [details] [diff] [review]
Part 4 : b2g specific code v3

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

r- because of the code to register the alarm clock by default.

::: b2g/chrome/content/shell.js
@@ +331,5 @@
>  };
>  
> +// listen for system messages and relay them to Gaia
> +Services.obs.addObserver(function(aSubject, aTopic, aData) {
> +  dump("XxXxX got system-messages-open-app\n");

remove this dump

@@ +601,5 @@
> +let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +timer.initWithCallback(function() {
> +  dump("Firing alarm\n");
> +  messenger.sendMessage("alarm", { time: Date.now(), reason: "get up!" }, Services.io.newURI("http://clock.foo.org/manifest.webapp", null, null));
> +}, 5000, timer.TYPE_REPEATING_SLACK);

Leftover?

::: b2g/installer/package-manifest.in
@@ +306,4 @@
>  @BINPATH@/components/xuldoc.xpt
>  @BINPATH@/components/xultmpl.xpt
>  @BINPATH@/components/zipwriter.xpt
> +@BINPATH@/components/dom_messages.xpt

This is ordered alphabetically, isn't it?
Comment 48 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-14 23:38:09 PDT
Comment on attachment 632483 [details] [diff] [review]
Part 5 : webapps manifest support

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

Sounds good but I would like to understand the aResult[0] part.

::: b2g/chrome/content/shell.js
@@ -596,5 @@
>  let messenger = Cc["@mozilla.org/system-message-internal;1"].getService(Ci.nsISystemMessagesInternal);
>  
> -messenger.registerPage("alarm", Services.io.newURI("http://clock.foo.org/", null, null), 
> -                                Services.io.newURI("http://clock.foo.org/manifest.webapp", null, null));
> -

I don't think we want that.

::: dom/apps/src/Webapps.jsm
@@ +77,5 @@
> +    if (aManifest.messages && Array.isArray(aManifest.messages) && aManifest.messages.length > 0) {
> +      let manifest = new DOMApplicationManifest(aManifest, aApp.origin);
> +      let launchPath = Services.io.newURI(manifest.fullLaunchPath(), null, null);
> +      let manifestURL = Services.io.newURI(aApp.manifestURL, null, null);
> +      aManifest.messages.forEach(function(aMessage) {

nit: add a name to your anonymous function

@@ +83,5 @@
> +      });
> +    }
> +  },
> +
> +  _registerSystemMessagesForId: function(aId) {

nit: add a name to your anonymous function

@@ +85,5 @@
> +  },
> +
> +  _registerSystemMessagesForId: function(aId) {
> +    let app = this.webapps[aId];
> +    this._readManifests([{ id: aId }], (function(aResult) {

nit: add a name to your anonymous function

@@ +86,5 @@
> +
> +  _registerSystemMessagesForId: function(aId) {
> +    let app = this.webapps[aId];
> +    this._readManifests([{ id: aId }], (function(aResult) {
> +      this._registerSystemMessages(aResult[0].manifest, app);

Why don't you loop over the result instead of using aResult[0]?
Comment 49 [:fabrice] Fabrice Desré 2012-06-15 17:58:33 PDT
Created attachment 633742 [details] [diff] [review]
Part 4 : b2g specific code v4

Removed all the useless leftovers and reordered the .xpt in the package manifest.
Comment 50 [:fabrice] Fabrice Desré 2012-06-15 18:00:55 PDT
(In reply to Vivien Nicolas (:vingtetun) from comment #48)

> @@ +86,5 @@
> > +
> > +  _registerSystemMessagesForId: function(aId) {
> > +    let app = this.webapps[aId];
> > +    this._readManifests([{ id: aId }], (function(aResult) {
> > +      this._registerSystemMessages(aResult[0].manifest, app);
> 
> Why don't you loop over the result instead of using aResult[0]?

Since we pass only one id to _readManifests, we know that we'll only get get one result.

I added the missing anonymous function names in my local patch.
Comment 51 Mounir Lamouri (:mounir) 2012-06-18 09:39:30 PDT
(In reply to Jose M. Cantera from comment #44)
> Thus If I add an alarm by using the Alarm API but I don't specify the
> messages field on the manifest, my app won't be awoken.

Actually, when we designed this API with Jonas, we thought that the app should still be awoken but the event should be sent to the default page.
If the app is currently awake, the event should be sent to the current page.
Comment 52 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-18 10:16:15 PDT
Actually, for consistency, I think that if the message isn't listed in the manifest, we should either always send to the default page, or always block the message. Doing something different if the app is currently running seems error prone since you can easily end up with things working if the app isn't running, but break if the app is running, or work if the app is running but break if the app isn't running.
Comment 53 Mounir Lamouri (:mounir) 2012-06-19 11:19:58 PDT
Comment on attachment 632480 [details] [diff] [review]
Part 3 : DOM and internal parts

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

r- because you are allowing more than one handler. setHandler() should always over-write the previous one.

Also, looking at your code, I believe this wouldn't work as expected:

setHandler(handler);
<- message coming
<- message coming
hasPendingMessage();

That last call should return 0 but it seems like it would return 2 with your implementation.

::: dom/base/Navigator.cpp
@@ +1233,5 @@
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIDOMNavigatorSystemMessages> messageManager = do_CreateInstance("@mozilla.org/system-message-manager;1", &rv);
> +  
> +  nsCOMPtr<nsIDOMGlobalPropertyInitializer> gpi(do_QueryInterface(messageManager));

nit:
nsCOMPtr<nsIDOMGlobalPropertyInitializer> gpi = do_QueryInterface(messageManager);

is nicer ;)

@@ +1235,5 @@
> +  nsCOMPtr<nsIDOMNavigatorSystemMessages> messageManager = do_CreateInstance("@mozilla.org/system-message-manager;1", &rv);
> +  
> +  nsCOMPtr<nsIDOMGlobalPropertyInitializer> gpi(do_QueryInterface(messageManager));
> +  if (!gpi) {
> +    return NS_ERROR_FAILURE;

NS_ENSURE_TRUE(gpi, NS_ERROR_FAILURE);

@@ +1251,5 @@
> +
> +NS_IMETHODIMP
> +Navigator::HasPendingMessage(nsAString const& aType, bool *aResult)
> +{
> +  nsresult rv = EnsureMessagesManager();

Hmm, |*aResult = false;| wouldn't hurt.

@@ +1260,5 @@
> +
> +NS_IMETHODIMP
> +Navigator::SetMessageHandler(nsAString const& aType, nsIDOMSystemMessageCallback *aCallback)
> +{
> +  nsresult rv = EnsureMessagesManager();

Same here, |*aCallback = nsnull;| wouldn't hurt.

::: dom/messages/SystemMessageInternal.js
@@ +15,5 @@
> +  return Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
> +});
> +
> +// limit the number of pending messages for a given page
> +let kMaxPendingMessages = 5;

I would put this in the |catch| block and say as a comment that |getIntPref| throws if there is no user defined value for that pref.

@@ +59,5 @@
> +
> +  registerPage: function registerPage(aType, aPageURI, aManifestURI) {
> +    this._pages.push({ type: aType,
> +                       uri: aPageURI.spec,
> +                       manifest: aManifestURI ? aManifestURI.spec : "",

Shouldn't we throw if |!aManifest|?

::: dom/messages/SystemMessageManager.js
@@ +53,5 @@
> +    }
> +
> +    // don't register twice the same handler
> +    if (handlers[aType].indexOf(aHandler) == -1) {
> +      handlers[aType].push(aHandler);

There should be only one handler for each type. This should not be an array.

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +15,5 @@
> +     * Allow any internal user to broadcast a message of a given type.
> +     * @param type        The type of the message to be sent.
> +     * @param message     The message payload.
> +     * @param manifestURI Set this URI to the app manifest URI if the message must only be 
> +     *                    dispatched to an app.

I guess you forgot to update that comment.
Comment 54 [:fabrice] Fabrice Desré 2012-06-19 14:38:59 PDT
Created attachment 634596 [details] [diff] [review]
Part 3 : DOM and internal parts v3

Addressing Mounir's comments.

Also, I made sure that getPendingMessage() is actually false when it should.
Comment 55 Gene Lian [:gene] (I already quit Mozilla) 2012-06-22 01:47:36 PDT
Fabrice,

Just a quick message to let you know. Although the review is not yet completely done, I've already integrated your current patches (Part 1 ~ Part 5) and release a temporal Gecko to front-end engineers, where the Alarm API can use the system message mechanism to send "alarm" messages to a specified manifestURL.

  messenger.sendMessage("alarm", currentAlarm, currentAlarm.manifestURL);

Please let me know if you also had some local changes on your end that haven't been uploaded and should be essential. The performance is still under testing and hope everything works as expected :)

Thanks,
Gene
Comment 56 Mounir Lamouri (:mounir) 2012-06-25 02:47:24 PDT
Comment on attachment 634596 [details] [diff] [review]
Part 3 : DOM and internal parts v3

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

General nit: put an upper-case character at the beginning of a comment line and a dot at the end.

I think there are still some issues with SetMessageHandler and HasPendingMessages. I see at least two cases that wouldn't work as expected:
1.
setMessageHandler(...)
<- one message coming
<- another message coming
hasPendingMessage(); // will return true, should return false

2.
<- one message coming
hasPendingMessage(); // will return true;
<- another message coming
hasPendingMessage(); // will return true;
setMessageHandler(); // will execute the first message but not the second

Also, make sure that this works: (it does for the moment, but keep it in mind)
setMessageHandler('type', handler);
// stuff happening
setMessageHandler('type' null);
<- one message coming
hasPendingMessage('type'); // will return true

::: b2g/chrome/content/shell.js
@@ +645,5 @@
> +let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +timer.initWithCallback(function() {
> +    dump("Firing alarm\n");
> +    messenger.sendMessage("alarm", { time: Date.now(), reason: "get up!" }, Services.io.newURI("http://clock.foo.org/manifest.webapp", null, null));
> +    }, 5000, timer.TYPE_REPEATING_SLACK);

I guess all of this is a test and has been put in the patch by mistake, right?

::: dom/base/Navigator.cpp
@@ +1251,5 @@
> +Navigator::HasPendingMessage(nsAString const& aType, bool *aResult)
> +{
> +  nsresult rv = EnsureMessagesManager();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  *aResult = false;

No, this has to be set at the beginning of the method so in case of early return, |*aResult| has a defined value.

@@ +1257,5 @@
> +  return mMessagesManager->HasPendingMessage(aType, aResult);
> +}
> +
> +NS_IMETHODIMP
> +Navigator::SetMessageHandler(nsAString const& aType, nsIDOMSystemMessageCallback *aCallback)

nit: I'm pretty sure this line doesn't follow the 80 columns max rule.

::: dom/messages/SystemMessageInternal.js
@@ +41,5 @@
> +
> +SystemMessageInternal.prototype = {
> +  sendMessage: function sendMessage(aType, aMessage, aManifestURI) {
> +    debug("Broadcasting " + aType + " " + JSON.stringify(aMessage));
> +    ppmm.sendAsyncMessage("SystemMessageManager:Message" , { type: aType, msg: aMessage });

It seems that this will send a message to *all* apps listening to it. It's not really what we want. Only the targeted app should get the message.

@@ +62,5 @@
> +  },
> +
> +  registerPage: function registerPage(aType, aPageURI, aManifestURI) {
> +    if (!aPageURI || !aManifestURI) {
> +      throw Cr.NS_ERROR_FAILURE;

Maybe you could use NS_ERROR_INVALID_ARG.

::: dom/messages/SystemMessageManager.js
@@ +45,5 @@
> +
> +  setMessageHandler: function sysMessMgr_setMessageHandler(aType, aHandler) {
> +    debug("setMessage handler for [" + aType + "] " + aHandler);
> +    if (!aType || !aHandler) {
> +      // Just bail out if parameters are incorrects.

Maybe you should allow aHandler to be null? In that case, it will just remove the handler.

@@ +76,5 @@
> +    if (aType in pendings) {
> +      return pendings[aType].length != 0;
> +    }
> +
> +    //Send a sync message to the parent to check if we have a pending message for this type

nit: // Send [..] type.

@@ +81,5 @@
> +    let messages = cpmm.sendSyncMessage("SystemMessageManager:GetPending", 
> +                                        { type: aType, 
> +                                          uri: this._uri, 
> +                                          manifest: this._manifest })[0];
> +    if (messages) {

if (!messages) {
  return false;
}

@@ +94,5 @@
> +        }
> +      });
> +    }
> +
> +    return !!messages;

retur true;

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +10,5 @@
> +
> +[scriptable, uuid(fdc1ba03-5d8f-4de9-894a-333c7a136c5f)]
> +interface nsISystemMessagesInternal : nsISupports
> +{
> +	/*

nit: indentation issue
Comment 57 [:fabrice] Fabrice Desré 2012-06-25 15:54:30 PDT
Created attachment 636516 [details] [diff] [review]
Part 3 : DOM and internal parts v4

Addressing comments, and fixed the behavior in different scenariis involving calling hasPendingMessage() at various places.

I filed bug 768228 to add proper tests, converting my existing tests that use a gaia patch. Mounir, let me know if you want to them run.
Comment 58 [:fabrice] Fabrice Desré 2012-06-26 02:26:06 PDT
Created attachment 636637 [details] [diff] [review]
Part 1 : IDL v2

Updating to MozPrefix.
Comment 59 [:fabrice] Fabrice Desré 2012-06-26 02:27:08 PDT
Created attachment 636638 [details] [diff] [review]
Part 3 : DOM and internal parts v5

Updating to MozPrefix.
Comment 60 Mounir Lamouri (:mounir) 2012-06-26 15:25:34 PDT
Comment on attachment 636637 [details] [diff] [review]
Part 1 : IDL v2

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

Kyle should review the configure.in changes.

Please, re-ask a sr with the changes in the interface names (I know, this is annoying... :().

::: dom/messages/interfaces/nsIDOMNavigatorSystemMessages.idl
@@ +4,5 @@
> +
> +#include "domstubs.idl"
> +
> +[scriptable, function, uuid(42692976-57fd-4bb4-ab95-2b97ebdc5056)]
> +interface nsIDOMSystemMessageCallback : nsISupports

I think you can name this nsISystemMessageCallback so it will not pollute the window scope.

@@ +10,5 @@
> +    void handleMessage(in jsval message);
> +};
> +
> +[scriptable, uuid(091e90dd-0e8b-463d-8cdc-9225d3a6ff90)]
> +interface nsIDOMNavigatorSystemMessages : nsISupports

Maybe you could do the same here. Otherwise, you will have to prefix with 'Moz'.
Comment 61 Mounir Lamouri (:mounir) 2012-06-27 01:10:43 PDT
Comment on attachment 636638 [details] [diff] [review]
Part 3 : DOM and internal parts v5

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

r=me with the following comments fixed.

::: dom/base/Navigator.cpp
@@ +175,5 @@
>    }
>  #endif
> +
> +  if (mMessagesManager) {
> +    mMessagesManager = nsnull;

That should be inside #ifdef too.

@@ +1234,5 @@
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIDOMNavigatorSystemMessages> 
> +    messageManager = do_CreateInstance("@mozilla.org/system-message-manager;1", 
> +                                       &rv);

nit: what about:
nsCOMPtr<nsIDOMNavigatorSystemMessages> messageManager =
  do_CreateInstance("@mozilla.org/system-message-manager;1", &rv);

@@ +1237,5 @@
> +    messageManager = do_CreateInstance("@mozilla.org/system-message-manager;1", 
> +                                       &rv);
> +  
> +  nsCOMPtr<nsIDOMGlobalPropertyInitializer>
> +    gpi = do_QueryInterface(messageManager);

nit: same here.

@@ +1251,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +Navigator::MozHasPendingMessage(nsAString const& aType, bool *aResult)

nit: const nsAString& aType

@@ +1261,5 @@
> +  return mMessagesManager->MozHasPendingMessage(aType, aResult);
> +}
> +
> +NS_IMETHODIMP
> +Navigator::MozSetMessageHandler(nsAString const& aType, 

nit: const nsAString& aType

@@ +1271,5 @@
> +  return mMessagesManager->MozSetMessageHandler(aType, aCallback);
> +}
> +#else
> +NS_IMETHODIMP
> +Navigator::MozHasPendingMessage(nsAString const& aType, bool *aResult)

Instead of having two method definitions, I would have put the #ifdef inside the methods like:
NS_IMETHODIMP
Navigator::MozSetMessageHandler(const nsAString& aType, nsIDOMSystemMessageCallback *aCallback)
{
#ifdef MOZ_SYS_MSG
  nsresult rv = EnsureMessageManager();
  NS_ENSURE_SUCCESS(rv, rv);

  return mMessagesManager->MozSetMessageHandler(aType, aCallback);
#else
  return NS_ERRROR_NOT_IMPLEMENTED;
#endif
}

::: dom/base/Navigator.h
@@ +81,4 @@
>  #ifdef MOZ_B2G_BT
>                  , public nsIDOMNavigatorBluetooth
>  #endif
> +                , public nsIDOMNavigatorSystemMessages

Isn't that interface prefixed with Moz? It should.

@@ +153,4 @@
>  #ifdef MOZ_B2G_BT
>    nsCOMPtr<nsIDOMBluetoothManager> mBluetooth;
>  #endif
> +  nsCOMPtr<nsIDOMNavigatorSystemMessages> mMessagesManager;

Would be nice to put that inside #ifdef MOZ_SYS_MSG so we can save a few bits here.

::: dom/messages/SystemMessageInternal.js
@@ +48,5 @@
> +
> +    // Queue the message for pages that registered an handler for this type.
> +    this._pages.forEach(function sendMess_openPage(aPage) {
> +      if (aPage.type != aType || aPage.manifest != aManifestURI.spec)
> +        return;

Could you print an error message if that happens? This would have to assert if that was in C++.

::: dom/messages/SystemMessageManager.js
@@ +51,5 @@
> +      // Just bail out if we have no type.
> +      return;
> +    }
> +
> +    let handlers = this._handlers;

not: not needed.

@@ +77,5 @@
> +      });
> +    }
> +  },
> +
> +  _getPendingMessages: function sysMessMgr_getPendingMessages(aType, aFromHandler) {

Rename aFromHandler to aForceUpdate.

@@ +79,5 @@
> +  },
> +
> +  _getPendingMessages: function sysMessMgr_getPendingMessages(aType, aFromHandler) {
> +    debug("hasPendingMessage " + aType);
> +    let pendings = this._pendings;

Do you really need that?

@@ +96,5 @@
> +                                          uri: this._uri,
> +                                          manifest: this._manifest })[0];
> +    if (!messages) {
> +      // No new pending messages, but the queue may not be empty yet.
> +      return pendings.length != 0;

Shouldn't that be pendings[aType].length?

@@ +106,5 @@
> +    // Doing that instead of pending.concat() to avoid array copy
> +    messages.forEach(function hpm_addPendings(aMessage) {
> +      pendings[aType].push(aMessage);
> +      if (pendings.length > kMaxPendingMessages) {
> +        pendings.splice(0, 1);

ditto

@@ +110,5 @@
> +        pendings.splice(0, 1);
> +      }
> +    });
> +
> +    return pendings.length != 0;

ditto

@@ +128,5 @@
> +          aMessage.json.type + " for " + aMessage.json.manifest +
> +          " (" + this._manifest + ")");
> +
> +    let msg = aMessage.json;
> +    if (msg.manifest != this._manifest)

The page uri has no importance?
Let say I have an APP A with Page 1 registering for a message handler and Page 2 is opened when the message comes. We will try to send the message to Page 2. Is that what we want? I would expect us to open Page 1 instead.
Comment 62 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-29 13:50:44 PDT
Why do you want a configure flag?
Comment 63 [:fabrice] Fabrice Desré 2012-06-29 16:36:46 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #62)
> Why do you want a configure flag?

For now I we only support b2g, but I want it to build on both device and desktop builds. If there's a way to #ifdef that without adding a new flag, I'll be happy to use that.
Comment 64 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-07-02 10:01:33 PDT
Comment on attachment 636637 [details] [diff] [review]
Part 1 : IDL v2

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

You don't need a configure option.

https://bugzilla.mozilla.org/show_bug.cgi?id=769460#c1
Comment 65 [:fabrice] Fabrice Desré 2012-07-02 15:49:50 PDT
Created attachment 638522 [details] [diff] [review]
Part 1 : IDL v3

Updated to remove the configure flag, and just keep the AC_SUBST macro.
Comment 66 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-07-02 16:04:52 PDT
Comment on attachment 638522 [details] [diff] [review]
Part 1 : IDL v3

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

There's no need for a dom/messages/interfaces directory.  Just move that up into dom/messages.
Comment 69 :Ms2ger (⌚ UTC+1/+2) 2012-07-03 05:36:46 PDT
Comment on attachment 628862 [details] [diff] [review]
Part 2 : adding getApp() v2

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

::: dom/base/nsGlobalWindow.cpp
@@ +10128,5 @@
>  bool
>  nsGlobalWindow::IsPartOfApp()
>  {
> +  mozIDOMApplication* app;
> +  nsresult rv = GetApp(&app);

This leaks...
Comment 70 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-07-03 06:34:03 PDT
Indeed it does.  Does this code not have tests?
Comment 71 Jeremie Patonnier :Jeremie 2013-04-08 09:07:35 PDT
Basic documentation has been provide here:

 - https://developer.mozilla.org/en-US/docs/DOM/window.navigator.mozSetMessageHandler
 - https://developer.mozilla.org/en-US/docs/DOM/window.navigator.mozHasPendingMessage

As there is very few information out their about that API, it needs a serious technical review.

Note You need to log in before you can comment on or make changes to this bug.