In some applications (notably the Sms app), we encode informations inside the icon URL, so that we can retrieve it when we receive the object through the system message. It would be easier to have a `state` arbitrary object instead, like what we have in the pushState API.

How do you retrieve the icon URL? That was only recently added to the API... Storing state on the object might be an idea, but we should probably discuss that on the WHATWG list first.

Seems like we have it in the system message's imageURL property: https://mxr.mozilla.org/mozilla-b2g18/source/b2g/chrome/content/shell.js#712

> but we should probably discuss > that on the WHATWG list first. Anne, can you handle this discussion ? To be honest, I don't feel like subscribing to this list for this ;-)

The e-mail app also definitely wants that state object too. On the recent dev-webapi thread about the e-mail app's many desires I described our use-case (for what I called 'data', but 'state' sounds good too) in https://groups.google.com/d/msg/mozilla.dev.webapi/vxfl3yNW4x0/udYQ_1fUTcMJ

Having this would help cleaning up usage of Notification as part of the migration to the new API in bug 938541.

Yup, this is now a part of the official spec: http://notifications.spec.whatwg.org/#dom-notification-data Robert Bindar is currently working on the gecko implementation.

With the current structured cloned api we can pass to the data object primitive types, ArrayBuffer, ImageData, Map, Set, Date and RegExp objects and these can be successfully stored into the database and can be successfully fetched as well, but unfortunately the system messages manager uses Cu.cloneInto to clone the payloads and therefore the system app is able to see only primitive types, same situation for any app that receives a notification via mozSetMessageHandler.

Comment on attachment 8470413 [details] [diff] [review] custom_data_support.patch This needs to be reviewed by a DOM peer. Ben, can you take a look at this, or forward to someone who can.

This is a try run, some failed tests, but they seem unrelated. https://tbpl.mozilla.org/?tree=Try&rev=611245864b9f

Comment on attachment 8470413 [details] [diff] [review] custom_data_support.patch Andrea, would you like to review this? If not flip back to me.

Comment on attachment 8470413 [details] [diff] [review] custom_data_support.patch Review of attachment 8470413 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. but I want to review it again. ::: b2g/components/AlertsService.js @@ +137,5 @@ > // It seems like there is no callbacks anymore, forward the click on > // notification via a system message containing the title/text/icon of > // the notification so the app get a change to react. > if (data.target) { > + let dataObj = this.deserializeStructuredClone(listener.dataStr); question: can you deserialize the string when you create the listener (line 108) ? In this way you don't have to deserialize the string for any message. @@ +166,5 @@ > delete this._listeners[data.uid]; > } > + }, > + > + deserializeStructuredClone: function(dataString) { Can you use this method exported by AlertsHelper.jsm ? @@ +184,5 @@ > + // nsIStructuredCloneContainer, but not by Cu.cloneInto), e.g. ImageData. > + // After the structured clone callback systems will be unified, we'll not > + // have to perform this check anymore. > + try { > + let data = Cu.cloneInto(dataObj, {}); This is actually quite heavy because uses the structured clone algorithm again. Would be nice to have: Cu.clonableInto(dataObj, {}) that returns a boolean. Follow up? ::: dom/interfaces/notification/nsINotificationStorage.idl @@ -21,2 @@ > */ > [implicit_jscontext] you must change the UUID for this IDL interface. ::: dom/src/notification/Notification.cpp @@ +76,5 @@ > options); > + ErrorResult rv; > + notification->InitData(aCx, aData, rv); > + if (rv.Failed()) { > + return NS_ERROR_FAILURE; you should return the correct error value. @@ +451,5 @@ > > nsString alertName; > notification->GetAlertName(alertName); > > + nsString dataString = EmptyString(); nsString dataString; without EmptyString() @@ +513,5 @@ > > +Notification::~Notification() {} > + > +NS_IMPL_CYCLE_COLLECTION_CLASS(Notification) > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(Notification, DOMEventTargetHelper) mDataObjectContainer @@ +517,5 @@ > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(Notification, DOMEventTargetHelper) > + NS_IMPL_CYCLE_COLLECTION_UNLINK(mData) > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(Notification, DOMEventTargetHelper) mDataObjectContainer @@ +575,5 @@ > nsCOMPtr<nsIObserver> observer = new NotificationObserver(this); > > + // mDataObjectContainer might be uninitialized here because the notification > + // was constructed with an undefined data property. > + nsString dataStr = EmptyString(); No EmtpyString() @@ +837,5 @@ > +{ > + if (!mData && mDataObjectContainer) { > + nsresult rv; > + rv = mDataObjectContainer->DeserializeToVariant(aCx, getter_AddRefs(mData)); > + if (NS_FAILED(rv)) { if (NS_WARN_IF(NS_FAILED(rv))) @@ +841,5 @@ > + if (NS_FAILED(rv)) { > + aRetval.setNull(); > + return; > + } > + } at this point, it can be that mData is null. I would like to see: if (!mData) { aRetval.setNull(); return; } @@ +846,5 @@ > + VariantToJsval(aCx, mData, aRetval); > +} > + > +void > +Notification::InitData(JSContext* aCx, JS::Handle<JS::Value> aData, Call this: InitFromJSVal @@ +856,5 @@ > + mDataObjectContainer = new nsStructuredCloneContainer(); > + aRv = mDataObjectContainer->InitFromJSVal(aData); > +} > + > +void Notification::InitData(JSContext* aCx, const nsAString& aData, Call this: InitFromBase64 it helps to understand what aData is. ::: dom/src/notification/Notification.h @@ +9,5 @@ > #include "mozilla/dom/NotificationBinding.h" > > #include "nsIObserver.h" > > +#include "nsStructuredCloneContainer.h" remove this @@ +14,3 @@ > #include "nsCycleCollectionParticipant.h" > > class nsIPrincipal; class nsIStructuredCloneContainer; class nsIVariant; @@ +75,5 @@ > { > aRetval = mIconUrl; > } > > + void GetDataCloneContainer(nsCOMPtr<nsIStructuredCloneContainer> &aRet) >& aRet @@ +77,5 @@ > } > > + void GetDataCloneContainer(nsCOMPtr<nsIStructuredCloneContainer> &aRet) > + { > + aRet = mDataObjectContainer; This should be: nSIStructuredCloneContainer* GetDataCloneContainer() { return mDataObjectcontainer; } ::: toolkit/components/alerts/nsIAlertsService.idl @@ +56,5 @@ > in AString title, > in AString text, > [optional] in boolean textClickable, > [optional] in AString cookie, > [optional] in nsIObserver alertListener, change the UUID of this interface.

(In reply to Andrea Marchesini (:baku) from comment #14) > Comment on attachment 8470413 [details] [diff] [review] > custom_data_support.patch > > Review of attachment 8470413 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. but I want to review it again. > > ::: b2g/components/AlertsService.js > @@ +137,5 @@ > > // It seems like there is no callbacks anymore, forward the click on > > // notification via a system message containing the title/text/icon of > > // the notification so the app get a change to react. > > if (data.target) { > > + let dataObj = this.deserializeStructuredClone(listener.dataStr); > > question: can you deserialize the string when you create the listener (line > 108) ? > In this way you don't have to deserialize the string for any message. This is a good idea. > @@ +184,5 @@ > > + // nsIStructuredCloneContainer, but not by Cu.cloneInto), e.g. ImageData. > > + // After the structured clone callback systems will be unified, we'll not > > + // have to perform this check anymore. > > + try { > > + let data = Cu.cloneInto(dataObj, {}); > > This is actually quite heavy because uses the structured clone algorithm > again. > Would be nice to have: Cu.clonableInto(dataObj, {}) that returns a boolean. > Follow up? > Indeed, it would be nice to have it :) I'll file a bug for this.

I'll submit the patch for review again as soon as I can, I'm delayed by a failing rooting hazard test.

I fixed the nits and I solved the failed static analysis test. An observation would be that we cannot reuse the code for deserializeStructuredClone whithout IPC, AlertsHelper.jsm and AlertsService.js live in different processes. I'll be back with a follow up for that function "clonableInto(..)".

This is the try run where the rooting test failed https://tbpl.mozilla.org/?tree=Try&rev=812b9315f001 Here it is fixed https://tbpl.mozilla.org/?tree=Try&rev=e4ca5700bf65 I can run a new one, but I don't think it's necessary, the change is insignificant.

Comment on attachment 8475593 [details] [diff] [review] custom_data_support.patch Review of attachment 8475593 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/notification/nsINotificationStorage.idl @@ +64,2 @@ > void put(in DOMString origin, > in DOMString id, change the UUID also for nsINotificationStorage

This is the follow up we spoke about: https://bugzilla.mozilla.org/show_bug.cgi?id=1056234

rebased patch

Comment on attachment 8470415 [details] gaia integration tests https://tbpl.mozilla.org/?rev=f4a41b1af77d3eb3993112086c994291c6f8aa3b&tree=Gaia-Try Apparently Gij is broken.

Comment on attachment 8470415 [details] gaia integration tests Can you file a followup for the integration test part? Add details in that bug for what tests are currently failing, and what tests we need to add. That way we can keep this bug closed for the gecko part, and fix the gaia part separately.

I will file a followup. The failing tests are unrelated, Gij fails for everybody else.