See the discussion in the webapi mailing list: https://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/a3c6e4c31d04b663

I should have a wip ready tomorrow

IDL for the DOM part, to be exposed on navigator

Mounir for the c++, vingtetun the js parts.

There is a bit of test code, that works with https://github.com/fabricedesre/gaia/tree/activities

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 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 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 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 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+.

Addressed comments from comment #6

I went with this option: mozIDOMApplication getApp()

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 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

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 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 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 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 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.

(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.

(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().

(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?

(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.

(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.

I changed the way pending message lists are maintained, this should be clearer now.

Vivien, Minor changes to matches changes in the message format. I'll remove the test code before pushing.

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 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?

(In reply to Jonas Sicking (:sicking) from comment #29) > > This interface will be implemented by the Navigator object, right? Yes!

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 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.

(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 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 on attachment 628865 [details] [diff] [review] Part 4 : b2g specific code v2 Seems like this is the wrong patch :)

(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

(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.

(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

Addressing previous comments.

With the good patch this time ;)

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.

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 ...

(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.

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?

(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 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 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 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]?

Removed all the useless leftovers and reordered the .xpt in the package manifest.

(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.

(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.

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 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.

Addressing Mounir's comments. Also, I made sure that getPendingMessage() is actually false when it should.

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 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

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.

Updating to MozPrefix.

Updating to MozPrefix.

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 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.

Why do you want a configure flag?

(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 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

Updated to remove the configure flag, and just keep the AC_SUBST macro.

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a45c3f6f8f https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9a1c775077 https://hg.mozilla.org/integration/mozilla-inbound/rev/2155615b4670 https://hg.mozilla.org/integration/mozilla-inbound/rev/ae504d48bcb0 https://hg.mozilla.org/integration/mozilla-inbound/rev/452df149aafc

https://hg.mozilla.org/mozilla-central/rev/d1a45c3f6f8f https://hg.mozilla.org/mozilla-central/rev/8b9a1c775077 https://hg.mozilla.org/mozilla-central/rev/2155615b4670 https://hg.mozilla.org/mozilla-central/rev/ae504d48bcb0 https://hg.mozilla.org/mozilla-central/rev/452df149aafc

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...

Indeed it does. Does this code not have tests?

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.