Add data property in persistent web notification

Add data property in persistent web notification

When creating a persistent notification from ServiceWorker, the
specification defines a "data" attribute that authors can use to store
arbitrary, serializable content in.

For storing arbitrary, serialized data should be sent to chromium.
And chromium will store this data.
Afterward, this data will be return when onnotificationclick event is fired.

There is no way to test about notification.data in persistent notification, since chromium doesn't support to store this data now. 

The Web Notification specification:
https://notifications.spec.whatwg.org/#dom-notification-data

BUG=442129, 467812
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191976

[Sanghyun Park, 2015-03-15 14:16:38]

sh919.park@samsung.com changed reviewers:
+ peter@chromium.org

[Sanghyun Park, 2015-03-15 14:23:55]

sh919.park@samsung.com changed reviewers:
+ l.gombos@samsung.com

[Sanghyun Park, 2015-03-15 14:23:55]

This patch is for persistent web notification.
Please refer the below comment. 
Thanks.

https://codereview.chromium.org/1005353002/diff/1/public/platform/modules/not...
File public/platform/modules/notifications/WebNotificationData.h (right):

https://codereview.chromium.org/1005353002/diff/1/public/platform/modules/not...
public/platform/modules/notifications/WebNotificationData.h:45: WebString
dataAsWireString;
I think that there is no necessity for using WebSerializedScrtiptValue.
Because  chromium will only store data's WireString without other action.
What do you think about this?

[Peter Beverloo, 2015-03-15 16:25:05]

Thanks!

lgtm % the following comments. I've launched some try-bots as well.

What's the plan for testing this? Until the database is completely hooked up
there won't be a convenient way to do so. At the very least, please file a bug
to make sure we don't forget.

https://codereview.chromium.org/1005353002/diff/20001/Source/modules/notifica...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1005353002/diff/20001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:172: // No send data property in
non-persistent notification.
It would be good to have a more helpful comment here to explain *why* we don't
include data for non-persistent notifications. What about:

"The lifetime and availability of non-persistent notifications is tied to the
page they were created by, and thus the data doesn't have to be known to the
embedder."

https://codereview.chromium.org/1005353002/diff/20001/public/platform/modules...
File public/platform/modules/notifications/WebNotificationData.h (right):

https://codereview.chromium.org/1005353002/diff/20001/public/platform/modules...
public/platform/modules/notifications/WebNotificationData.h:45: WebString
dataAsWireString;
Can we just call this "data"? I agree that its contents can be opaque to the
embedder, in which case it doesn't have to know that it's a serialized wire
string either :-).

[Sanghyun Park, 2015-03-15 17:44:22]

On 2015/03/15 16:25:05, Peter Beverloo wrote:
> Thanks!
> 
> lgtm % the following comments. I've launched some try-bots as well.
> 
> What's the plan for testing this? Until the database is completely hooked up
> there won't be a convenient way to do so. At the very least, please file a bug
> to make sure we don't forget.

Sure, I'm also worried about this.
I'll make a bug about this testing. 

> 
>
https://codereview.chromium.org/1005353002/diff/20001/Source/modules/notifica...
> File Source/modules/notifications/Notification.cpp (right):
> 
>
https://codereview.chromium.org/1005353002/diff/20001/Source/modules/notifica...
> Source/modules/notifications/Notification.cpp:172: // No send data property in
> non-persistent notification.
> It would be good to have a more helpful comment here to explain *why* we don't
> include data for non-persistent notifications. What about:
> 
> "The lifetime and availability of non-persistent notifications is tied to the
> page they were created by, and thus the data doesn't have to be known to the
> embedder."
> 
>
https://codereview.chromium.org/1005353002/diff/20001/public/platform/modules...
> File public/platform/modules/notifications/WebNotificationData.h (right):
> 
>
https://codereview.chromium.org/1005353002/diff/20001/public/platform/modules...
> public/platform/modules/notifications/WebNotificationData.h:45: WebString
> dataAsWireString;
> Can we just call this "data"? I agree that its contents can be opaque to the
> embedder, in which case it doesn't have to know that it's a serialized wire
> string either :-).

[Sanghyun Park, 2015-03-15 17:45:05]

Thanks for review.

I'll fix about these

https://codereview.chromium.org/1005353002/diff/20001/Source/modules/notifica...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1005353002/diff/20001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:172: // No send data property in
non-persistent notification.
On 2015/03/15 16:25:05, Peter Beverloo wrote:
> It would be good to have a more helpful comment here to explain *why* we don't
> include data for non-persistent notifications. What about:
> 
> "The lifetime and availability of non-persistent notifications is tied to the
> page they were created by, and thus the data doesn't have to be known to the
> embedder."

Okay, I will add this comment. :)

https://codereview.chromium.org/1005353002/diff/20001/public/platform/modules...
File public/platform/modules/notifications/WebNotificationData.h (right):

https://codereview.chromium.org/1005353002/diff/20001/public/platform/modules...
public/platform/modules/notifications/WebNotificationData.h:45: WebString
dataAsWireString;
On 2015/03/15 16:25:05, Peter Beverloo wrote:
> Can we just call this "data"? I agree that its contents can be opaque to the
> embedder, in which case it doesn't have to know that it's a serialized wire
> string either :-).

Thanks for your agree :).
I'll change this name to "data"

[Sanghyun Park, 2015-03-15 18:05:58]

Patchset #3 (id:40001) has been deleted

[Sanghyun Park, 2015-03-15 18:07:39]

The CQ bit was checked by sh919.park@samsung.com

[Sanghyun Park, 2015-03-15 18:07:40]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1005353002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-03-15 18:07:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1005353002/60001

[commit-bot: I haz the power, 2015-03-15 18:55:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-15 18:55:13]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[Peter Beverloo, 2015-03-15 22:35:04]

Two optional nits and one comment - nothing serious :)

https://codereview.chromium.org/1005353002/diff/60001/Source/modules/notifica...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1005353002/diff/60001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:172: // No send data property in
non-persistent notification.
nit: drop line 172, it doesn't add anything. Lines 173-175 should be split
between two lines :).

https://codereview.chromium.org/1005353002/diff/60001/Source/modules/notifica...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.h
(right):

https://codereview.chromium.org/1005353002/diff/60001/Source/modules/notifica...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.h:19: static
ScriptPromise showNotification(ScriptState*, ServiceWorkerRegistration&, const
String& title, const NotificationOptions&,  ExceptionState&);
micro nit: double space

https://codereview.chromium.org/1005353002/diff/60001/public/platform/modules...
File public/platform/modules/notifications/WebNotificationData.h (right):

https://codereview.chromium.org/1005353002/diff/60001/public/platform/modules...
public/platform/modules/notifications/WebNotificationData.h:34: , data(data)
Sorry -- as mentioned in the previous CL, you will still need to include the
"data"-less constructor in this CL until Chromium gets updated to always pass
|data|.

[Sanghyun Park, 2015-03-16 00:00:21]

Thank you for review.

https://codereview.chromium.org/1005353002/diff/60001/Source/modules/notifica...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1005353002/diff/60001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:172: // No send data property in
non-persistent notification.
On 2015/03/15 22:35:04, Peter Beverloo wrote:
> nit: drop line 172, it doesn't add anything. Lines 173-175 should be split
> between two lines :
I'll fix this

https://codereview.chromium.org/1005353002/diff/60001/Source/modules/notifica...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.h
(right):

https://codereview.chromium.org/1005353002/diff/60001/Source/modules/notifica...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.h:19: static
ScriptPromise showNotification(ScriptState*, ServiceWorkerRegistration&, const
String& title, const NotificationOptions&,  ExceptionState&);
On 2015/03/15 22:35:04, Peter Beverloo wrote:
> micro nit: double space

Oops, I'll fix this..

https://codereview.chromium.org/1005353002/diff/60001/public/platform/modules...
File public/platform/modules/notifications/WebNotificationData.h (right):

https://codereview.chromium.org/1005353002/diff/60001/public/platform/modules...
public/platform/modules/notifications/WebNotificationData.h:34: , data(data)
On 2015/03/15 22:35:04, Peter Beverloo wrote:
> Sorry -- as mentioned in the previous CL, you will still need to include the
> "data"-less constructor in this CL until Chromium gets updated to always pass
> |data|.

Oops it's my mistake. Sorry.
How about make 1 more constructor like before?
WDYT?

https://codereview.chromium.org/1005353002/diff/60001/public/platform/modules...
public/platform/modules/notifications/WebNotificationData.h:34: , data(data)
On 2015/03/15 22:35:04, Peter Beverloo wrote:
> Sorry -- as mentioned in the previous CL, you will still need to include the
> "data"-less constructor in this CL until Chromium gets updated to always pass
> |data|.

Oops, it's my mistake. Sorry...
I'll add "data"-less constructor.

[Sanghyun Park, 2015-03-16 06:26:10]

Patchset #4 (id:80001) has been deleted

[Sanghyun Park, 2015-03-16 06:33:49]

Dear Peter.
I uploaded new patch.
Would you review this again?

[Sanghyun Park, 2015-03-16 06:54:35]

sh919.park@samsung.com changed reviewers:
+ keonho07.kim@samsung.com

[Peter Beverloo, 2015-03-16 11:19:30]

Please mention the bug you filed for testing this feature in the CL description
(in the BUG= line). Still lg otherwise.

https://codereview.chromium.org/1005353002/diff/100001/public/platform/module...
File public/platform/modules/notifications/WebNotificationData.h (right):

https://codereview.chromium.org/1005353002/diff/100001/public/platform/module...
public/platform/modules/notifications/WebNotificationData.h:26:
WebNotificationData(const WebString& title, Direction direction, const
WebString& lang, const WebString& body, const WebString& tag, const WebURL&
icon, bool silent)
Please add a FIXME to remove this constructor when the embedder switches over.

[Sanghyun Park, 2015-03-16 12:09:35]

Thanks :)

On 2015/03/16 11:19:30, Peter Beverloo wrote:
> Please mention the bug you filed for testing this feature in the CL
description
> (in the BUG= line). Still lg otherwise.

I don't have EditBugs permission.
After getting permission, I'll make a bug and add bugId in this CL.

> 
>
https://codereview.chromium.org/1005353002/diff/100001/public/platform/module...
> File public/platform/modules/notifications/WebNotificationData.h (right):
> 
>
https://codereview.chromium.org/1005353002/diff/100001/public/platform/module...
> public/platform/modules/notifications/WebNotificationData.h:26:
> WebNotificationData(const WebString& title, Direction direction, const
> WebString& lang, const WebString& body, const WebString& tag, const WebURL&
> icon, bool silent)
> Please add a FIXME to remove this constructor when the embedder switches over.
I'll add FIXME.

[Sanghyun Park, 2015-03-17 03:10:46]

The CQ bit was checked by sh919.park@samsung.com

[Sanghyun Park, 2015-03-17 03:10:47]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1005353002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-03-17 03:11:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1005353002/120001

[commit-bot: I haz the power, 2015-03-17 04:58:59]

Committed patchset #5 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191976