715814 - Web Activities

Status: RESOLVED FIXED

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

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

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

Comment 2

[Mounir Lamouri (:mounir)]

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

Comment 3

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

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

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : IDL

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : DOM implementation

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: Part 3 : Events

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: Part 4 : Desktop support

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: Part 5 : b2g support

Comment 9

[[:fabrice] Fabrice Desré]

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 10

[Mounir Lamouri (:mounir)]

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?

Comment 11

[Mounir Lamouri (:mounir)]

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

[Mounir Lamouri (:mounir)]

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 13

[Mounir Lamouri (:mounir)]

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.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 17

[Mounir Lamouri (:mounir)]

Lets put some pressure on bug 723206...

Comment 18

[Mounir Lamouri (:mounir)]

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

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

[Olli Pettay [:smaug][[e-mail]]]

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 22

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 23

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : IDL

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

Comment 24

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : DOM implementation

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.

Comment 25

[[:fabrice] Fabrice Desré]

Attachment: Part 3 : Events

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

Comment 26

[[:fabrice] Fabrice Desré]

Attachment: Part 4 : Desktop support

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

Comment 27

[[:fabrice] Fabrice Desré]

Attachment: Part 5 : b2g support

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 28

[Olli Pettay [:smaug][[e-mail]]]

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 29

[Mounir Lamouri (:mounir)]

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 30

[Mounir Lamouri (:mounir)]

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?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

d is dictionary ;)

Comment 32

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : IDL

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.

Comment 33

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : DOM implementation

Updated to add the isRegisteredActivityHandler method

Comment 34

[[:fabrice] Fabrice Desré]

Attachment: Part 3 : Events

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

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 36

[[:fabrice] Fabrice Desré]

Attachment: Part 3 : Events

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 37

[Mounir Lamouri (:mounir)]

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

Comment 38

[Etienne Segonzac (:etienne)]

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.

Comment 39

[Etienne Segonzac (:etienne)]

Attachment: Test file for the event bug

Comment 40

[[:fabrice] Fabrice Desré]

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

Comment 41

[[:fabrice] Fabrice Desré]

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

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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 43

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 44

[[:fabrice] Fabrice Desré]

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?

Comment 45

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 46

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

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

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 48

[[:fabrice] Fabrice Desré]

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#

Comment 49

[Dietrich Ayala (:dietrich)]

What else needs to happen for this to land?

Comment 50

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 51

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : IDL v2

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

Comment 52

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : implementation of the Activity object

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 53

[Mounir Lamouri (:mounir)]

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 54

[Mounir Lamouri (:mounir)]

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.

Comment 55

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : IDL v3

I now have one interface per file with comments.

The internal IDL will make a comeback in another patch :)

Comment 56

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : implementation of the Activity object WIP

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 57

[[:fabrice] Fabrice Desré]

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 58

[Mounir Lamouri (:mounir)]

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 59

[Mounir Lamouri (:mounir)]

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

Comment 60

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : IDL v4

mozify all the things!

Comment 61

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : implementation of the Activity object

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 62

[Mounir Lamouri (:mounir)]

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 63

[Mounir Lamouri (:mounir)]

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.

Comment 64

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 2 - implementation of the Activity object

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.

Comment 65

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 3 - Navigator Impl

Comment 66

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 4 - DOM Impl in JS

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 67

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 5 - dom/apps/ part

Comment 68

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 6 - dom/messages/ part

Comment 69

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 7 - b2g/ part

Comment 70

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 71

[Mounir Lamouri (:mounir)]

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 72

[Mounir Lamouri (:mounir)]

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 73

[Andreas Gal :gal]

Comment on attachment 641130 [details] [diff] [review]
Part 5 - dom/apps/ part

Looks like mounir wants to take this.

Comment 74

[Mounir Lamouri (:mounir)]

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 75

[Mounir Lamouri (:mounir)]

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 76

[Mounir Lamouri (:mounir)]

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 77

[Mounir Lamouri (:mounir)]

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 78

[Mounir Lamouri (:mounir)]

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.

Comment 79

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 5 - B2G specific part

Comment 80

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 4 - DOM Impl in JS

Comment 81

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 82

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 83

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 85

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 2 - implementation of the Activity object

Comment 86

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 3 - Navigator Impl

Comment 87

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 88

[Mounir Lamouri (:mounir)]

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 89

[Mounir Lamouri (:mounir)]

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 90

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 91

[Mounir Lamouri (:mounir)]

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.

Comment 92

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

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

Comment 93

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 94

[Mounir Lamouri (:mounir)]

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

Comment 95

[Mounir Lamouri (:mounir)]

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

Comment 96

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

[Mounir Lamouri (:mounir)]

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

Comment 98

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 3 - DOM Impl in JS (v 0.5)

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 99

[Mounir Lamouri (:mounir)]

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.

Comment 100

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 3 - DOM Impl in JS (v 0.6)

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

[Mounir Lamouri (:mounir)]

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?

Comment 102

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Part 4 - B2G specific part

Comment 103

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 104

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 105

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 106

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 107

[Ryan VanderMeulen [:RyanVM]]

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 108

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 109

[Jeremie Patonnier :Jeremie]

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.

Comment 110

[Florian Quèze [:florian]]

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?

Comment 111

[[:fabrice] Fabrice Desré]

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

Comment 112

[Mounir Lamouri (:mounir)]

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