We're going to start out by prototyping the two main intents proposals in gaia, and when that prototype is stable, move it into gecko. https://github.com/andreasgal/gaia/issues/237 This bug is a placeholder for the later gecko impl.

we will need to review this, but it probably needs more bake time till then

Curtis, did you put Daniel as an assignee on purpose?

Yes, but now that I look at it again I am removing him and just leaving him as secr:

Posting my wip patches to get some feedback. Test pages are at : http://people.mozilla.org/~fdesre/activities/ (load this one to register a couple activities) http://people.mozilla.org/~fdesre/activities/launch.html to test startActivity() Known issues: - for now I anchored the API to navigator.mozActivities and not just on navigator. I have no strong opinion there, and I'll be happy to change if needed. - the matching of mime types is just a string equality (eg, no image/* support yet). - events are dispatched and can be acted upon using addEventListener("mozactivity",...) but setting onmozactivity=... doesn't work. I probably didn't add enough event magic yet. Help! - on b2g, use https://github.com/andreasgal/gaia/pull/1241 to get Gaia support. We rely on the promp.select() call for the activity chooser (works on desktop, untested but likely broken on device).

Comment on attachment 614963 [details] [diff] [review] Part 1 : IDL Review of attachment 614963 [details] [diff] [review]: ----------------------------------------------------------------- Could you add the file to dom/activities/interfaces/ instead?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10) > Comment on attachment 614963 [details] [diff] [review] > Part 1 : IDL > > Review of attachment 614963 [details] [diff] [review]: > ----------------------------------------------------------------- > > Could you add the file to dom/activities/interfaces/ instead? Sorry, I didn't mean to press "Submit". I will finish that feedback a bit later.

Comment on attachment 614963 [details] [diff] [review] Part 1 : IDL Review of attachment 614963 [details] [diff] [review]: ----------------------------------------------------------------- Could you add the file to dom/activities/interfaces/ instead? Also, I would prefer one idl file per interface but it's up to you. ::: dom/Makefile.in @@ +76,4 @@ > $(NULL) > > DIRS += \ > + activities \ You are not adding this directory... but you should :) ::: dom/interfaces/activities/nsIDOMActivities.idl @@ +13,5 @@ > +interface nsIDOMActivity : nsISupports > +{ > + readonly attribute DOMString action; > + readonly attribute DOMString type; > + readonly attribute jsval data; I guess those three attributes could return null. Could you add a comment saying that. @@ +31,5 @@ > + void initActivityEvent(in DOMString typeArg, > + in boolean canBubbleArg, > + in boolean cancelableArg, > + in nsIDOMDOMRequest requestArg, > + in nsIDOMActivity activityArg); Nowadays, we don't use initFoo() but a ctor. See: https://bug738131.bugzilla.mozilla.org/attachment.cgi?id=612652 @@ +39,5 @@ > +interface nsIDOMActivityManager : nsISupports > +{ > + nsIDOMDOMRequest startActivity(in DOMString action, in DOMString type, in jsval data); > + nsIDOMDOMRequest registerActivity(in DOMString action, in DOMString type, in DOMString title); > + nsIDOMDOMRequest unregisterActivity(in DOMString action, in DOMString type); register and unregister were out of my proposal on purpose. Do you have any use case for those in an app context? or do you need them to manage activities? @@ +42,5 @@ > + nsIDOMDOMRequest registerActivity(in DOMString action, in DOMString type, in DOMString title); > + nsIDOMDOMRequest unregisterActivity(in DOMString action, in DOMString type); > + > + // request.result will be an array of nsIDOMActivity objects > + nsIDOMDOMRequest getRegisteredActivities(); Again, this wasn't in my proposal. Why is that needed? @@ +51,5 @@ > +[scriptable, uuid(1f17afd1-547b-40bf-9600-b356f64f83c4)] > +interface nsIInternalActivity : nsIDOMActivity { > + readonly attribute DOMString title; > + readonly attribute DOMString uri; > + void init(in DOMString action, in DOMString type, in DOMString data, in DOMString title, in DOMString uri); Can you make a ctor instead? The same way Activity should have a ctor. @@ +55,5 @@ > + void init(in DOMString action, in DOMString type, in DOMString data, in DOMString title, in DOMString uri); > +}; > + > +[scriptable, uuid(3e5eeb79-a206-41fd-a531-dad5139bd6b5)] > +interface nsIActivityGlue : nsISupports I guess this is fully internal? Assuming that, I'm not looking at it.

Comment on attachment 614966 [details] [diff] [review] Part 3 : Events Review of attachment 614966 [details] [diff] [review]: ----------------------------------------------------------------- Olli (:smaug) should do the review of this patch. It seems generally good except for the ctor (as said in part1 comments). ::: content/events/public/nsEventNameList.h @@ +371,4 @@ > EventNameType_HTMLXUL, > NS_EVENT) > > +EVENT(mozactivity, I believe that should be WINDOW_EVENT.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #12) > @@ +31,5 @@ > > + void initActivityEvent(in DOMString typeArg, > > + in boolean canBubbleArg, > > + in boolean cancelableArg, > > + in nsIDOMDOMRequest requestArg, > > + in nsIDOMActivity activityArg); > > Nowadays, we don't use initFoo() but a ctor. > See: https://bug738131.bugzilla.mozilla.org/attachment.cgi?id=612652 I guess it means bug 723206 should be resolved first.

Comment on attachment 614968 [details] [diff] [review] Part 5 : b2g support >diff --git a/b2g/components/ActivitiesGlue.js b/b2g/components/ActivitiesGlue.js >+ActivitiesGlue.prototype = { >+ >+ run: function() { >+ let strings = []; >+ this.activities.forEach(function(item) { >+ strings.push(item.title); >+ }); >+ let choice = {}; >+ // XXX : TODO l10n >+ if (Services.prompt.select(null, "Activity chooser", "Select a provider", strings.length, strings, choice)) >+ Services.DOMRequest.fireSuccess(this.req, this.activities[choice.value]); >+ else >+ Services.DOMRequest.fireError(this.req, "NO_PROVIDER_SELECTED"); This UI should be retrieved from the content to let it be customizable/localizable by the content. It's a hard problem for the moment, can you open a followup for that? >+ }, >+ >+ chooseActivity: function(aWindow, aActivities) { >+ this.req = Services.DOMRequest.createRequest(aWindow); >+ this.activities = aActivities; >+ Services.tm.currentThread.dispatch(this, Ci.nsIEventTarget.DISPATCH_NORMAL); >+ return this.req; >+ }, >+ Does having it as a nsIRunnable is a way to make the Services.prompt.select call block the current JS thread until it returns instead of blocking the main thread? >+ getWindow: function(aActivity) { >+ let browserWin = Services.wm.getMostRecentWindow("navigator:browser"); >+ let content = browserWin.content; >+ let id = "activity" + Math.random(); >+ let event = content.document.createEvent("CustomEvent"); >+ event.initCustomEvent("mozChromeEvent", true, true, >+ { type: "activity-openwindow", uri: aActivity.uri, title: aActivity.title, id: id }); >+ let window = null; >+ >+ // XXX event dispatching is synchronous so that should work. Color me surprised. >+ // We should rather have a dedicated API. >+ content.addEventListener("mozContentEvent", function act_getWindow(aEvent) { >+ if (evt.detail.id != id) >+ return; >+ >+ content.removeEventListener("mozContentEvent", act_getWindow); >+ window = evt.detail.window; >+ }); >+ >+ content.dispatchEvent(event); >+ dump("XxXxX getWindow = " + window + "\n"); >+ return window; >+ }, What do you think of exposing a 'window' property on the mozApps API? If the application is opened you get the window associated to it. If it is not opened you can open it via mozApps.launch() and wait until it is started to retrieve the window property. This will let this code retrieve the correct window by looping over the apps and they registered activities (and this will let background service get a reference to their main window to use postMessage).

Comment on attachment 614965 [details] [diff] [review] Part 2 : DOM implementation >diff --git a/dom/activities/Activities.js b/dom/activities/Activities.js >+ chooseActivity: function(aRequest, aMessage) { >+ let selectedActivity; >+ >+ if (!Cc["@mozilla.org/activities/glue;1"]) { >+ debug("No @mozilla.org/activities/glue;1 component!"); >+ return; >+ } >+ >+ let glue = Cc["@mozilla.org/activities/glue;1"].createInstance(Ci.nsIActivityGlue); >+ >+ function launchActivity() { >+ let win = glue.getWindow(selectedActivity); >+ >+ let observer = function(aWindow, aTopic, aData) { >+ Services.obs.removeObserver(observer, "content-document-global-created"); >+ >+ let uri = aWindow.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation).currentURI; >+ if (uri.spec != selectedActivity.uri) >+ return; >+ >+ // doing that to let the page a chance to add its own event listeners before we >+ // fire our own event >+ aWindow.addEventListener("DOMContentLoaded", function act_domContentListener(e) { >+ aWindow.addEventListener("load", function act_loadListener(e) { >+ let event = aWindow.document.createEvent("MozActivityEvent"); >+ event.initActivityEvent("mozactivity", true, true, aRequest, selectedActivity); >+ aWindow.dispatchEvent(event); >+ debug("Event dispatched"); >+ aWindow.removeEventListener("load", act_loadListener); >+ }); >+ aWindow.removeEventListener("DOMContentLoaded", act_domContentListener); >+ }); >+ } >+ >+ // set up an observer to get notified when the real document is loaded. >+ // we then check that the URI of the doc matches what we expect. >+ Services.obs.addObserver(observer, "content-document-global-created", false); >+ } This code won't work if the application is already opened since 'DOMContentLoaded' and 'load' are already fired. Also I think you want to setTimeout(..., 0); the call to aWindow.dispatchEvent(event) to make sure it is not nested into the load event. This will also make sure the aWindow.removeEventListener code is called, otherwise it's unclear what happened if there is an error in the event handler. >+/** >+ * nsIDOMActivity object >+ */ >+function Activity(aAction, aType, aData) { >+ this._action = aAction; >+ this._type = aType; >+ this._data = aData; >+} >+ >+Activity.prototype = { >+ _action: null, >+ _type: null, >+ _data: null, >+ >+ get action() { >+ return this._action; >+ }, >+ >+ get type() { >+ return this._type; >+ }, >+ >+ get data() { >+ return this._data; >+ }, >+ Is there any reasons to use getters instead of exposing the properties directly? >+ >+/** >+ * nsIDOMActivity object >+ */ >+function InternalActivity(aAction, aType, aData, aURI, aTitle) { >+ this._action = aAction; >+ this._type = aType; >+ this._data = aData; >+ this._uri = aURI; >+ this._title = aTitle; >+} >+ >+InternalActivity.prototype = { >+ _action: null, >+ _type: null, >+ _data: null, >+ _uri: null, >+ _title: null, >+ >+ get action() { >+ return this._action; >+ }, >+ >+ get type() { >+ return this._type; >+ }, >+ >+ get data() { >+ return this._data; >+ }, >+ >+ get uri() { >+ return this._uri; >+ }, >+ >+ get title() { >+ return this._title; >+ }, >+ Same question as above. Also what is an internal activity compared to a regular activity? >+++ b/dom/activities/ActivitiesService.jsm I can't really comment on the database code but the e10s code seems good.

Lets put some pressure on bug 723206...

(In reply to Vivien Nicolas (:vingtetun) from comment #15) > What do you think of exposing a 'window' property on the mozApps API? > If the application is opened you get the window associated to it. > If it is not opened you can open it via mozApps.launch() and wait until it > is started to retrieve the window property. > > This will let this code retrieve the correct window by looping over the apps > and they registered activities (and this will let background service get a > reference to their main window to use postMessage). Are we in a privileged context? If not, we could have .launch() returning the window in DOMRequest.result and launch() on an already launched app would be a no-op but would still return the window.

(In reply to Vivien Nicolas (:vingtetun) from comment #15) > >diff --git a/b2g/components/ActivitiesGlue.js b/b2g/components/ActivitiesGlue.js > >+ > >+ chooseActivity: function(aWindow, aActivities) { > >+ this.req = Services.DOMRequest.createRequest(aWindow); > >+ this.activities = aActivities; > >+ Services.tm.currentThread.dispatch(this, Ci.nsIEventTarget.DISPATCH_NORMAL); > >+ return this.req; > >+ }, > >+ > > Does having it as a nsIRunnable is a way to make the Services.prompt.select > call block the current JS thread until it returns instead of blocking the > main thread? Ok, now I have read the DOM impl. Since startActivity is asynchronous I guess the answer to my question is 'no'. So why ActivityGlue is a nsIRunnable? > >+ getWindow: function(aActivity) { > >+ let browserWin = Services.wm.getMostRecentWindow("navigator:browser"); > >+ let content = browserWin.content; > >+ let id = "activity" + Math.random(); > >+ let event = content.document.createEvent("CustomEvent"); > >+ event.initCustomEvent("mozChromeEvent", true, true, > >+ { type: "activity-openwindow", uri: aActivity.uri, title: aActivity.title, id: id }); > >+ let window = null; > >+ > >+ // XXX event dispatching is synchronous so that should work. Color me surprised. > >+ // We should rather have a dedicated API. > >+ content.addEventListener("mozContentEvent", function act_getWindow(aEvent) { > >+ if (evt.detail.id != id) > >+ return; > >+ > >+ content.removeEventListener("mozContentEvent", act_getWindow); > >+ window = evt.detail.window; > >+ }); > >+ > >+ content.dispatchEvent(event); > >+ dump("XxXxX getWindow = " + window + "\n"); > >+ return window; > >+ }, > > What do you think of exposing a 'window' property on the mozApps API? > If the application is opened you get the window associated to it. > If it is not opened you can open it via mozApps.launch() and wait until it > is started to retrieve the window property. > > This will let this code retrieve the correct window by looping over the apps > and they registered activities (and this will let background service get a > reference to their main window to use postMessage). Ok. It seems that the backend code is more generic than what I thought and targets web sites that have registered an activity too. Sounds OK then.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #18) > (In reply to Vivien Nicolas (:vingtetun) from comment #15) > > What do you think of exposing a 'window' property on the mozApps API? > > If the application is opened you get the window associated to it. > > If it is not opened you can open it via mozApps.launch() and wait until it > > is started to retrieve the window property. > > > > This will let this code retrieve the correct window by looping over the apps > > and they registered activities (and this will let background service get a > > reference to their main window to use postMessage). > > Are we in a privileged context? If not, we could have .launch() returning > the window in DOMRequest.result and launch() on an already launched app > would be a no-op but would still return the window. My use case for the background service is not in a privileged context. I think it requires extra permissions to be able to use launch(), so I was thinkink of ading somthing similar to getSelf() but for returning the 'window' object.

Comment on attachment 614963 [details] [diff] [review] Part 1 : IDL >+interface nsIDOMMozActivityEvent : nsIDOMEvent >+{ >+ readonly attribute nsIDOMActivity activity; >+ >+ // data will go to request.result. >+ void postResult(in jsval data); >+ >+ // data will go to request.error. >+ void postError(in DOMString data); So, these two methods shouldn't be here. Perhaps they could be moved to nsIDOMOMRequest and the event could have .request >+[scriptable, uuid(1f17afd1-547b-40bf-9600-b356f64f83c4)] >+interface nsIInternalActivity : nsIDOMActivity { { should be in the next line

Comment on attachment 614966 [details] [diff] [review] Part 3 : Events >+++ b/dom/interfaces/core/nsIInlineEventHandlers.idl >@@ -102,6 +102,8 @@ interface nsIInlineEventHandlers : nsISu > [implicit_jscontext] attribute jsval onvolumechange; > [implicit_jscontext] attribute jsval onwaiting; > >+ [implicit_jscontext] attribute jsval onmozactivity; >+ I believe you want onmozactivity on nsIDOMWindow, not nsIInlineEventHandlers But I'll review the rest of the patch once the API is fixed.

I splitted the IDL in several files, and moved them under dom/activities/interfaces

Changes in the way we store activities and retrieve them from the database to allow more flexibility: A page can now register for image/* and be used for image/png, image/jpeg, etc. The way we notify providers also changed: if the page was not previously opened, window.activitySource will be an activity event, and if we're reusing an already opened page, we dispatch it an event.

Events are now working properly, and we can also set window.onmozactivity to catch events.

Updated to support the new glue API. Reuses a tab if one is already opened for the provider.

Updated to support the new glue API. Still needs some work on the gaia side to support reusing existing windows.(wip at https://github.com/andreasgal/gaia/pull/1241)

Comment on attachment 618449 [details] [diff] [review] Part 3 : Events >+NS_IMETHODIMP >+ActivityEvent::PostResult(const JS::Value& data) >+{ >+ nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); >+ NS_ASSERTION(rs, "No DOMRequest Service!"); >+ >+ nsresult rv = rs->FireSuccess(mRequest, data); >+ return rv; >+} >+ >+NS_IMETHODIMP >+ActivityEvent::PostError(const nsAString& data) >+{ >+ nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); >+ NS_ASSERTION(rs, "No DOMRequest Service!"); >+ >+ nsresult rv = rs->FireError(mRequest, data); >+ return rv; >+} So the API is still very strange > >+ DOM_CLASSINFO_MAP_BEGIN(MozActivityEvent, nsIDOMMozActivityEvent) >+ DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozActivityEvent) >+ DOM_CLASSINFO_EVENT_MAP_ENTRIES >+ DOM_CLASSINFO_MAP_END use 2 space indentation So implementation looks ok, but the API doesn't.

Comment on attachment 618447 [details] [diff] [review] Part 1 : IDL Review of attachment 618447 [details] [diff] [review]: ----------------------------------------------------------------- Seems like most interfaces in nsIDOMActivities.idl are internal, am I right? Maybe we could have two files to make this clear. Otherwise, this isn't implementing recent changes I proposed like |type| being in |data| instead of being an attribute. Did you do that on purpose? ::: dom/activities/Makefile.in @@ +6,5 @@ > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = \ > +$(srcdir) \ > +$(NULL) Alignment issues here and VPATH could be = @srcdir@ only. @@ +14,5 @@ > +PARALLEL_DIRS = interfaces > + > +ifdef ENABLE_TESTS > +# DIRS += tests > +endif Remove that, please. @@ +16,5 @@ > +ifdef ENABLE_TESTS > +# DIRS += tests > +endif > + > +include $(topsrcdir)/config/config.mk config.mk might not be needed actually. ::: dom/activities/interfaces/Makefile.in @@ +10,5 @@ > +include $(DEPTH)/config/autoconf.mk > + > +MODULE = dom > +XPIDL_MODULE = dom_activities > +GRE_MODULE = 1 You have MODULE, XPIDL_MODULE and GRE_MODULE. Why not only XPIDL_MODULE? Seems like it is enough in dom/sms/interfaces/Makefile.in. @@ +13,5 @@ > +XPIDL_MODULE = dom_activities > +GRE_MODULE = 1 > + > +XPIDLSRCS = nsIDOMActivities.idl \ > + nsIDOMActivity.idl \ There is an indentation issue here. ::: dom/activities/interfaces/nsIDOMActivities.idl @@ +20,5 @@ > + > + // data will go to request.error. > + void postError(in DOMString data); > + > + void initActivityEvent(in DOMString typeArg, So you are going with init...() for the moment? ::: dom/activities/interfaces/nsIDOMActivity.idl @@ +9,5 @@ > +{ > + // these fields can be null > + readonly attribute DOMString action; > + readonly attribute DOMString type; > + readonly attribute jsval data; Indentatiotn seems weird here... ::: dom/activities/interfaces/nsIDOMNavigatorActivities.idl @@ +14,5 @@ > + nsIDOMDOMRequest registerActivityHandler(in DOMString action, in DOMString type, in DOMString title); > + nsIDOMDOMRequest unregisterActivityHandler(in DOMString action, in DOMString type); > + > + // request.result will be an array of nsIDOMActivity objects > + nsIDOMDOMRequest getRegisteredActivityHandlers(); This doesn't look like what I proposed. Why did you chose that instead of isRegistered method? ::: toolkit/toolkit-makefiles.sh @@ +46,4 @@ > ipc/ipdl/Makefile > ipc/testshell/Makefile > dom/Makefile > + dom/interfaces/activities/Makefile I think you forgot to update that file.

Comment on attachment 618449 [details] [diff] [review] Part 3 : Events Review of attachment 618449 [details] [diff] [review]: ----------------------------------------------------------------- Could you put your C++ class in a namespace? (mozilla::dom or mozilla::dom::activity/activities) ::: dom/activities/src/ActivityEvent.cpp @@ +39,5 @@ > +ActivityEvent::InitActivityEvent(const nsAString& aType, > + bool aCanBubble, > + bool aCancelable, > + nsIDOMDOMRequest* aRequest, > + nsIDOMActivity* aActivity) Indentation issues. @@ +53,5 @@ > +NS_IMETHODIMP > +ActivityEvent::PostResult(const JS::Value& data) > +{ > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); > + NS_ASSERTION(rs, "No DOMRequest Service!"); Use MOZ_ASSERT() or do if (!rs) { return; } because here, we would crash... @@ +56,5 @@ > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); > + NS_ASSERTION(rs, "No DOMRequest Service!"); > + > + nsresult rv = rs->FireSuccess(mRequest, data); > + return rv; return rs->FireSuccess(mRequest, data); @@ +63,5 @@ > +NS_IMETHODIMP > +ActivityEvent::PostError(const nsAString& data) > +{ > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); > + NS_ASSERTION(rs, "No DOMRequest Service!"); Use MOZ_ASSERT() or do if (!rs) { return; } because here, we would crash... @@ +66,5 @@ > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); > + NS_ASSERTION(rs, "No DOMRequest Service!"); > + > + nsresult rv = rs->FireError(mRequest, data); > + return rv; return rs->FireError(mRequest, data); @@ +74,5 @@ > +ActivityEvent::InitFromCtor(const nsAString& aType, > + JSContext* aCx, jsval* aVal) > +{ > + mozilla::dom::MozActivityEventInit d; > + nsresult rv = d.Init(aCx, aVal); I think you could rename this something else that "d"... Ever 'e' would be a better name ;) ::: dom/activities/src/ActivityEvent.h @@ +11,5 @@ > + > +#include "nsIDOMDOMRequest.h" > + > +class ActivityEvent : public nsDOMEvent, > + public nsIDOMMozActivityEvent I would prefer the comma before "public nsIDOMMozActivityEvent". I prefer the style but also adding a new class in the inheritance list is easier. ::: dom/activities/src/Makefile.in @@ +5,5 @@ > +DEPTH = ../../.. > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = \ > + $(srcdir) \ VPATH = @srcdir@ should be enough for the moment. @@ +14,5 @@ > +LIBRARY_NAME = dom_activity_s > +LIBXUL_LIBRARY = 1 > +FORCE_STATIC_LIB = 1 > + > +DEFINES += -D_IMPL_NS_LAYOUT Why do you need that? @@ +16,5 @@ > +FORCE_STATIC_LIB = 1 > + > +DEFINES += -D_IMPL_NS_LAYOUT > + > +include $(topsrcdir)/dom/dom-config.mk And that?

d is dictionary ;)

Addressed review comments in comment 29, with the following: I split up the IDL more : the internal bits are in their own file now. isRegistered has been added, but I kept getRegisteredActivityHandlers. I think they both have valid use cases. > Otherwise, this isn't implementing recent changes I proposed like |type| being in |data| instead of being an attribute. Did you do that on purpose? Yes, I'm waiting on some webdev feedback before updating that part. > So you are going with init...() for the moment? I still plan to get rid of that. The event patch has code for the new way but I could not get it to work from chrome JS.

Updated to add the isRegisteredActivityHandler method

Addressing Mounir's comments on the previous patch. No other changes.

Comment on attachment 619789 [details] [diff] [review] Part 3 : Events >+++ b/content/events/src/nsEventDispatcher.cpp >@@ -920,7 +920,8 @@ nsEventDispatcher::CreateEvent(nsPresCon > NS_ADDREF(*aDOMEvent = static_cast<nsDOMEvent*>(new nsDOMStorageEvent())); > return NS_OK; > } >- >+ if (aEventType.LowerCaseEqualsLiteral("mozactivityevent")) >+ return NS_NewDOMActivityEvent(aDOMEvent, aPresContext, nsnull); Could you remove this. JS should use new MozActivityEvent() >+NS_IMETHODIMP >+ActivityEvent::PostResult(const JS::Value& data) >+{ >+ nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); >+ MOZ_ASSERT(rs, "No DOMRequest Service!"); >+ >+ return rs->FireSuccess(mRequest, data); >+} >+ >+NS_IMETHODIMP >+ActivityEvent::PostError(const nsAString& data) >+{ >+ nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1"); >+ MOZ_ASSERT(rs, "No DOMRequest Service!"); >+ >+ return rs->FireError(mRequest, data); >+} And I still think these methods don't belong to an event.

The previous patch was missing a couple of macros to allow the |new MozActivityEvent(request, activity)| syntax. Last issue to solve on this front is that the dictionaty entries for request and activity are always null, even though I'm sure I'm not passing null objects to the constructor : This is chrome js code doing: let event = new aWindow.MozActivityEvent(request, activity)

Comment on attachment 619786 [details] [diff] [review] Part 1 : IDL Review of attachment 619786 [details] [diff] [review]: ----------------------------------------------------------------- Vivien should review dom/activities/interfaces/nsIDOMActivitiesInternal.idl Ideally, you should move that idl to the patch doing the B2G implementation. ::: dom/activities/Makefile.in @@ +4,5 @@ > + > +DEPTH = ../.. > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = @srcdir@ nit: alignment @@ +10,5 @@ > +include $(DEPTH)/config/autoconf.mk > + > +PARALLEL_DIRS = interfaces > + > +include $(topsrcdir)/config/config.mk I don't think you need to include "config.mk" here. ::: dom/activities/interfaces/Makefile.in @@ +11,5 @@ > + > +XPIDL_MODULE = dom_activities > + > +XPIDLSRCS = nsIDOMActivityEvent.idl \ > + nsIDOMActivity.idl \ nit: usually I think we do: VARIABLE = \ FOO \ BAR \ $(NULL) ::: dom/activities/interfaces/nsIDOMActivity.idl @@ +8,5 @@ > +interface nsIDOMActivity : nsISupports > +{ > + // these fields can be null > + readonly attribute DOMString action; > + readonly attribute DOMString type; I'm not sure why you want this to stay. You said you want to ask some web devs opinions. Do you have some people in mind? FWIW, I talked with Etienne and Vivien and both of them agree that |type| could move into |data| without problems. ::: dom/activities/interfaces/nsIDOMNavigatorActivities.idl @@ +10,5 @@ > +[scriptable, uuid(e7cb7d2f-11d2-4783-a8b0-bddabb4a5c03)] > +interface nsIDOMActivityManager : nsISupports > +{ > + nsIDOMDOMRequest startActivity(in DOMString action, in DOMString type, in jsval data); > + nsIDOMDOMRequest registerActivityHandler(in DOMString action, in DOMString type, in DOMString title); This should take something like |in jsval options| because we might want to specify a few options. @@ +14,5 @@ > + nsIDOMDOMRequest registerActivityHandler(in DOMString action, in DOMString type, in DOMString title); > + nsIDOMDOMRequest unregisterActivityHandler(in DOMString action, in DOMString type); > + > + // request.result will be an array of nsIDOMActivity objects > + nsIDOMDOMRequest getRegisteredActivityHandlers(); What are the use cases for that? I would prefer this to be out of the API unless there is strong use cases but I can't see one. @@ +19,5 @@ > + > + // onsuccess fires if true, onerror if false > + nsIDOMDOMRequest isActivityHandlerRegistered(in DOMString action, in DOMString type); > + > + // will be null if the page is not loaded from a startActivity() call nit: // Will [...] call. ::: dom/interfaces/base/nsIDOMWindow.idl @@ +73,1 @@ > interface nsIDOMWindow : nsISupports I wonder why nsIDOMWindow doesn't inherit from nsIDOMEventTarget. Olli, do you happen to know? ::: toolkit/toolkit-makefiles.sh @@ +67,4 @@ > dom/interfaces/xbl/Makefile > dom/interfaces/xpath/Makefile > dom/interfaces/xul/Makefile > + dom/activities/Makefile You are missing dom/activities/interfaces/Makefile.in

Apparently something here causes an off-by-one error with events. I get a |MozScrolledAreaChanged| instead of |transitionend|, |animationstart| instead of |animationend| ... I'll attach a test file.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #37) > ::: dom/activities/interfaces/nsIDOMActivity.idl > @@ +8,5 @@ > > +interface nsIDOMActivity : nsISupports > > +{ > > + // these fields can be null > > + readonly attribute DOMString action; > > + readonly attribute DOMString type; > > I'm not sure why you want this to stay. You said you want to ask some web > devs opinions. Do you have some people in mind? > FWIW, I talked with Etienne and Vivien and both of them agree that |type| > could move into |data| without problems. Fine by me then. I was just waiting on them ;) > ::: dom/activities/interfaces/nsIDOMNavigatorActivities.idl > @@ +10,5 @@ > > +[scriptable, uuid(e7cb7d2f-11d2-4783-a8b0-bddabb4a5c03)] > > +interface nsIDOMActivityManager : nsISupports > > +{ > > + nsIDOMDOMRequest startActivity(in DOMString action, in DOMString type, in jsval data); > > + nsIDOMDOMRequest registerActivityHandler(in DOMString action, in DOMString type, in DOMString title); > > This should take something like |in jsval options| because we might want to > specify a few options. Yes, instead of |DOMString type|. How do you expected options to be matched? Like regexpes? > @@ +14,5 @@ > > + nsIDOMDOMRequest registerActivityHandler(in DOMString action, in DOMString type, in DOMString title); > > + nsIDOMDOMRequest unregisterActivityHandler(in DOMString action, in DOMString type); > > + > > + // request.result will be an array of nsIDOMActivity objects > > + nsIDOMDOMRequest getRegisteredActivityHandlers(); > > What are the use cases for that? I would prefer this to be out of the API > unless there is strong use cases but I can't see one. The use case is to list all the handlers registered. Even checking a list of known handlers is inconvenient with the async isActivityHandlerRegistered().

(In reply to Etienne Segonzac from comment #39) > Created attachment 620312 [details] > Test file for the event bug Thanks Etienne! I found the error.

Comment on attachment 618452 [details] [diff] [review] Part 5 : b2g support Etienne is working on moving most of this code in Gaia itself. I don't have any feedback because of that.

Comment on attachment 619788 [details] [diff] [review] Part 2 : DOM implementation >diff --git a/dom/activities/Activities.js b/dom/activities/Activities.js >+ >+function debug(aMsg) { >+ dump("XxXxX " + Date.now() + " " + aMsg + "\n"); >+} Add a guard to not dump thing when you're not in DEBUG mode and maybe replace XxXxXx by something else ;) >+ActivityManager.prototype = { >+ __proto__: DOMRequestIpcHelper.prototype, >+ >+ convertActivitiesArray: function(aActivities, aData, aInternal) { Sometimes you're giving names to your anonymous functions, sometimes not. >+ >+ chooseActivity: function(aRequest, aMessage) { >+ let selectedActivity; >+ >+ if (!Cc["@mozilla.org/activities/glue;1"]) { >+ debug("No @mozilla.org/activities/glue;1 component!"); >+ return; >+ } if (["@mozilla.org/activities/glue;1"]] in Cc) to not throw a warning in strict mode >+ >+ let glue = Cc["@mozilla.org/activities/glue;1"].createInstance(Ci.nsIActivityGlue); >+ >+ function launchActivity() { >+ let observer = function(aWindow, aTopic, aData) { >+ Services.obs.removeObserver(observer, "content-document-global-created"); >+ >+ let uri = aWindow.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation).currentURI; >+ if (uri.spec != selectedActivity.uri) >+ return; Sounds like you want to use nsIURI.equals instead. >+ >+ let event = aWindow.document.createEvent("MozActivityEvent"); >+ event.initActivityEvent("mozactivity", true, true, aRequest, selectedActivity); Is the event really cancellable? >+ let handler = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowActivity); >+ handler.activityEvent = event; >+ } Setting this belongs to the API that will be exposed to the DOM and I think Mounir has some suggestions about a window.navigator.activity object instead of setting this directly onto the window. (also the .idl claimed it is called activitySource instead of activityEvent). >+ >+ let frame = glue.getWindow(selectedActivity); >+ >+ if (frame.isNew) { >+ // set up an observer to get notified when the real document is loaded, not about:blank. >+ // we then check that the URI of the doc matches what we expect. >+ Services.obs.addObserver(observer, "content-document-global-created", false); >+ } else { >+ // just dispatch the activity event to the existing window >+ let event = frame.window.document.createEvent("MozActivityEvent"); >+ event.initActivityEvent("mozactivity", true, true, aRequest, selectedActivity); The event is probably not cancellable. >+ frame.window.dispatchEvent(event); >+ } >+ } Same comment as before about firing events directly onto the window versus a navigator.activity object. >+ >+ let activities = this.convertActivitiesArray(aMessage.activities, aMessage.data, true); >+ // if we have many activities, display an activity picker. >+ if (aMessage.activities.length > 1) { >+ let request = glue.chooseActivity(this._window, activities, activities.length); >+ request.onsuccess = function(e) { >+ selectedActivity = request.result; >+ launchActivity(); >+ }.bind(this); >+ } else { >+ selectedActivity = activities[0]; >+ launchActivity(); >+ } Currently the plan is to implement the 'set as default' feature in Gaia but I wonder if it should be part of the API? This would avoid calling chooseActivity if there is a default choice. Maybe it's only UI... I'm not sure. >+ receiveMessage: function actmgr_receiveMessage(aMessage) { >+ //debug("content message: " + aMessage.name + " - " + JSON.stringify(aMessage.json)); nit: leftover >+ let msg = aMessage.json; >+ if (msg.oid != this._id) >+ return nit: add a line break after a return it helps to see it in the mass of code >+ let req = this.getRequest(msg.requestID); >+ if (!req) >+ return; nit: same as above >+ startActivity: function actmgr_startActivity(aAction, aType, aData) { >+ let request = this.createRequest(); >+ let requestID = this.getRequestId(request); this does not belong to this bug, but the pattern: create a request, get the request id is used often, maybe it worth a method in DOMRequestHelper that return a [request, id] >+ >+ get activitySource() { >+ return this._window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowActivity).activityEvent; >+ }, This is the contradiction I was speaking about a few comments before. There is a mix of activity[Source|Event] >+ >+ uninit: function() { >+ }, nit: leftover >+ >+ // nsIDOMGlobalPropertyInitializer implementation >+ init: function(aWindow) { nit: as mentionned at the beginning, there is sometimes names for your anonymous functions, sometimes not. >+ this.initHelper(aWindow, ["Activities:Start:OK", "Activities:Start:KO", >+ "Activities:Register:OK", "Activities:Register:KO", >+ "Activities:Unregister:OK", "Activities:Unregister:KO", >+ "Activities:List:OK", "Activities:List:KO", >+ "Activities:IsRegistered:OK", "Activities:IsRegistered:KO"]); >+ >+ Services.obs.addObserver(this, "inner-window-destroyed", false); DOMRequestHelper.initHelper add the observer for you, you don't need to do that yourself. >+ let util = this._window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); >+ this._id = util.outerWindowID; DOMRequestHelper.initHelper define this._id too. Any reason for not using the id define by it? >+/** >+ * nsIDOMActivity object >+ */ >+function Activity(aAction, aType, aData) { >+ this._action = aAction; >+ this._type = aType; >+ this._data = aData; >+} >+ >+Activity.prototype = { >+ _action: null, >+ _type: null, >+ _data: null, >+ >+ get action() { >+ return this._action; >+ }, >+ >+ get type() { >+ return this._type; >+ }, >+ >+ get data() { >+ return this._data; >+ }, >+ Why do you use getters instead of returning action/type/data directly? >+/** >+ * nsIDOMActivity object >+ */ nit: nsIDOMInternalActivity >+function InternalActivity(aAction, aType, aData, aURI, aTitle) { >+ this._action = aAction; >+ this._type = aType; >+ this._data = aData; >+ this._uri = aURI; >+ this._title = aTitle; >+} >+ >+InternalActivity.prototype = { >+ _action: null, >+ _type: null, >+ _data: null, >+ _uri: null, >+ _title: null, >+ >+ get action() { >+ return this._action; >+ }, >+ >+ get type() { >+ return this._type; >+ }, >+ >+ get data() { >+ return this._data; >+ }, >+ >+ get uri() { >+ return this._uri; >+ }, >+ >+ get title() { >+ return this._title; >+ }, Same as before. Maybe there is a good reason for using getters but I would like to know it :) >diff --git a/dom/activities/ActivitiesService.jsm b/dom/activities/ActivitiesService.jsm >+ >+let idbGlobal = this; >+ >+let DEBUG = 0; >+/* static functions */ >+if (DEBUG) >+ debug = function (s) { dump("-*- Activities: " + s + "\n"); } >+else >+ debug = function (s) {} I don't realy like this pattern but anyway |debug| is undeclared. What about: let DEBUG = false; let debug = DEBUG ? function(s) { dump("-*- Activities: " + s + "\n"); } : function(s) {}; >+ActivitiesDb.prototype = { >+ __proto__: IndexedDBHelper.prototype, >+ >+ init: function init() { >+ let idbManager = Components.classes["@mozilla.org/dom/indexeddb/manager;1"].getService(Ci.nsIIndexedDatabaseManager); nit: Components.classes -> Cc >+ // unique ids >+ createId: function(aObject, aType) { nit: init has a name, this one has none. >+ // XXX : make a more robust key generator >+ return aObject.uri + "#" + aObject.action + "#" + aType; >+ }, There is a few methods where you're using bind(this) twice just to get access to this method. What about making it a global, remove the binds call and use the uuid generator. >+ // create a compound key for (action, type) tuples >+ createMatchKey: function(aObject) { >+ // simply use the action as a match key, since we want to >+ // have a finer match for the mime type (and possibly other fields) >+ return aObject.action; >+ }, Sorry I don't really understand what this function does. The comment above the prototype says it uses the tuples (action, type). The comment underneath says for finer matching it uses only action... >+ find: function(aObject, aSuccess, aError, aMatch) { I've never like this find method prototype. It force the caller to fill up 2 callbacks and aError is often an empty callback. What about doing something similar to DOMRequest (that you use without listening for onerror btw)? >+ this.newTxn("readonly", function (txn, store) { >+ debug("Looking for " + aObject.action + " - " + aObject.type); >+ let index = store.index("matchKey"); >+ let request = index.getAll(this.createMatchKey(aObject)); >+ request.onsuccess = function (aEvent) { >+ debug("Request successful. Record count: " + aEvent.target.result.length); >+ if (!txn.result) >+ txn.result = []; >+ for (let i in aEvent.target.result) { >+ let result = aEvent.target.result[i]; aEvent.target.result.forEach(function(result) { let result = results[i]; if (!aMatch(result)) return; txn.result.push(....); }); >+ if (aMatch(result)) { >+ txn.result.push({ >+ action: result.action, >+ type: result.type, >+ uri: result.uri, >+ title: result.title >+ }); >+ } >+ } >+ } >+ }.bind(this), aSuccess, aError); >+ }, >+ >+ list: function(aURI, aSuccess, aError) { >+ this.newTxn("readonly", function (txn, store) { >+ debug("Listing activities for " + aURI); >+ let index = store.index("uri"); >+ let request = index.getAll(aURI); >+ request.onsuccess = function (aEvent) { >+ debug("Request successful. Record count: " + aEvent.target.result.length); >+ if (!txn.result) >+ txn.result = []; >+ for (let i in aEvent.target.result) { >+ let result = aEvent.target.result[i]; >+ txn.result.push({ >+ action: result.action, >+ type: result.type, >+ uri: result.uri, >+ title: result.title >+ }); >+ } >+ } >+ }.bind(this), aSuccess, aError); >+ }, Same comment as the previous method. >+ >+ isRegistered: function(aObject, aSuccess, aError) { >+ this.newTxn("readonly", function (txn, store) { >+ debug("Checking activity " + aObject.action + " - " + aObject.type + " for " + aObject.uri); >+ let index = store.index("uri"); >+ let request = index.getAll(aObject.uri); >+ request.onsuccess = function (aEvent) { >+ debug("Request successful. Record count: " + aEvent.target.result.length); >+ if (!txn.result) >+ txn.result = false; >+ for (let i in aEvent.target.result) { >+ let result = aEvent.target.result[i]; >+ if (result.action == aObject.action && result.type == aObject.type) >+ txn.result = true; >+ } Array.some? >+ } >+ }.bind(this), aSuccess, aError); >+ } >+} idem. >+function MimeType(aType) { >+ this.isMime = false; >+ this.type = null; >+ this.subType = null; >+ this.charset = null; >+ >+ this.init(aType); >+} >+ >+MimeType.prototype = { >+ init: function(aType) { >+ let splited = aType.split("/"); >+ // if not a real mime type, just consider it's a string >+ if (splited.length < 2) { >+ this.type = aType; >+ return; >+ } Can a mimetype be image/png/foo? >+ // not a recognized type >+ // see http://www.iana.org/assignments/media-types/index.html >+ if (["application", "audio", "example", "image", "message", "model", >+ "multipart", "text", "video"].indexOf(splited[0]) == -1) { >+ this.type = aType; >+ return; >+ } >+ this.isMime = true; >+ this.type = splited[0]; >+ if (splited[1].indexOf(";")) { >+ let charset = splited[1].split(";"); >+ this.subType = charset[0]; >+ this.charset = charset[1]; >+ } else { >+ this.subType = splited[1]; >+ } >+ }, All this code make me thing a super magic regexp can do it. >+ >+ // returns true if this mime type is a superset of aOther >+ // eg. image/*.compare(image/png) == true >+ // XXX we don't do anything with the charset for now. >+ compare: function(aOther) { nit: compare -> equals >+ // they need to be both mime types or not. >+ if (this.isMime ? !aOther.isMime : aOther.isMime) { >+ return false; >+ } if (this.isMime !== aOther.isMime) return false; >+ >+ // not mime types? just compare the opaque strings >+ if (!this.isMime) { >+ return this.type == aOther.type; >+ } >+ >+ return ((this.type == "*" || this.type == aOther.type) && >+ (this.subType == "*" || this.subType == aOther.subType)); nit: no need for the extra parenthesis >+ >+ debug: function() { toString? >+ return "[isMime:" + this.isMime + ", " + >+ "type:" + this.type + ", " + >+ "subType:" + this.subType + ", " + >+ "charset:" + this.charset + "]"; >+ } >+} Also is it possible to reused some part of nsIDOMMimeType here instead of (probably) rewriting this code? >+ >+let Activities = { >+ >+ uninit: function() { >+ >+ }, nit: leftover >+ >+ startActivity: function(aActivity) { >+ let mime = new MimeType(aActivity.type); >+ this.db.find(aActivity, function(results) { >+ if (results.length == 0) { >+ ppmm.sendAsyncMessage("Activities:Start:KO", { oid: aActivity.oid, requestID: aActivity.requestID, error: "NO_PROVIDER" }); >+ return; >+ } else { >+ ppmm.sendAsyncMessage("Activities:Start:OK", { oid: aActivity.oid, requestID: aActivity.requestID, data: aActivity.data, activities: results }); >+ } >+ }, >+ function(e) { >+ ppmm.sendAsyncMessage("Activities:Start:KO", { oid: aActivity.oid, requestID: aActivity.requestID, error: "NO_PROVIDER" }); >+ }, >+ function(aResult) { >+ return (new MimeType(aResult.type)).compare(mime); >+ }); Too many callbacks for me. Something like: var request = this.db.find(aActivity, function match(result) { return mime.equals(new MimeType(result.type)); }); request.onsuccess = function(results) { ... } request.onerror = function(error) { ... } would make the code clearer. If you don't want to go that far, give at least names to your methods, so it will be easier to know who is who (onSuccess, onError, compare) >+ receiveMessage: function(aMessage) { >+ let mm = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager); >+ let msg = aMessage.json; >+ switch(aMessage.name) { >+ case "Activities:Start": >+ this.startActivity(msg); >+ break; >+ case "Activities:Register": >+ this.db.add(msg, function(aEvent) { >+ mm.sendAsyncMessage("Activities:Register:OK", { oid: msg.oid }); >+ }, >+ function(aEvent) { >+ mm.sendAsyncMessage("Activities:Register:KO", { oid: msg.oid, error: "REGISTER_ERROR" }); >+ }); >+ break; >+ case "Activities:Unregister": >+ this.db.remove(msg, function(aEvent) { >+ mm.sendAsyncMessage("Activities:Unregister:OK", { oid: msg.oid }); >+ }, >+ function(aEvent) { >+ mm.sendAsyncMessage("Activities:Unregister:KO", { oid: msg.oid, error: "UNREGISTER_ERROR" }); >+ }); >+ break; >+ case "Activities:List": >+ this.db.list(msg.uri, function(aResults) { >+ mm.sendAsyncMessage("Activities:List:OK", { oid: msg.oid, requestID: msg.requestID, activities: aResults }); >+ }, >+ function(aEvent) { >+ mm.sendAsyncMessage("Activities:List:KO", { oid: msg.oid, requestID: msg.requestID, error: "NO_ACTIVITIES" }); >+ }); >+ break; >+ case "Activities:IsRegistered": >+ this.db.isRegistered(msg, function(aResult) { >+ if (aResult) >+ mm.sendAsyncMessage("Activities:IsRegistered:OK", { oid: msg.oid, requestID: msg.requestID }); >+ else >+ mm.sendAsyncMessage("Activities:IsRegistered:KO", { oid: msg.oid, requestID: msg.requestID, error: "NO_SUCH_ACTIVITY" }); >+ }, >+ function(aEvent) { >+ >+ }); >+ break; >+ } Same comment as above. >diff --git a/dom/activities/Makefile.in b/dom/activities/Makefile.in >diff --git a/dom/base/Navigator.cpp b/dom/base/Navigator.cpp >--- a/dom/base/nsDOMClassInfo.cpp >+++ b/dom/base/nsGlobalWindow.h I have not reviewed this part.

I agree with most comments, just replying to give details on some. (In reply to Vivien Nicolas (:vingtetun) from comment #43) > >+ > >+ let event = aWindow.document.createEvent("MozActivityEvent"); > >+ event.initActivityEvent("mozactivity", true, true, aRequest, selectedActivity); > > Is the event really cancellable? No, but I'm moving away from initActivityEvent() to new MozActivityEvent() anyway. > >+ let handler = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowActivity); > >+ handler.activityEvent = event; > >+ } > > Setting this belongs to the API that will be exposed to the DOM and I think > Mounir has some suggestions about a window.navigator.activity object instead > of setting this directly onto the window. (also the .idl claimed it is > called activitySource instead of activityEvent). > >+ > >+ get activitySource() { > >+ return this._window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowActivity).activityEvent; > >+ }, > > This is the contradiction I was speaking about a few comments before. There > is a mix of activity[Source|Event] I understand you confusion. Here's how this works: nsGlobalWindow implements the new nsIDOMWindowActivity interface which is not exposed to web content, but provides a place to store the event that we later pass to the page as navigator.activitySource. I should rename activityEvent in nsIDOMWindowActivity to activitySource if that makes everything clearer. Also, if we make navigator.activitySource read/write, this may not be needed at all. > >+ let frame = glue.getWindow(selectedActivity); > >+ > >+ if (frame.isNew) { > >+ // set up an observer to get notified when the real document is loaded, not about:blank. > >+ // we then check that the URI of the doc matches what we expect. > >+ Services.obs.addObserver(observer, "content-document-global-created", false); > >+ } else { > >+ // just dispatch the activity event to the existing window > >+ let event = frame.window.document.createEvent("MozActivityEvent"); > >+ event.initActivityEvent("mozactivity", true, true, aRequest, selectedActivity); > > The event is probably not cancellable. > > >+ frame.window.dispatchEvent(event); > >+ } > >+ } > > Same comment as before about firing events directly onto the window versus a > navigator.activity object. In the case where we reuse an already opened window, how can the provider now that a new activity request is being made if we don't send an avent? You said that yourself in dev-webapi :) > > >+ > >+ let activities = this.convertActivitiesArray(aMessage.activities, aMessage.data, true); > >+ // if we have many activities, display an activity picker. > >+ if (aMessage.activities.length > 1) { > >+ let request = glue.chooseActivity(this._window, activities, activities.length); > >+ request.onsuccess = function(e) { > >+ selectedActivity = request.result; > >+ launchActivity(); > >+ }.bind(this); > >+ } else { > >+ selectedActivity = activities[0]; > >+ launchActivity(); > >+ } > > Currently the plan is to implement the 'set as default' feature in Gaia but > I wonder if it should be part of the API? This would avoid calling > chooseActivity if there is a default choice. Maybe it's only UI... I'm not > sure. Not sure either, but if we store the default in the platform, we also need to define when we clear the default choice. That may be difficult now that we'll have arbitrary filters and not just (action, type). > >+function MimeType(aType) { > >+ this.isMime = false; > >+ this.type = null; > >+ this.subType = null; > >+ this.charset = null; > >+ > >+ this.init(aType); > >+} > > All this code make me thing a super magic regexp can do it. Yes, especially since we are moving to more generic filters. > Too many callbacks for me. > Something like: > > If you don't want to go that far, give at least names to your methods, so it > will be easier to know who is who (onSuccess, onError, compare) This is kind of dictated by the indexedDB helper, and the match callback doesn't help for sure. Would passing an object with the 3 functions sounds better for you?

(In reply to Fabrice Desré [:fabrice] from comment #44) > > >+ frame.window.dispatchEvent(event); > > >+ } > > >+ } > > > > Same comment as before about firing events directly onto the window versus a > > navigator.activity object. > > In the case where we reuse an already opened window, how can the provider > now that a new activity request is being made if we don't send an avent? You > said that yourself in dev-webapi :) Sure! My comment is mostly with latest Mounir's suggestions in mind. > > > Too many callbacks for me. > > Something like: > > > > If you don't want to go that far, give at least names to your methods, so it > > will be easier to know who is who (onSuccess, onError, compare) > > This is kind of dictated by the indexedDB helper, and the match callback > doesn't help for sure. Would passing an object with the 3 functions sounds > better for you? Maybe. In the first time just giving names to the methods should help.

SecReview Scheduled: https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html?view=month&action=view&invId=110473-110472&pstat=AC&exInvId=110473-179818&useInstance=1&instStartTime=1338408000000&instDuration=3600000

Comment on attachment 620119 [details] [diff] [review] Part 3 : Events The API is still strange in this patch. Other than that, and missing tests, this looks ok.

Public annoucement : I'm starting to make changes to implement the last proposal posted here: http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/a71b7824323fa5bd#

What else needs to happen for this to land?

(In reply to Dietrich Ayala (:dietrich) from comment #49) > What else needs to happen for this to land? I assume this is blocked on bug 755245 (System messages).

Updated IDL according to https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.webapi/pxt4JDI_pb0

That's mostly DOM boiler plate, but I'd like to be sure I'm making that correctly. I had to modify DOMRequest since DOMActivity inherits from it but has no window available in the constructor (we get it by implementing nsIJSNativeInitializer).

Comment on attachment 634708 [details] [diff] [review] Part 1 : IDL v2 Review of attachment 634708 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer one IDL file per web-facing interfaces. I will not r+ that file because I have no way to understand why you are adding the stuff in nsIDOMActivitiesInternal. Feel free to re-attach a patch without that file. I will gladly review it. ::: dom/activities/interfaces/nsIDOMActivity.idl @@ +7,5 @@ > + > +[scriptable, uuid(f5bf5e9b-f53f-470f-b560-0d6f4c1c98ad)] > +interface nsIDOMActivityOptions : nsISupports > +{ > + // These fields can be null Actually, only "data" can be null. @@ +13,5 @@ > + readonly attribute jsval data; > +}; > + > +[scriptable, uuid(1601d370-08d9-47b7-8802-d4291533b843)] > +interface nsIDOMActivityHandlerDescription : nsISupports We need a ctor that takes a "name" for that object. @@ +36,5 @@ > + */ > +[scriptable, builtinclass, uuid(1f59cd9a-b8b5-4a98-bd2a-897b73584946)] > +interface nsIDOMActivity : nsIDOMDOMRequest > +{ > + nit: you can remove that empty line. ::: dom/activities/interfaces/nsIDOMNavigatorActivities.idl @@ +7,5 @@ > + > +[scriptable, uuid(e7cb7d2f-11d2-4783-a8b0-bddabb4a5c03)] > +interface nsIDOMActivityManager : nsISupports > +{ > + nsIDOMDOMRequest registerActivityHandler(in nsIDOMActivityHandlerDescription description, in DOMString title); Why "title"? I don't think this is part of the proposed API.

Comment on attachment 634711 [details] [diff] [review] Part 2 : implementation of the Activity object Review of attachment 634711 [details] [diff] [review]: ----------------------------------------------------------------- I think you have to add this macro in DOMActivity.h too: NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(DOMActivity, DOMRequest) ::: dom/activities/src/DOMActivity.cpp @@ +9,5 @@ > +namespace dom { > + > +DOMCI_DATA(Activity, DOMActivity) > + > +// I'm probably not using the correct macros here, or at least missing some I think you are missing: NS_IMPL_CYCLE_COLLECTION_CLASS(DOMActivity) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(DOMActivity, DOMRequest) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mOptions) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(DOMActivity, DOMRequest) NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mOptions) NS_IMPL_CYCLE_COLLECTION_UNLINK_END @@ +21,5 @@ > +NS_IMPL_ADDREF_INHERITED(DOMActivity, DOMRequest) > +NS_IMPL_RELEASE_INHERITED(DOMActivity, DOMRequest) > + > +DOMActivity::DOMActivity() : DOMRequest() > +{ As said in the header, no need for this. @@ +36,5 @@ > + nsCOMPtr<nsPIDOMWindow> ownerWindow = do_QueryInterface(aOwner); > + NS_ENSURE_STATE(ownerWindow); > + > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aOwner); > + Init(window); It's me or you are doing more or less twice the same thing here? Could be: nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aOwner); NS_ENSURE_TRUE(window, NS_ERROR_FAILURE); Init(window); return NS_OK; ::: dom/activities/src/DOMActivity.h @@ +26,5 @@ > + NS_DECL_NSIDOMACTIVITY > + NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::) > + NS_FORWARD_NSIDOMDOMREQUEST(DOMRequest::) > + > + DOMActivity(); No need for that, the default ctor will call DOMRequest ctor too. ::: dom/activities/src/Makefile.in @@ +24,5 @@ > + DOMActivity.h \ > + $(NULL) > + > +include $(topsrcdir)/config/config.mk > +include $(topsrcdir)/ipc/chromium/chromium-config.mk You likely don't need ipc for the moment. ::: dom/base/DOMRequest.cpp @@ +29,5 @@ > + : mResult(JSVAL_VOID) > + , mDone(false) > + , mRooted(false) > +{ > + nit: don't need that empty line. ::: layout/build/Makefile.in @@ +59,4 @@ > $(DEPTH)/content/xbl/src/$(LIB_PREFIX)gkconxbl_s.$(LIB_SUFFIX) \ > $(DEPTH)/content/xul/document/src/$(LIB_PREFIX)gkconxuldoc_s.$(LIB_SUFFIX) \ > $(DEPTH)/view/src/$(LIB_PREFIX)gkview_s.$(LIB_SUFFIX) \ > + $(DEPTH)/dom/activities/src/$(LIB_PREFIX)dom_activity_s.$(LIB_SUFFIX) \ nit: seems like there is an identation issue here.

I now have one interface per file with comments. The internal IDL will make a comeback in another patch :)

It's more a help? than a feedback? request : I added missing pieces in nsDOMClassInfo.cpp but building fails when linking libxul with this error: make[5]: Entering directory `/home/fabrice/dev/inbound/obj-b2g-desktop/toolkit/library' ../../dom/base/nsDOMClassInfo.o: In function `nsDOMClassInfo::Init()': /home/fabrice/dev/inbound/dom/base/nsDOMClassInfo.cpp:4546: undefined reference to `kDOMClassInfo_Activity_interfaces' /usr/bin/ld: ../../dom/base/nsDOMClassInfo.o: relocation R_X86_64_PC32 against undefined hidden symbol `kDOMClassInfo_Activity_interfaces' can not be used when making a shared object /usr/bin/ld: final link failed: Bad value collect2: ld returned 1 exit status make[5]: *** [libxul.so] Error 1 I checked similar objects but could not find what I'm missing.

Comment on attachment 635550 [details] [diff] [review] Part 2 : implementation of the Activity object WIP Review of attachment 635550 [details] [diff] [review]: ----------------------------------------------------------------- Nevermind, I figured out the linking issue.

Comment on attachment 635548 [details] [diff] [review] Part 1 : IDL v3 Review of attachment 635548 [details] [diff] [review]: ----------------------------------------------------------------- rs+sr=me with the following comments applied. ::: dom/activities/interfaces/nsIDOMActivityHandlerDescription.idl @@ +13,5 @@ > + attribute DOMString name; > + attribute DOMString href; > + attribute DOMString disposition; > + attribute boolean returnValue; > + attribute jsval filters; Add a comment explaining this can be null. ::: dom/activities/interfaces/nsIDOMActivityOptions.idl @@ +6,5 @@ > + > +[scriptable, uuid(f5bf5e9b-f53f-470f-b560-0d6f4c1c98ad)] > +interface nsIDOMActivityOptions : nsISupports > +{ > + // The |data| field can be null put that on top of |data| declaration. ::: dom/activities/interfaces/nsIDOMNavigatorActivities.idl @@ +10,5 @@ > +/** > + * This interface is implemented by the Navigator object. > + */ > +[scriptable, uuid(e7cb7d2f-11d2-4783-a8b0-bddabb4a5c03)] > +interface nsIDOMActivityManager : nsISupports Name this interface nsIDOMNavigatorActivities instead.

Comment on attachment 635548 [details] [diff] [review] Part 1 : IDL v3 Interfaces, methods and attributes need to be prefixed. Sorry for not noticing that earlier :(

mozify all the things!

Implementation of the MozActivity() constructor. It relays the call to a JS xpcom on its startActivity() method, that itself sends a message to the parent process, and listen to error and success messages to call handlers on the DOMRequest.

Comment on attachment 636650 [details] [diff] [review] Part 1 : IDL v4 Review of attachment 636650 [details] [diff] [review]: ----------------------------------------------------------------- rs+sr=me with the following changes. ::: dom/activities/interfaces/nsIDOMActivityRequestHandler.idl @@ +2,5 @@ > + * 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/. */ > + > +#include "domstubs.idl" > +#include "nsIDOMActivityOptions.idl" Don't #include. Do that instead: interface nsIDOMMozActivityOptions; this is the forward declaration in the IDL files. ::: dom/activities/interfaces/nsIDOMNavigatorActivities.idl @@ +2,5 @@ > + * 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/. */ > + > +#include "domstubs.idl" > +#include "nsIDOMActivityHandlerDescription.idl" Same here, you should be able to forward declare I believe.

Comment on attachment 636651 [details] [diff] [review] Part 2 : implementation of the Activity object Review of attachment 636651 [details] [diff] [review]: ----------------------------------------------------------------- Could you remove nsIDOMActivityProxy and ActivytProxy from this patch? They are not needed yet so it's hard to really review those. r+ with that and the following comments applied. ::: dom/activities/interfaces/nsIDOMActivityProxy.idl @@ +10,5 @@ > +/** > + * Implemented by @mozilla.org/activities/proxy;1 > + */ > +[scriptable, uuid(2241faf9-6219-4bc0-95f3-18651ac8a18b)] > +interface nsIDOMActivityProxy : nsISupports nsIActivityProxy. Don't prefix internal interfaces with "nsIDOM" but with "nsI". ::: dom/activities/src/Activities.manifest @@ +1,2 @@ > +component {ba9bd5cb-76a0-4ecf-a7b3-d2f7c43c5949} ActivityProxy.js > +contract @mozilla.org/activities/proxy;1 {ba9bd5cb-76a0-4ecf-a7b3-d2f7c43c5949} @mozilla.org/dom/activities/proxy;1 ::: dom/activities/src/ActivityProxy.js @@ +16,5 @@ > + .QueryInterface(Ci.nsISyncMessageSender); > +}); > + > +function debug(aMsg) { > + dump("-- ActivityProxy " + Date.now() + " : " + aMsg + "\n"); Comment that. ::: dom/activities/src/DOMActivity.cpp @@ +48,5 @@ > + Init(window); > + > + // We expect a single argument, which is a nsIDOMMozActivityOptions. > + NS_ENSURE_STATE(aArgc == 1); > + NS_ENSURE_STATE(!JSVAL_IS_PRIMITIVE(aArgv[0])); Never use NS_ENSURE_STATE. This is a good way to do stuff you do not expect. In that case you want: if (aArgc != 1 || !aArgv[0].isObject()) { return NS_ERROR_INVALID_ARG; } @@ +55,5 @@ > + nsContentUtils::XPConnect()->WrapJS(aContext, JSVAL_TO_OBJECT(aArgv[0]), > + NS_GET_IID(nsIDOMMozActivityOptions), > + getter_AddRefs(tmp)); > + nsCOMPtr<nsIDOMActivityOptions> options = do_QueryInterface(tmp); > + NS_ENSURE_STATE(options); Do that instead of the previous block: nsCOMPtr<nsIDOMActivityOptions> options = do_QueryInterface(nsContentUtils::XPConnect()->GetNativeOfWrapper(aContext, aArgv[0]->toObjectOrNull())); if (!options) { return NS_ERROR_INVALID_ARG; } ::: dom/activities/src/DOMActivity.h @@ +12,5 @@ > + > +#define NS_DOMACTIVITY_CID \ > + {0x1c5b0930, 0xc90c, 0x4e9c, {0xaf, 0x4e, 0xb0, 0xb7, 0xa6, 0x59, 0xb4, 0xed}} > + > +#define NS_DOMACTIVITY_CONTRACTID "@mozilla.org/activity;1" @mozilla.org/dom/activity;1 @@ +17,5 @@ > + > +namespace mozilla { > +namespace dom { > + > +class DOMActivity : public nsIDOMMozActivity You could name the class Activity. ::: dom/activities/src/Makefile.in @@ +16,5 @@ > + > +include $(topsrcdir)/dom/dom-config.mk > + > +CPPSRCS = \ > + DOMActivity.cpp \ nit: s/DOM// @@ +21,5 @@ > + $(NULL) > + > +EXPORTS_NAMESPACES = mozilla/dom > +EXPORTS_mozilla/dom = \ > + DOMActivity.h \ ditto ::: dom/base/DOMRequest.cpp @@ +23,5 @@ > + Init(aWindow); > +} > + > +// We need this constructor for DOMActivity that inherits from DOMRequest > +// but has now window available from the constructor. s/now/no/ @@ +32,5 @@ > +{ > +} > + > +void > +DOMRequest::Init(nsIDOMWindow* aWindow) I think we could use a mInitialized boolean that would be DEBUG only and would be check with a MOZ_ASSERT at the beginning of some/most(/all?) methods of DOMRequest. This could easily be a follow-up though.

Mounir I have added a bunch of #ifdef MOZ_SYS_MSG and add more changes from Fabrice to the Activity Object patch. Could you r? again? I have kept the ActivityProxy in this patch, and I would like gal to review it, this is pretty trivial since it basically forward messages.

This is the DOM Impl in JS of the nsIDOMNavigatorActivities methods exposed by the navigator object. It missed a correct way to affect the title/icon when using navigator.mozRegisterActivityHandler but I would like to fix that in a followup since it required some changes to dom/apps/ mostly.

Comment on attachment 641133 [details] [diff] [review] Part 7 - b2g/ part >diff --git a/b2g/components/ActivitiesGlue.js b/b2g/components/ActivitiesGlue.js >+function ActivitiesGlue() { Please document this. The functionality is slightly nontrivial. >+ActivitiesGlue.prototype = { >+ run: function() { >+ let browserWin = Services.wm.getMostRecentWindow("navigator:browser"); >+ let content = browserWin.shell.contentBrowser.contentWindow; I don't know what this magic is. If we have a <xul:browser> in our <xul:window>, in addition to the content <html:iframe>, will this continue to work? >+ let id = "activity-choice" + Math.random(); We can't use this implementation because collisions will lead to security bugs. Keeping a global hash of in-use IDs and looping here until we find an unused one wfm. The rest looks OK.

Comment on attachment 641129 [details] [diff] [review] Part 4 - DOM Impl in JS Review of attachment 641129 [details] [diff] [review]: ----------------------------------------------------------------- I would like to review that.

Comment on attachment 641130 [details] [diff] [review] Part 5 - dom/apps/ part Review of attachment 641130 [details] [diff] [review]: ----------------------------------------------------------------- And have a look at that.

Comment on attachment 641130 [details] [diff] [review] Part 5 - dom/apps/ part Looks like mounir wants to take this.

Comment on attachment 641126 [details] [diff] [review] Part 2 - implementation of the Activity object Review of attachment 641126 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following comments applied (especially, the change from WrapJS() to GetNativeOfWrapper()). Also, I do not believe you need to have all those #ifdef. I don't know why you added them. It would be as good to disable Web Activities where we don't want it enabled. If MOZ_SYS_MSG isn't set, the current code I reviewed will likely be compiling and "work" (as much as it can). If we have to depend on MOZ_SYS_MSG it is because something wouldn't compile without system message support or, maybe, if it wouldn't work properly (but we can always have distinct paths). I haven't removed gal's review in case of he wants to do it. However, I feel confortable r+ the entire patch. ::: dom/activities/src/Activity.cpp @@ +39,5 @@ > +Activity::Initialize(nsISupports* aOwner, > + JSContext* aContext, > + JSObject* aObject, > + PRUint32 aArgc, > + JS::Value* aArgv) nit: identation @@ +42,5 @@ > + PRUint32 aArgc, > + JS::Value* aArgv) > +{ > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aOwner); > + NS_ENSURE_STATE(window); I would prefer something more explicit: NS_ENSURE_TRUE(window, NS_ERROR_UNEXPECTED); // (could be NS_ERROR_FAILURE, I don't care) @@ +55,5 @@ > + nsCOMPtr<nsISupports> tmp; > + nsContentUtils::XPConnect()->WrapJS(aContext, JSVAL_TO_OBJECT(aArgv[0]), > + NS_GET_IID(nsIDOMMozActivityOptions), > + getter_AddRefs(tmp)); > + nsCOMPtr<nsIDOMMozActivityOptions> options = do_QueryInterface(tmp); You missed that comment to Fabrice's patch. You should do: nsCOMPtr<nsIDOMActivityOptions> options = do_QueryInterface(nsContentUtils::XPConnect()->GetNativeOfWrapper(aContext, aArgv[0]->toObjectOrNull())); ::: dom/activities/src/Activity.h @@ +19,5 @@ > +namespace dom { > + > +class Activity : public nsIDOMMozActivity > + , public nsIJSNativeInitializer // In order to get a window for the DOMRequest > + , public DOMRequest nit: identation

Comment on attachment 641128 [details] [diff] [review] Part 3 - Navigator Impl Review of attachment 641128 [details] [diff] [review]: ----------------------------------------------------------------- r=me if you remove those awful #ifdef MOZ_SYS_MSG and if you come with a good explanations for them. Also, make sure to wrap all those patches into one before pushing. You obviously haven't try to compile/use them separately. Make sure to change nsIDOMMozNavigatorActivities to nsIActivityManager in EnsureActivityManager(). ::: dom/base/Navigator.cpp @@ +1344,5 @@ > + > + nsresult rv; > + nsCOMPtr<nsIDOMMozNavigatorActivities> > + activityManager = do_CreateInstance("@mozilla.org/dom/activities/manager;1", > + &rv); nit: nsCOMPtr<nsIDOMMozNavigatorActivities> activityManager = do_CreateInstance("@mozilla.org/dom/activities/manager;1", &rv); But... nsIDOMMozNavigatorActivities?! shouldn't that be nsIDOMActivityManager from part 4? (btw, better to cut patches in a way that doesn't require opening all of them...) It should actually be nsIActivityManager... @@ +1348,5 @@ > + &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCOMPtr<nsIDOMGlobalPropertyInitializer> > + gpi = do_QueryInterface(activityManager); nit: ditto, move gpi on the line above. @@ +1362,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +Navigator::MozRegisterActivityHandler(nsIDOMMozActivityHandlerDescription* aDescription, nsIDOMDOMRequest** aRequest) nit: new line for nsIDOMDOMRequest** @@ +1387,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + return mActivityManager->MozIsActivityHandlerRegistered(aDescription, aResult); > +} > +#else I don't think you need that else given that the interface and the inheritance only happens when MOZ_SYS_MSG is enabled.

Comment on attachment 641129 [details] [diff] [review] Part 4 - DOM Impl in JS Review of attachment 641129 [details] [diff] [review]: ----------------------------------------------------------------- r- because a lot of stuff isn't really understandable and because ActivityManager interfaces are obviously broken. Maybe you should attach a patch with everything inside. It might be easier to review. ::: dom/activities/src/Activities.js @@ +34,5 @@ > + return > + > + let req = this.getRequest(msg.requestID); > + if (!req) > + return; Can that happen? Shouldn't we print an error/warning message? @@ +39,5 @@ > + > + switch (aMessage.name) { > + case "Activities:Register:OK": > + Services.DOMRequest.fireSuccess(req, true); > + break; return; @@ +42,5 @@ > + Services.DOMRequest.fireSuccess(req, true); > + break; > + case "Activities:Register:KO": > + Services.DOMRequest.fireError(req, msg.error); > + break; return; @@ +50,5 @@ > + getManifestURL: function actmgr_getManifestURL() { > + let win = this._window; > + let utils = win.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + let app = utils.getApp(); No need for those temporary variables, this would work: let app = this._window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).getApp(); return app ? app.manifestURL : ""; @@ +59,5 @@ > + let request = this.createRequest(); > + let requestID = this.getRequestId(request); > + > + let json = { > + "oid": this._id, Why oid? @@ +104,5 @@ > + classID: Components.ID("{ce10d7c0-ead6-4f7d-a484-389e2d4c6f09}"), > + > + QueryInterface: XPCOMUtils.generateQI([ > + Ci.nsIDOMActivityManager, Ci.nsIDOMGlobalPropertyInitializer, > + Ci.nsIDOMMozNavigatorActivities I've never seen nsIDOMActivityManager (does that exist?!) and I don't understand why that should be using nsIDOMMozNavigatorActivities. @@ +112,5 @@ > + classID: Components.ID("{ce10d7c0-ead6-4f7d-a484-389e2d4c6f09}"), > + contractID: "@mozilla.org/dom/activities/manager;1", > + interfaces: [Ci.nsIDOMActivityManager], > + flags: Ci.nsIClassInfo.DOM_OBJECT, > + classDescription: "Activity Manager" Why is that part of DOM? ::: dom/activities/src/Activities.manifest @@ +16,5 @@ > +component {ce10d7c0-ead6-4f7d-a484-389e2d4c6f09} Activities.js > +contract @mozilla.org/dom/activities/manager;1 {ce10d7c0-ead6-4f7d-a484-389e2d4c6f09} > + > +component {8fbc96fb-97de-4f34-af4d-bee9ebc849f8} Activities.js > +contract @mozilla.org/dom/activities/activity-internal;1 {8fbc96fb-97de-4f34-af4d-bee9ebc849f8} Where is activity-internal defined? In a following patch I image..? ::: dom/activities/src/ActivitiesService.jsm @@ +60,5 @@ > + * id: String > + * name: String > + * data: jsval > + * manifest: String > + * title: String This is obviously outdated. @@ +239,5 @@ > + error: "USER_ABORT" }); > + } else { > + let target = results[aChoice]; > + let sysmm = Cc["@mozilla.org/system-message-internal;1"] > + .getService(Ci.nsISystemMessagesInternal); This is the only moment where we need to know if MOZ_SYS_MSG is enabled. Eventually, sysmm would be null if it's not enabled. Could you check that? and handle that case. @@ +242,5 @@ > + let sysmm = Cc["@mozilla.org/system-message-internal;1"] > + .getService(Ci.nsISystemMessagesInternal); > + let disposition = target.disposition ? target.dispotion : "window"; > + sysmm.sendMessage("activity", { id: aMsg.id, payload: aMsg.options }, > + Services.io.newURI(target.manifest, null, null), I don't understand why so many things are being passed but I maybe that will be clarified with the following patches? @@ +258,5 @@ > + for (let prop in aResult.data) { > + // We only compare when the key is present in the consumer. > + if (prop in aMsg.options.data) { > + // XXX TODO: arrays of strings. > + isMatching = aMsg.options.data[prop] == aResult.data[prop]; Open a bug for that. For regexp too. @@ +316,5 @@ > + // is an indexedDB database and there is no synchronous way > + // of requesting a value. > + // In order to offer a synchronous call the choice is between > + // keeping a in-memory table of values or lock this function > + // until the value is returned. I have choose the later. I would clearly prefer the former. That shouldn't cost a lot because it's unlikely pages will register too many activities. The only pitfall would be when loading the page: we need to get that information ASAP. Blocking the application seems quite bad. In that case, the API should be async. ::: dom/activities/src/ActivityWrapper.js @@ +50,5 @@ > + > +function ActivityRequestHandler() { > + debug("ActivityRequestHandler"); > + this.wrappedJSObject = this; > + this._source = Cc["@mozilla.org/dom/activities/options;1"].createInstance(Ci.nsIDOMMozActivityOptions); I guess this is set by ActivityWrapper. However, this is never used so it's hard to say if that's done correctly. ::: dom/activities/src/Makefile.in @@ +26,5 @@ > $(NULL) > > EXTRA_COMPONENTS = \ > ActivityProxy.js \ > + Activities.js \ Name this ActivityManager.js please. @@ +27,5 @@ > > EXTRA_COMPONENTS = \ > ActivityProxy.js \ > + Activities.js \ > + ActivityWrapper.js \ I would prefer one js file per components implementation. There is no reason why this file implementing 3 components has the name of only one of them. For whom the intent isn't clear, based on that patch.

Comment on attachment 641130 [details] [diff] [review] Part 5 - dom/apps/ part Review of attachment 641130 [details] [diff] [review]: ----------------------------------------------------------------- And have a look at that. ::: dom/apps/src/Webapps.jsm @@ +93,5 @@ > } > }, > > + _registerActivities: function(aManifest, aApp) { > + if (aManifest.activities) { nit: if (!aManifest.activities) { return; } @@ +95,5 @@ > > + _registerActivities: function(aManifest, aApp) { > + if (aManifest.activities) { > + let manifest = new DOMApplicationManifest(aManifest, aApp.origin); > + let launchPath = Services.io.newURI(manifest.fullLaunchPath(), null, null); You should use "href". @@ +100,5 @@ > + let manifestURL = Services.io.newURI(aApp.manifestURL, null, null); > + for (let activity in aManifest.activities) { > + let handler = { > + description: { name: activity, > + data: aManifest.activities[activity].filters }, description should match "nsIDOMActivityHandlerDescription.idl", shouldn't it? I think you are confusing with nsIDOMActivityOptions here. @@ +115,5 @@ > _registerSystemMessagesForId: function(aId) { > let app = this.webapps[aId]; > this._readManifests([{ id: aId }], (function registerManifest(aResult) { > this._registerSystemMessages(aResult[0].manifest, app); > + this._registerActivities(aResult[0].manifest, app); Please, plug this into init() instead, otherwise rename the method.

Comment on attachment 641132 [details] [diff] [review] Part 6 - dom/messages/ part Review of attachment 641132 [details] [diff] [review]: ----------------------------------------------------------------- Ideally, we should make system message not aware of web activities specificities. However, I'm not sure how we could do that. Anyhow, for the moment, I will just cancel the review and re-review a patch with everything inside so I can get the big picture. I indeed usually prefer small patches to review but this is useful when the patches are incremental which means you can understand the logical path that has been used to build the patches. Here, it's a big patch that has been cut in a few pieces. It's unfortunately harder to understand each pieces alone than the all thing.

(In reply to Mounir Lamouri (:mounir) from comment #77) > ::: dom/apps/src/Webapps.jsm > @@ +115,5 @@ > > _registerSystemMessagesForId: function(aId) { > > let app = this.webapps[aId]; > > this._readManifests([{ id: aId }], (function registerManifest(aResult) { > > this._registerSystemMessages(aResult[0].manifest, app); > > + this._registerActivities(aResult[0].manifest, app); > > Please, plug this into init() instead, otherwise rename the method. _registerSystemMessagesForId is called during init() already. Do you want me to change something?

(In reply to Mounir Lamouri (:mounir) from comment #76) > @@ +39,5 @@ > > + > > + switch (aMessage.name) { > > + case "Activities:Register:OK": > > + Services.DOMRequest.fireSuccess(req, true); > > + break; > > return; > > @@ +42,5 @@ > > + Services.DOMRequest.fireSuccess(req, true); > > + break; > > + case "Activities:Register:KO": > > + Services.DOMRequest.fireError(req, msg.error); > > + break; > > return; > I have kept 'break' since this is done in a lot of files and this is expected. But if you really want I can change it. > @@ +59,5 @@ > > + let request = this.createRequest(); > > + let requestID = this.getRequestId(request); > > + > > + let json = { > > + "oid": this._id, > > Why oid? > object id I guess. > > @@ +242,5 @@ > > + let sysmm = Cc["@mozilla.org/system-message-internal;1"] > > + .getService(Ci.nsISystemMessagesInternal); > > + let disposition = target.disposition ? target.dispotion : "window"; > > + sysmm.sendMessage("activity", { id: aMsg.id, payload: aMsg.options }, > > + Services.io.newURI(target.manifest, null, null), > > I don't understand why so many things are being passed but I maybe that will > be clarified with the following patches? I have removed 'disposition' support from those patches to not block on that. It could be add back in a followup. > @@ +316,5 @@ > > + // is an indexedDB database and there is no synchronous way > > + // of requesting a value. > > + // In order to offer a synchronous call the choice is between > > + // keeping a in-memory table of values or lock this function > > + // until the value is returned. I have choose the later. > > I would clearly prefer the former. That shouldn't cost a lot because it's > unlikely pages will register too many activities. The only pitfall would be > when loading the page: we need to get that information ASAP. > Blocking the application seems quite bad. In that case, the API should be > async. > Do you want navigator.mozIsActivityHandlerRegistered to return a DOMRequest instead of a bool?

(In reply to Mounir Lamouri (:mounir) from comment #74) > r=me with the following comments applied (especially, the change from > WrapJS() to GetNativeOfWrapper()). > > > @@ +55,5 @@ > > + nsCOMPtr<nsISupports> tmp; > > + nsContentUtils::XPConnect()->WrapJS(aContext, JSVAL_TO_OBJECT(aArgv[0]), > > + NS_GET_IID(nsIDOMMozActivityOptions), > > + getter_AddRefs(tmp)); > > + nsCOMPtr<nsIDOMMozActivityOptions> options = do_QueryInterface(tmp); > > You missed that comment to Fabrice's patch. You should do: > nsCOMPtr<nsIDOMActivityOptions> options = > > do_QueryInterface(nsContentUtils::XPConnect()->GetNativeOfWrapper(aContext, > aArgv[0]->toObjectOrNull())); > It fails with a NS_ERROR_ILLEGAL_VALUE :/

Comment on attachment 641684 [details] [diff] [review] Part 5 - B2G specific part >+ActivitiesDialog.prototype = { >+ let browser = Services.wm.getMostRecentWindow("navigator:browser"); >+ let content = browser.getContentWindow(); >+ I don't understand how this works and you didn't answer my question or document this. Might be better to have someone who knows what they're doing review this part. I'm unsure this is correct. >+ chooseActivity: function ap_chooseActivity(aActivities, aCallback) { >+ this.callback = aCallback; >+ this.activities = aActivities; >+ Services.tm.currentThread.dispatch(this, Ci.nsIEventTarget.DISPATCH_NORMAL); Oh ... I misread this last time, I thought this class was instantiated per request, sorry. In that case, I don't believe this implementation is correct; what happens if chooseAactivity(A1, C1) this.callback = C1 this.activities = A1 [dispatch task 1] [event queue is busy] chooseAactivity(A2, C2) this.callback = C2 this.activities = A2 [dispatch task 2] dequeue task 1 run() // with A2, C2 Then we would end up dropping A1/C1 on the floor. If something at a higher level prevents that, document it. Otherwise need a closure here.

Mounir, will you have time to r? this this week?

Comment on attachment 641949 [details] [diff] [review] Part 2 - implementation of the Activity object Review of attachment 641949 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but please have a second look for GetNativeOfWrapper(). ::: dom/activities/interfaces/nsIActivityProxy.idl @@ +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/. */ > + > +#include "domstubs.idl" nit: #include "nsISupports.idl" should be enough. ::: dom/activities/src/Activity.cpp @@ +54,5 @@ > + > + nsCOMPtr<nsISupports> tmp; > + nsContentUtils::XPConnect()->WrapJS(aContext, JSVAL_TO_OBJECT(aArgv[0]), > + NS_GET_IID(nsIDOMMozActivityOptions), > + getter_AddRefs(tmp)); AFAICT, GetNativeOfWrapper() should work here. Can you copy-paste the code you used? Anyway, you should use aArgv[0].toObjectOrNull() instead of JSVAL_TO_OBJECT(aArgv[0]). ::: dom/activities/src/ActivityProxy.js @@ +75,5 @@ > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIActivityProxy]) > +} > + > +const NSGetFactory = XPCOMUtils.generateNSGetFactory([ActivityProxy]); > + nit: no need for that empty line

Comment on attachment 641950 [details] [diff] [review] Part 3 - Navigator Impl Review of attachment 641950 [details] [diff] [review]: ----------------------------------------------------------------- r=me, assuming the nsIActivityManager stuff are correctly declared and defined in part 4.

Comment on attachment 641687 [details] [diff] [review] Part 4 - DOM Impl in JS Review of attachment 641687 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/activities/src/ActivitiesService.jsm @@ +95,5 @@ > + // add the filter keys=value tuples, sorting keys > + if (aObject.data) { > + let filters = Object.keys(aObject.data).sort(); > + filters.forEach(function(aProp) { > + let data = converter.convertToByteArray(aProp + "=" + aObject.data[aProp], {}); What happens if the key contains an '=' character? It seems like that can cause collisions where there shouldn't be. Not sure if is a security problem or not though. Would it help to add the key and value to the hash separately? ::: dom/activities/src/ActivityManager.js @@ +47,5 @@ > + getManifestURL: function actmgr_getManifestURL() { > + let app = this._window.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils) > + .getApp(); > + return app ? app.manifestURL : ""; What happens if we're not inside an app? Don't you need to grab the page URL somewhere here then? Does this patch even intend to implement WebActivities for web pages? @@ +58,5 @@ > + let json = { > + "oid": this._id, > + "manifest": this.getManifestURL(), > + "name": aHandlerDescription.name, > + "description": aHandlerDescription, You need to sanitize the aHandlerDescription.filters property here or in the service since XPConnect won't do it for you. I can't think of any way to directly exploit this, but it's scary that totally arbitrary data is passed to the parent process and then saved in IndexedDB. The URL passed in aHandlerDescription.href needs to be resolved against the current page base-URL and then checked for same-origin-ness with the current page. Same in unregister and isRegistered

Comment on attachment 641687 [details] [diff] [review] Part 4 - DOM Impl in JS Review of attachment 641687 [details] [diff] [review]: ----------------------------------------------------------------- nit: there are very often useless empty lines at the end of files. Shouldn't you unregister activities on uninstall? |returnValue| from ActivityHandlerDescription is never used nor passed. How is the UI going to handle the situation where the user leaves an app during an activity? The UI will have to make sure to follow the security decisions: https://wiki.mozilla.org/Security/Reviews/B2GWebActivities Also, given how late this is coming and the issues we have, Jonas and I think that we should disable the navigator API. That will reduce the possible usage of the API to installed apps (which are going to have activities registered at install time). It will reduce security treats and it will prevent having to take a bad decision regarding isRegisteredActivity(). Given the number of requested changes, I would like to see a new patch. If you attach it before I wake up, I will review it in the morning. ::: dom/activities/interfaces/nsIActivityManager.idl @@ +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/. */ > + > +#include "domstubs.idl" nit: #include "nsISupports.idl" should be enough ::: dom/activities/interfaces/nsIActivityUIGlue.idl @@ +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/. */ > + > +#include "domstubs.idl" nit: #include "nsISupports.idl" @@ +17,5 @@ > +interface nsIActivityUIGlue : nsISupports > +{ > + /** > + * @param activities A json blob which is an array of { "name":"...", "title":"...", "icon":"..." }. > + Where |name| is the activity name (eg. "share", "pick"). Given that |name| is going to be the same for each activity in |activities|, I would prefer this to be another parameter. @@ +20,5 @@ > + * @param activities A json blob which is an array of { "name":"...", "title":"...", "icon":"..." }. > + Where |name| is the activity name (eg. "share", "pick"). > + * @param onresult The callback to send the index of the choosen activity. Send -1 if no choice is made. > + */ > + void chooseActivity(in jsval activities, in nsIActivityUIGlueCallback onresult); void chooseActivity(in DOMString name, in jsval activities, in nsIActivityUIGlueCallback onresult); ::: dom/activities/src/ActivitiesService.jsm @@ +7,5 @@ > +const Cu = Components.utils; > +const Cc = Components.classes; > +const Ci = Components.interfaces; > + > +const EXPORTED_SYMBOLS = ["Activities"]; This is not used. @@ +16,5 @@ > +Cu.import("resource://gre/modules/IndexedDBHelper.jsm"); > + > +XPCOMUtils.defineLazyGetter(this, "NetUtil", function() { > + Cu.import("resource://gre/modules/NetUtil.jsm"); > + return NetUtil; You don't need that lazy getter. @@ +45,5 @@ > + __proto__: IndexedDBHelper.prototype, > + > + init: function actdb_init() { > + let idbManager = Cc["@mozilla.org/dom/indexeddb/manager;1"] > + .getService(Ci.nsIIndexedDatabaseManager); nit: indentation @@ +126,5 @@ > + let object = { > + manifest: aObject.manifest, > + name: aObject.name, > + title: aObject.title || "", > + icon: aObject.icon || "", You probably don't need |title| and |icon| when you remove, do you? @@ +275,5 @@ > + debug("Error in startActivity: " + aError + "\n"); > + }; > + > + let matchFunc = function matchFunc(aResult) { > + let isMatching = true; you can remove that @@ +278,5 @@ > + let matchFunc = function matchFunc(aResult) { > + let isMatching = true; > + for (let prop in aResult.data) { > + // We only compare when the key is present in the consumer. > + if (prop in aMsg.options.data) { I do not agree. If there is a filter on something but the activity doesn't have the information, the filtering should fail. @@ +280,5 @@ > + for (let prop in aResult.data) { > + // We only compare when the key is present in the consumer. > + if (prop in aMsg.options.data) { > + // Bug 773383: arrays of strings / regexp. > + isMatching = aMsg.options.data[prop] == aResult.data[prop]; if (aMsg.options.data[prop] != aResult.data[prop]) { return false; } @@ +283,5 @@ > + // Bug 773383: arrays of strings / regexp. > + isMatching = aMsg.options.data[prop] == aResult.data[prop]; > + } > + if (!isMatching) > + break; remove that @@ +285,5 @@ > + } > + if (!isMatching) > + break; > + } > + return isMatching; return true; @@ +347,5 @@ > + } > + ); > + > + while (!closingWatchDog && waitForResult) { > + Services.tm.currentThread.processNextEvent(true); That is very unfortunate... ::: dom/activities/src/ActivityManager.js @@ +29,5 @@ > + __proto__: DOMRequestIpcHelper.prototype, > + > + receiveMessage: function actmgr_receiveMessage(aMessage) { > + let msg = aMessage.json; > + if (msg.oid != this._id) You are never initializing |this._id|, are you? @@ +92,5 @@ > + this.initHelper(aWindow, [ > + "Activities:Register:OK", "Activities:Register:KO" > + ]); > + > + this._uri = aWindow.document.nodePrincipal.URI.spec; You should get the the documentURI instead of the principal URI because the system principal's URI is null. But... it seems that you actually don't use |_uri| so you can also just remove it ;) @@ +102,5 @@ > + QueryInterface: XPCOMUtils.generateQI([ > + Ci.nsIActivityManager, Ci.nsIDOMGlobalPropertyInitializer > + ]), > + > + classInfo: XPCOMUtils.generateCI({ It's not a DOM object. I believe you don't need that, right? ::: dom/activities/src/ActivityOptions.js @@ +13,5 @@ > + //dump("-- ActivityOptions.js " + Date.now() + " : " + aMsg + "\n"); > +} > + > +/** > + * nsIDOMMozActivityOptions implementation. Could you explain that data from that object will be filed by ActivityHandler. Detailed explanation will be welcome ;) ::: dom/activities/src/ActivityRequestHandler.js @@ +20,5 @@ > + //dump("-- ActivityRequestHandler.js " + Date.now() + " : " + aMsg + "\n"); > +} > + > +/** > + * nsIDOMMozActivityRequestHandler implementation. Explain how this._source will be filed. @@ +67,5 @@ > + }) > +} > + > +const NSGetFactory = XPCOMUtils.generateNSGetFactory([ActivityRequestHandler]); > + nit: empty blank line ::: dom/activities/src/ActivityWrapper.js @@ +14,5 @@ > +XPCOMUtils.defineLazyGetter(this, "cpmm", function() { > + return Cc["@mozilla.org/childprocessmessagemanager;1"] > + .getService(Ci.nsIFrameMessageManager) > + .QueryInterface(Ci.nsISyncMessageSender); > +}); You don't need that I believe. ::: dom/apps/src/Webapps.jsm @@ +103,5 @@ > + if (!aManifest.activities) { > + return; > + } > + > + let manifest = new DOMApplicationManifest(aManifest, aApp.origin); That is *so* confusing... Maybe a quick comment explaining that DOMApplicationManifest() returns a manifest object with localized fields and some helpers would help understand why we have |aManifest| and |manifest| in the same method. @@ +113,5 @@ > + "title": manifest.name, > + "icon": manifest.iconURLForSize(128), > + "description": description > + } > + cpmm.sendAsyncMessage("Activities:Register", json); Aren't we the parent process here? Is cpmm.sendAsyncMessage() sending the message to the same process if there is no parent? I'm not very familiar to cpmm/ppmm but I would like to understand how that happens to work. @@ +116,5 @@ > + } > + cpmm.sendAsyncMessage("Activities:Register", json); > + > + let launchURL = manifest.fullLaunchPath(description.href); > + let launchPath = Services.io.newURI(launchURL, null, null); nit: |launchURL| is useless here. @@ +128,5 @@ > let app = this.webapps[aId]; > this._readManifests([{ id: aId }], (function registerManifest(aResult) { > + let manifest = aResult[0].manifest; > + this._registerSystemMessages(manifest, app); > + this._registerActivities(manifest, app); I'm not a big fan of adding _registerActivities() in _registerSystemMessagesForId() even more because _registerActivities() do more than registering the system message. Could you rename _registerSystemMessagesForId() to something like _processManifestForId()? or whatever that doesn't make that looks like a dirty hack.

The UI will make ActivityGlue fires a USER_ABORT if the application is closed without returning any values. This is not done in Gaia yet but it will be it's responsibility. @@ +102,5 @@ > + QueryInterface: XPCOMUtils.generateQI([ > + Ci.nsIActivityManager, Ci.nsIDOMGlobalPropertyInitializer > + ]), > + > + classInfo: XPCOMUtils.generateCI({ > It's not a DOM object. I believe you don't need that, right? It still an xpcom component :) ::: dom/apps/src/Webapps.jsm @@ +113,5 @@ > + "title": manifest.name, > + "icon": manifest.iconURLForSize(128), > + "description": description > + } > + cpmm.sendAsyncMessage("Activities:Register", json); > Aren't we the parent process here? Is cpmm.sendAsyncMessage() sending the message to the same process if there is no parent? I'm not very familiar to cpmm/ppmm but I would like to understand how that happens to work. We are. But ppmm can't send message to itself. We need to use cppm to speak to the parent messages loop. @@ +103,5 @@ > + if (!aManifest.activities) { > + return; > + } > + > + let manifest = new DOMApplicationManifest(aManifest, aApp.origin); > That is *so* confusing... Maybe a quick comment explaining that DOMApplicationManifest() returns a manifest object with localized fields and some helpers would help understand why we have |aManifest| and |manifest| in the same method. I agree :/ But this same line of code is used in a few different places. If it needs to be fixed I believe it can be done in a followup and DOMApplicationManifest could be renamed because I don't think this name makes sense. For now all the navigator.* interfaces returns NS_ERROR_NOT_IMPLEMENTED. I can remove them completely if you want but since some part of this code have already been r+'ed I still think it makes sense to land them. But I have not strong opinion on that so if you insist I can remove it completely.

(In reply to Jonas Sicking (:sicking) from comment #90) > Comment on attachment 641687 [details] [diff] [review] > Part 4 - DOM Impl in JS > > Review of attachment 641687 [details] [diff] [review]: > ----------------------------------------------------------------- > > Does this patch even intend to implement WebActivities for web pages? > I think this patch does not try to do that. There is only a few changes to do to make it happens but it misses some clarity on the API for web pages right now. Which title/icon should you use when you register an activity from a web page? > @@ +58,5 @@ > > + let json = { > > + "oid": this._id, > > + "manifest": this.getManifestURL(), > > + "name": aHandlerDescription.name, > > + "description": aHandlerDescription, > > You need to sanitize the aHandlerDescription.filters property here or in the > service since XPConnect won't do it for you. I can't think of any way to > directly exploit this, but it's scary that totally arbitrary data is passed > to the parent process and then saved in IndexedDB. > > The URL passed in aHandlerDescription.href needs to be resolved against the > current page base-URL and then checked for same-origin-ness with the current > page. > > Same in unregister and isRegistered Can it be done in a followup? I would like to have a better idea of what is valid for aHandlerDescription and what is not and I guess this will need some discussions.

(In reply to Vivien Nicolas (:vingtetun) from comment #92) > Created attachment 643395 [details] [diff] [review] > Part 4 - DOM Impl in JS (v 0.4) > > The UI will make ActivityGlue fires a USER_ABORT if the application is > closed without returning any values. This is not done in Gaia yet but it > will be it's responsibility. What do you mean by close? Lost blur or killed? That should happen for both cases IIRC. When |returnValue| is true. Otherwise, as soon as the app is launched, the success event should be fired. > @@ +102,5 @@ > > + QueryInterface: XPCOMUtils.generateQI([ > > + Ci.nsIActivityManager, Ci.nsIDOMGlobalPropertyInitializer > > + ]), > > + > > + classInfo: XPCOMUtils.generateCI({ > > > It's not a DOM object. I believe you don't need that, right? > > It still an xpcom component :) Still... > ::: dom/apps/src/Webapps.jsm > > @@ +113,5 @@ > > + "title": manifest.name, > > + "icon": manifest.iconURLForSize(128), > > + "description": description > > + } > > + cpmm.sendAsyncMessage("Activities:Register", json); > > > Aren't we the parent process here? Is cpmm.sendAsyncMessage() sending the message to the same process if there is no parent? I'm not very familiar to cpmm/ppmm but I would like to understand how that happens to work. > > We are. But ppmm can't send message to itself. We need to use cppm to speak > to the parent messages loop. So, what we actually want to do here is to send the message to the process we are in. right? Do we have something less confusing to do that than using cpmm? > @@ +103,5 @@ > > + if (!aManifest.activities) { > > + return; > > + } > > + > > + let manifest = new DOMApplicationManifest(aManifest, aApp.origin); > > > That is *so* confusing... Maybe a quick comment explaining that DOMApplicationManifest() returns a manifest object with localized fields and some helpers would help understand why we have |aManifest| and |manifest| in the same method. > > I agree :/ But this same line of code is used in a few different places. If > it needs to be fixed I believe it can be done in a followup and > DOMApplicationManifest could be renamed because I don't think this name > makes sense. Sure. I would like you to put a comment. Not to change everything. > For now all the navigator.* interfaces returns NS_ERROR_NOT_IMPLEMENTED. I > can remove them completely if you want but since some part of this code have > already been r+'ed I still think it makes sense to land them. But I have not > strong opinion on that so if you insist I can remove it completely. Using NS_ERROR_NOT_IMPLEMENTED for methods we expose to the web is wrong. We should simply remove those.

(In reply to Vivien Nicolas (:vingtetun) from comment #93) > (In reply to Jonas Sicking (:sicking) from comment #90) > > Comment on attachment 641687 [details] [diff] [review] > > Part 4 - DOM Impl in JS > > > > Review of attachment 641687 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Does this patch even intend to implement WebActivities for web pages? > > > > I think this patch does not try to do that. There is only a few changes to > do to make it happens but it misses some clarity on the API for web pages > right now. Which title/icon should you use when you register an activity > from a web page? title should be page URL and there should be no icon IIRC. > > @@ +58,5 @@ > > > + let json = { > > > + "oid": this._id, > > > + "manifest": this.getManifestURL(), > > > + "name": aHandlerDescription.name, > > > + "description": aHandlerDescription, > > > > You need to sanitize the aHandlerDescription.filters property here or in the > > service since XPConnect won't do it for you. I can't think of any way to > > directly exploit this, but it's scary that totally arbitrary data is passed > > to the parent process and then saved in IndexedDB. > > > > The URL passed in aHandlerDescription.href needs to be resolved against the > > current page base-URL and then checked for same-origin-ness with the current > > page. > > > > Same in unregister and isRegistered > > > Can it be done in a followup? I would like to have a better idea of what is > valid for aHandlerDescription and what is not and I guess this will need > some discussions. Follow-up will be fine.

(In reply to Mounir Lamouri (:mounir) from comment #95) > (In reply to Vivien Nicolas (:vingtetun) from comment #93) > > (In reply to Jonas Sicking (:sicking) from comment #90) > > > Comment on attachment 641687 [details] [diff] [review] > > > Part 4 - DOM Impl in JS > > > > > > Review of attachment 641687 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Does this patch even intend to implement WebActivities for web pages? > > > > > > > I think this patch does not try to do that. There is only a few changes to > > do to make it happens but it misses some clarity on the API for web pages > > right now. Which title/icon should you use when you register an activity > > from a web page? > > title should be page URL and there should be no icon IIRC. > I think this can be discussed in a different bug so I won't bikeshed too much but pageURL seems bad in terms of UX. Maybe both the pageURL and the page title could be saved and both can be displayed in the UI? And why not the favicon? /me will open a new bug.

Comment on attachment 643395 [details] [diff] [review] Part 4 - DOM Impl in JS (v 0.4) Review of attachment 643395 [details] [diff] [review]: ----------------------------------------------------------------- You should remove ActivityManager (.js and .idl), we don't need those given that we don't have an API in the navigator object. Please, update your patch with the following comments applied. I will try to review with tonight. ::: dom/activities/interfaces/nsIActivityUIGlue.idl @@ +16,5 @@ > +[scriptable, uuid(8624ad73-937a-400f-9d93-39ab5449b867)] > +interface nsIActivityUIGlue : nsISupports > +{ > + /** > + * @param name The name of the activity handler nit: "The name of the activity to handle." @@ +17,5 @@ > +interface nsIActivityUIGlue : nsISupports > +{ > + /** > + * @param name The name of the activity handler > + * @param activities A json blob which is an array of { "name":"...", "icon":"..." }. { "title": ..., "icon": ... } would be less confusing. ::: dom/activities/src/ActivitiesService.jsm @@ +53,5 @@ > + * manifest: String > + * name: String > + * title: String > + * icon: String > + * description: jsval We don't save filters? @@ +114,5 @@ > + store.put(object); > + }.bind(this), aSuccess, aError); > + }, > + > + // we want to remove all activities for (action, url) (name, manifest) @@ +120,5 @@ > + this.newTxn("readwrite", function (txn, store) { > + let object = { > + manifest: aObject.manifest, > + name: aObject.name, > + description: aObject.description Actually, you don't need |description| too. @@ +137,5 @@ > + request.onsuccess = function findSuccess(aEvent) { > + debug("Request successful. Record count: " + aEvent.target.result.length); > + if (!txn.result) { > + txn.result = { > + name: '', name: aObject.options.name, @@ +146,5 @@ > + aEvent.target.result.forEach(function(result) { > + if (!aMatch(result)) > + return; > + > + txn.result.name = result.title; txn.result.name = aObject.options.name; @@ +150,5 @@ > + txn.result.name = result.title; > + txn.result.options.push({ > + manifest: result.manifest, > + name: result.name, > + icon: result.icon, You need title here. @@ +172,5 @@ > + // ActivityManager.js > + "Activities:Register", > + "Activities:Unregister", > + ], > + init: function activities_init() { nit: put an empty line before |init: function [...]| @@ +237,5 @@ > + debug("Sending system message..."); > + sysmm.sendMessage("activity", { > + "id": aMsg.id, > + "payload": aMsg.options > + }, Services.io.newURI(aResults.options[aChoice].manifest, null, null)); If !returnValue, you should send a 'success' event to the DOMRequest now. @@ +251,5 @@ > + debug("Error in startActivity: " + aError + "\n"); > + }; > + > + let matchFunc = function matchFunc(aResult) { > + for (let prop in aResult.data) { shouldn't that be |for(let prop in aResult.filters) {| ? @@ +288,5 @@ > + mm.sendAsyncMessage("Activities:Register:KO", msg); > + }); > + break; > + case "Activities:Unregister": > + this.db.remove(msg, function onSuccess(aEvent) { Maybe db.remove() shouldn't take callbacks given that we don't use them? ::: dom/apps/src/Webapps.jsm @@ +27,5 @@ > return Cc["@mozilla.org/parentprocessmessagemanager;1"] > .getService(Ci.nsIFrameMessageManager); > }); > > +XPCOMUtils.defineLazyServiceGetter(this, "cpmm", I wonder if we could not use that. @@ +120,5 @@ > + "description": description > + } > + cpmm.sendAsyncMessage("Activities:Register", json); > + > + let launchPath = Services.io.newURI(manifest.fullLaunchPath(description.href), null, null); nit: let launchPath = Services.io.newURI(manifest.fullLaunchPath(description.href), null, null);

The same with review comments addressed. (In reply to Mounir Lamouri (:mounir) from comment #97) > Comment on attachment 643395 [details] [diff] [review] > Part 4 - DOM Impl in JS (v 0.4) > > Review of attachment 643395 [details] [diff] [review]: > ----------------------------------------------------------------- Sorry for all the confusion between name, title... > ::: dom/activities/src/ActivitiesService.jsm > @@ +53,5 @@ > > + * manifest: String > > + * name: String > > + * title: String > > + * icon: String > > + * description: jsval > > We don't save filters? They are stored via description. > ::: dom/apps/src/Webapps.jsm > @@ +27,5 @@ > > return Cc["@mozilla.org/parentprocessmessagemanager;1"] > > .getService(Ci.nsIFrameMessageManager); > > }); > > > > +XPCOMUtils.defineLazyServiceGetter(this, "cpmm", > > I wonder if we could not use that. > I have keep it for now. Not a big deal.

Comment on attachment 643536 [details] [diff] [review] Part 3 - DOM Impl in JS (v 0.5) Review of attachment 643536 [details] [diff] [review]: ----------------------------------------------------------------- You haven't replied about the returnValue issue. When there is no return value, selecting an activity handler should fire a success event to the caller (and open the handler). If there is a return value, that should be handled by the handler. Your patch isn't doing anything like that and should probably fire a success event when starting the handler if returnValue is false. r-, unless you have a plan or this is actually handled somewhere else. This shouldn't be hard to fix. I have another review comment, see below. ::: dom/activities/src/ActivitiesService.jsm @@ +145,5 @@ > + aEvent.target.result.forEach(function(result) { > + if (!aMatch(result)) > + return; > + > + txn.result.title = result.title; No... aObject.options.name.

(In reply to Mounir Lamouri (:mounir) from comment #99) > Comment on attachment 643536 [details] [diff] [review] > Part 3 - DOM Impl in JS (v 0.5) > > Review of attachment 643536 [details] [diff] [review]: > ----------------------------------------------------------------- > > You haven't replied about the returnValue issue. When there is no return > value, selecting an activity handler should fire a success event to the > caller (and open the handler). If there is a return value, that should be > handled by the handler. Your patch isn't doing anything like that and should > probably fire a success event when starting the handler if returnValue is > false. > > r-, unless you have a plan or this is actually handled somewhere else. This > shouldn't be hard to fix. Sigh... I have qrefresh it in the wrong patch... > I have another review comment, see below. > > ::: dom/activities/src/ActivitiesService.jsm > @@ +145,5 @@ > > + aEvent.target.result.forEach(function(result) { > > + if (!aMatch(result)) > > + return; > > + > > + txn.result.title = result.title; > > No... aObject.options.name. Actually I want to remove this line...

Comment on attachment 643774 [details] [diff] [review] Part 3 - DOM Impl in JS (v 0.6) Review of attachment 643774 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments addressed ::: dom/activities/src/ActivitiesService.jsm @@ +67,5 @@ > + > + debug("Created object stores and indexes"); > + }, > + > + // unique ids made of uri, action and filters update that comment based on my comment below @@ +85,5 @@ > + }); > + > + // add the filter keys=value tuples, sorting keys > + if (aObject.data) { > + let filters = Object.keys(aObject.data).sort(); Sorry to see that so late but it needs to be: aObject.description.filters I believe This said, I think you can also remove that and make the id generated based on URI and action. Let's not use |filters| for that now. That means a page will not be able to register for two activities af the same name with different filters. That can be done in a follow-up. @@ +239,5 @@ > + "payload": aMsg.options > + }, Services.io.newURI(result.manifest, null, null)); > + > + if (!result.description.returnValue) { > + ppmm.sendAsyncMessage("Activity:FireSuccess", { "id": aMsg.id }); Could you pass "result": null? Also, the idea was that some handlers will not set |returnValue| because the UA will know what should be done for some activities. Like "view" shouldn't return a value. Could you open a follow-up to do that?

try still running: https://tbpl.mozilla.org/?tree=Try&rev=c8164149e41c

(In reply to Vivien Nicolas (:vingtetun) from comment #103) > try still running: https://tbpl.mozilla.org/?tree=Try&rev=c8164149e41c Working on fixing a test failure.

Pushed a new patch to the try: https://tbpl.mozilla.org/?tree=Try&rev=b3fd739dc152

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a3791473b56 https://hg.mozilla.org/integration/mozilla-inbound/rev/7938ab0b63db https://hg.mozilla.org/integration/mozilla-inbound/rev/09094f943d29 https://hg.mozilla.org/integration/mozilla-inbound/rev/53da353104ee

https://hg.mozilla.org/mozilla-central/rev/53da353104ee https://hg.mozilla.org/mozilla-central/rev/09094f943d29 https://hg.mozilla.org/mozilla-central/rev/7938ab0b63db https://hg.mozilla.org/mozilla-central/rev/6a3791473b56

Comment on attachment 641949 [details] [diff] [review] Part 2 - implementation of the Activity object Review of attachment 641949 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/activities/src/Activity.cpp @@ +47,5 @@ > + > + Init(window); > + > + // We expect a single argument, which is a nsIDOMMozActivityOptions. > + if (aArgc != 1 || !aArgv[0].isObject()) { Should only throw if aArgc < 1, not when aArgc > 1.

Documentation is available here: - https://developer.mozilla.org/en-US/docs/WebAPI/Web_Activities - https://developer.mozilla.org/en-US/docs/DOM/MozActivity - https://developer.mozilla.org/en-US/docs/DOM/MozActivityRequestHandler - https://developer.mozilla.org/en-US/docs/DOM/MozActivityOptions It needs technical review, feel free to provide me any feedback on it.

Is the Web Activities API intended to be usable on desktop? Is see some patches here had Firefox desktop parts, but it seems that hasn't landed; what's the status?

(In reply to Florian Quèze [:florian] [:flo] from comment #110) > Is the Web Activities API intended to be usable on desktop? > > Is see some patches here had Firefox desktop parts, but it seems that hasn't > landed; what's the status? We first need desktop support for system messages (bug 868322) and there are also UX issues on desktop to solve.

(In reply to Florian Quèze [:florian] [:flo] from comment #110) > Is the Web Activities API intended to be usable on desktop? No. The Firefox Desktop team is aware of that but it is a low priority. Hopefully, the priority will raise.