[Notification API] add "data" attribute to NotificationOptions

RESOLVED FIXED in mozilla34

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: julienw, Assigned: Robert Bindar)

Tracking

(Blocks: 1 bug)

22 Branch
mozilla34
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Flags: needinfo?(annevk)

Comment 1

5 years ago
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.
Flags: needinfo?(annevk)
(Reporter)

Comment 2

5 years ago
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
(Reporter)

Updated

5 years ago
Depends on: 782211
(Reporter)

Comment 3

5 years ago
> 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.
Assignee: nobody → robertbindar
Summary: Notification API follow-up: use a "state" object in NotificationOptions object → [Notification API] add "data" attribute to NotificationOptions
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1047218
(Assignee)

Comment 9

4 years ago
Created attachment 8470413 [details] [diff] [review]
custom_data_support.patch

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

Comment 10

4 years ago
Created attachment 8470415 [details] [review]
gaia integration tests
(Assignee)

Updated

4 years ago
Attachment #8470413 - Flags: review?(mhenretty)
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.
Attachment #8470413 - Flags: review?(mhenretty) → review?(bent.mozilla)
(Assignee)

Comment 12

4 years ago
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.
Attachment #8470413 - Flags: review?(bent.mozilla) → review?(amarchesini)
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.
Attachment #8470413 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 16

4 years ago
I'll submit the patch for review again as soon as I can, I'm delayed by a failing rooting hazard test.
(Assignee)

Comment 17

4 years ago
Created attachment 8475593 [details] [diff] [review]
custom_data_support.patch

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(..)".
Attachment #8470413 - Attachment is obsolete: true
Attachment #8475593 - Flags: review?(amarchesini)
(Assignee)

Comment 18

4 years ago
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
Attachment #8475593 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 8476081 [details] [diff] [review]
custom_data_support.patch

This is the follow up we spoke about: https://bugzilla.mozilla.org/show_bug.cgi?id=1056234
Attachment #8475593 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 8476117 [details] [diff] [review]
custom_data_support.patch

rebased patch
Attachment #8476081 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/812981a7ba3f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Updated

4 years ago
Blocks: 1056828

Updated

4 years ago
Blocks: 1057478
(Assignee)

Comment 24

4 years ago
Comment on attachment 8470415 [details] [review]
gaia integration tests

https://tbpl.mozilla.org/?rev=f4a41b1af77d3eb3993112086c994291c6f8aa3b&tree=Gaia-Try

Apparently Gij is broken.
Attachment #8470415 - Flags: review?(mhenretty)
Comment on attachment 8470415 [details] [review]
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.
Attachment #8470415 - Flags: review?(mhenretty)
(Assignee)

Comment 26

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

Updated

4 years ago
Depends on: 1059020
You need to log in before you can comment on or make changes to this bug.