Add the data attribute to the Notification object

Add the data attribute to the Notification object

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

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

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

[Sanghyun Park, 2015-03-10 09:00:06]

Patchset #1 (id:1) has been deleted

[Sanghyun Park, 2015-03-10 09:01:18]

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

[Peter Beverloo, 2015-03-10 23:34:27]

Thank you for the patch!!

Please see my attached comments. Actually supporting this for persistent
notification will be a bit tricky, since we shouldn't store it in memory with
the notifications themselves.

I sent the following design document to blink-dev@ about this, but this will be
a rather involved project that'll take me some time to finish. It's also
unrelated to non-persistent notifications, as their data will never leave Blink.

https://docs.google.com/document/d/1F3CJbVstJeQz4OOFuLtYnrg2svQV87HZt3vIOaJvi...

https://codereview.chromium.org/993893002/diff/20001/LayoutTests/http/tests/n...
File LayoutTests/http/tests/notifications/notification-properties.html (right):

https://codereview.chromium.org/993893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/notification-properties.html:66: // Set
notification's data to a structured clone of options's data.
While I think that the three earlier additions to this file are great, I think
testing the abilities of the .data property should be done in a separate test,
also testing a few more cases.

Some things you can think about:

(1) Storing and restoring different types of values (number, string, object,
array, Number.NaN, and so on).

Since we don't have to test all of SerializedScriptValue, having some kind of
helper function for doing this concisely would be great, having each check look
like:

function assertNotificationDataReflects(value) {
  var notification = new Notification('Title', { data: value });
  assert_equals(notification.data, notification);
}

assertNotificationDataReflects(true);
assertNotificationDataReflects({ a: 1, b: 2 });

(2) Verifying the exception throwing behavior of the method.

It requires adding [RaisesException] to the IDL, so it can throw exceptions. We
need to make sure that these are properly shown when this is the case. One
example case in which this can occur (I think) is when you have a function in
the data property:

new Notification('Title', {
  data: function() { return 5; }
});

https://codereview.chromium.org/993893002/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/custom/V8NotificationCustom.cpp (right):

https://codereview.chromium.org/993893002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/V8NotificationCustom.cpp:24:
v8SetReturnValue(info, result);
I'm not too familiar with custom bindings, but as a preliminary look over this
code, I think we can simplify this a bit by writing it as follows:

=========================
v8::Local<v8::Object> holder = info.Holder();
Notification* impl = V8Notification::toImpl(holder);

if (!impl->data()) {
    v8SetReturnValueNull(info);
    return;
}

v8SetReturnValue(info, impl->data()->deserialize(info.GetIsolate()));
=========================

https://codereview.chromium.org/993893002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/V8NotificationCustom.cpp:27: }
micro nit: // namespace blink

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

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:1: 
nit: no blank line

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:92:
notification->setData(SerializedScriptValueFactory::instance().create(options.data(),
0, exceptionState, options.data().isolate()));
nit: s/0/nullptr/

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:92:
notification->setData(SerializedScriptValueFactory::instance().create(options.data(),
0, exceptionState, options.data().isolate()));
We would need to abort after this call if an exception was thrown.
(exceptionState.hadException())

I'm a bit torn here. On one hand, this feels like the responsibility of the
bindings, and could thus work better in a custom implementation. On the other
hand, that's fragile, and the added complexity here is manageable.

If we do decide to keep the code here, one question is how we'd go about memory
management for the |notification| object, since we already created the
Notification instance at this point. It would make sense to group all the
throwing behavior together, so perhaps move this block to line ~77 and store it
in a temporary variable?

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:93:
notification->data()->registerMemoryAllocatedWithCurrentScriptContext();
I don't think we need this here.

If I read the comment correctly, we should only call
registerMemoryAllocatedWithCurrentScriptContext() if we are the ones owning the
data backing the SerializedScriptValue. This would be true when re-creating the
Notification object to fire it as part of the "notificationclick" event, which
we do on line 120.

In the case of this constructor, which gets called when the developer uses "new
Notification()", the memory associated with the value passed in to the "data"
property is actually owned by the currently executing script.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:166: WebNotificationData
notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
m_data.release());
Why does the embedder need to know about the data associated with non-persistent
notifications? The WebNotificationData object is only use for presentational
purposes there. Instead of passing m_data, using ", nullptr /* data */);" would
be better here.

On a tangent, you would want to use get() instead of release() here. Using
release means that we free up the data after showing the notification, which
means that the following code would break:

=========================
var notification = new Notification('Title', { data: [1, 2, 3] });

console.log(notification.data);  // [1, 2, 3]
notification.onshow = function() {
  console.log(notification.data);  // null
};
=========================

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.h:34: #include
"bindings/core/v8/SerializedScriptValue.h"
nit: I think we can forward declare this? All constructors and destructors
(things that'd automatically touch m_data) are in the .cpp file.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/Notification.idl (right):

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.idl:59: [Custom=Getter] readonly
attribute any data;
Since we haven't send out an Intent to Ship yet, and it will take some time
before we can support this for persistent notifications, you should add a
runtime flag to Source/platform/RuntimeEnabledFeatures.in (with status=testing),
and use a [RuntimeEnabled=] annotation here.

What about "NotificationExperimental" for the name, so that we can re-use it for
future new features?

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/NotificationOptions.idl (right):

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/NotificationOptions.idl:20: any data = null;
While we don't actually generate code for it in the bindings, would you mind
prefixing this with [RuntimeEnabled=] when you add the runtime enabled feature I
noted in a comment slightly further on?

It'll help readability :-).

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:52: 
Mmm this duplication is not great. It happens in more places too. Would you mind
adding a FIXME around line 42 for unifying these code paths at some point in the
future? I'm not sure about a nice way to do that yet :-).

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:55: data
= SerializedScriptValueFactory::instance().create(options.data(), 0,
exceptionState, options.data().isolate());
nit: dito as in Notification.cpp, if exceptionState.hadException() is true then
we need to reject the |promise| and abort.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:56:
data->registerMemoryAllocatedWithCurrentScriptContext();
nit: dito as in Notification.cpp, we shouldn't have
registerMemoryAllocatedWithCurrentScriptContext() here.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:59:
WebNotificationData notification(title, dir, options.lang(), options.body(),
options.tag(), iconUrl, options.silent(), data.release());
note: data.release() is fine here, because we won't be using |data| anywhere
else anymore.

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

https://codereview.chromium.org/993893002/diff/20001/public/platform/modules/...
public/platform/modules/notifications/WebNotificationData.h:39:
WebNotificationData(const WebString& title, Direction direction, const
WebString& lang, const WebString& body, const WebString& tag, const WebURL&
icon, bool silent, const WebSerializedScriptValue& data)
I'm fairly sure that this will break Chromium. You'll need to maintain the
current constructor temporarily until Chromium code has been updated to use the
new one as well.

[Sanghyun Park, 2015-03-11 10:59:52]

Wow. Thank you for kind and detail review. :)
I'll upload fixed version.

https://codereview.chromium.org/993893002/diff/20001/LayoutTests/http/tests/n...
File LayoutTests/http/tests/notifications/notification-properties.html (right):

https://codereview.chromium.org/993893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/notification-properties.html:66: // Set
notification's data to a structured clone of options's data.
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> While I think that the three earlier additions to this file are great, I think
> testing the abilities of the .data property should be done in a separate test,
> also testing a few more cases.
Okay I'll make new test file about data property.

> 
> Some things you can think about:
> 
> (1) Storing and restoring different types of values (number, string, object,
> array, Number.NaN, and so on).
> 
> Since we don't have to test all of SerializedScriptValue, having some kind of
> helper function for doing this concisely would be great, having each check
look
> like:
> 
> function assertNotificationDataReflects(value) {
>   var notification = new Notification('Title', { data: value });
>   assert_equals(notification.data, notification);
> }
> 
> assertNotificationDataReflects(true);
> assertNotificationDataReflects({ a: 1, b: 2 });

It's good idea.
I'll add these. :)

> (2) Verifying the exception throwing behavior of the method.
> 
> It requires adding [RaisesException] to the IDL, so it can throw exceptions.
We
> need to make sure that these are properly shown when this is the case. One
> example case in which this can occur (I think) is when you have a function in
> the data property:
> 
> new Notification('Title', {
>   data: function() { return 5; }
> });

I'll also add negative tests like your comment. :)

https://codereview.chromium.org/993893002/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/custom/V8NotificationCustom.cpp (right):

https://codereview.chromium.org/993893002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/V8NotificationCustom.cpp:24:
v8SetReturnValue(info, result);
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> I'm not too familiar with custom bindings, but as a preliminary look over this
> code, I think we can simplify this a bit by writing it as follows:
> 
> =========================
> v8::Local<v8::Object> holder = info.Holder();
> Notification* impl = V8Notification::toImpl(holder);
> 
> if (!impl->data()) {
>     v8SetReturnValueNull(info);
>     return;
> }
> 
> v8SetReturnValue(info, impl->data()->deserialize(info.GetIsolate()));
> =========================

Your suggestion codes are more clean than this.
I'll fix this.

https://codereview.chromium.org/993893002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/V8NotificationCustom.cpp:27: }
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> micro nit: // namespace blink

Done.

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

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:1: 
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> nit: no blank line

Oops, this is my fault;; 
I'll remove this.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:92:
notification->setData(SerializedScriptValueFactory::instance().create(options.data(),
0, exceptionState, options.data().isolate()));
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> We would need to abort after this call if an exception was thrown.
> (exceptionState.hadException())
> 
> I'm a bit torn here. On one hand, this feels like the responsibility of the
> bindings, and could thus work better in a custom implementation. On the other
> hand, that's fragile, and the added complexity here is manageable.

I fully agree about your opinion.
I think that to use custom bind is better than this.
But, currently custom bind for dictionary type is not supported in idl parser.
I expect that it will take a lot of time for fixing idl parser.
So I'd like to implement this like this now. 
And I hope this will be to move this code to V8NotificationOptionsCustom, when
idl parser suppport custom for dictionary type. 
WDYT?

> 
> If we do decide to keep the code here, one question is how we'd go about
memory
> management for the |notification| object, since we already created the
> Notification instance at this point. It would make sense to group all the
> throwing behavior together, so perhaps move this block to line ~77 and store
it
> in a temporary variable?

Thanks,  I'll move this code to 77 line.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:92:
notification->setData(SerializedScriptValueFactory::instance().create(options.data(),
0, exceptionState, options.data().isolate()));
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> nit: s/0/nullptr/

I'll fix this.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:93:
notification->data()->registerMemoryAllocatedWithCurrentScriptContext();
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> I don't think we need this here.
> 
> If I read the comment correctly, we should only call
> registerMemoryAllocatedWithCurrentScriptContext() if we are the ones owning
the
> data backing the SerializedScriptValue. This would be true when re-creating
the
> Notification object to fire it as part of the "notificationclick" event, which
> we do on line 120.
> 
> In the case of this constructor, which gets called when the developer uses
"new
> Notification()", the memory associated with the value passed in to the "data"
> property is actually owned by the currently executing script.

I had checked about this again. 
You are right!
I misunderstood about registerMemoryAllocatedWithCurrentScriptContext's
comments.
I'll fix this.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:166: WebNotificationData
notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
m_data.release());
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> Why does the embedder need to know about the data associated with
non-persistent
> notifications? The WebNotificationData object is only use for presentational
> purposes there. Instead of passing m_data, using ", nullptr /* data */);"
would
> be better here.
> 
> On a tangent, you would want to use get() instead of release() here. Using
> release means that we free up the data after showing the notification, which
> means that the following code would break:
> 
> =========================
> var notification = new Notification('Title', { data: [1, 2, 3] });
> 
> console.log(notification.data);  // [1, 2, 3]
> notification.onshow = function() {
>   console.log(notification.data);  // null
> };
> =========================

Oops, This is my mistake.
I'll change to data.get().

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.h:34: #include
"bindings/core/v8/SerializedScriptValue.h"
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> nit: I think we can forward declare this? All constructors and destructors
> (things that'd automatically touch m_data) are in the .cpp file.

This header is required, becuase build break is occurred in "void
setData(PassRefPtr<SerializedScriptValue> data) { m_data = data; }"

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/Notification.idl (right):

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.idl:59: [Custom=Getter] readonly
attribute any data;
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> Since we haven't send out an Intent to Ship yet, and it will take some time
> before we can support this for persistent notifications, you should add a
> runtime flag to Source/platform/RuntimeEnabledFeatures.in (with
status=testing),
> and use a [RuntimeEnabled=] annotation here.
> 
> What about "NotificationExperimental" for the name, so that we can re-use it
for
> future new features?

I think this name is fine.:)
I'll add this runtime feature flag.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/NotificationOptions.idl (right):

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/NotificationOptions.idl:20: any data = null;
On 2015/03/10 23:34:26, Peter Beverloo wrote:
> While we don't actually generate code for it in the bindings, would you mind
> prefixing this with [RuntimeEnabled=] when you add the runtime enabled feature
I
> noted in a comment slightly further on?
> 
> It'll help readability :-).

I'll add [RuntimeEnabled=NotificationExperimental] like Notification.idl

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:52: 
On 2015/03/10 23:34:27, Peter Beverloo wrote:
> Mmm this duplication is not great. It happens in more places too. Would you
mind
> adding a FIXME around line 42 for unifying these code paths at some point in
the
> future? I'm not sure about a nice way to do that yet :-).

I cannot understand your comment properly. 
Is your meant that these codes are duplicated with Notification.cpp's code?
If my understanding is right, I'll add "FIXME" comment.
And I'll try to find solution. :)

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:55: data
= SerializedScriptValueFactory::instance().create(options.data(), 0,
exceptionState, options.data().isolate());
On 2015/03/10 23:34:27, Peter Beverloo wrote:
> nit: dito as in Notification.cpp, if exceptionState.hadException() is true
then
> we need to reject the |promise| and abort.

I'll fix this.

https://codereview.chromium.org/993893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:56:
data->registerMemoryAllocatedWithCurrentScriptContext();
On 2015/03/10 23:34:27, Peter Beverloo wrote:
> nit: dito as in Notification.cpp, we shouldn't have
> registerMemoryAllocatedWithCurrentScriptContext() here.

Ditto

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

https://codereview.chromium.org/993893002/diff/20001/public/platform/modules/...
public/platform/modules/notifications/WebNotificationData.h:39:
WebNotificationData(const WebString& title, Direction direction, const
WebString& lang, const WebString& body, const WebString& tag, const WebURL&
icon, bool silent, const WebSerializedScriptValue& data)
On 2015/03/10 23:34:27, Peter Beverloo wrote:
> I'm fairly sure that this will break Chromium. You'll need to maintain the
> current constructor temporarily until Chromium code has been updated to use
the
> new one as well.

I know that this constructor is temporary.
This class has two constructors.
1. WebNotificationData(const WebString& title, Direction direction, const
WebString& lang, const WebString& body, const WebString& tag, const WebURL&
icon))
2. WebNotificationData(const WebString& title, Direction direction, const
WebString& lang, const WebString& body, const WebString& tag, const WebURL&
icon, bool silent, const WebSerializedScriptValue& data)

Currently Chromium use 1st constructor.
So I add data in 2nd.
If I'm wrong, please let me know.

[Sanghyun Park, 2015-03-11 14:39:36]

Patchset #2 (id:40001) has been deleted

[Sanghyun Park, 2015-03-11 14:59:07]

Patchset #2 (id:60001) has been deleted

[Peter Beverloo, 2015-03-12 01:16:25]

peter@chromium.org changed reviewers:
+ haraken@chromium.org

[Peter Beverloo, 2015-03-12 01:16:27]

+haraken

Thank you, that looks much better already! I've got mostly nits, and one larger
comment.

Haraken-san, could you review the bindings and provide some guidance for the
following situation? (See ServiceWorkerRegistrationNotifications.cpp.)

- SWR.showNotification() returns a Promise.
- The C++ implementation uses SerializedScriptValueFactory to apply the
structured clone algorithm and create a wire string for the "data" property.
- We would like to catch this exception, and rather than throwing it, rejecting
the promise with it.

Would the following pattern be an acceptable use of TrackExceptionState?

=====
TrackExceptionState exceptionState;
SerializedScriptValueFactory::instance().create(...)
if (exceptionState.hadException())
  return exceptionState.reject(scriptState);
=====

I see that the bindings do have an ExceptionState, and automatically convert
thrown exceptions to rejected promises.

Unless we use [RaisesException] though, this ExceptionState won't be available
to the implementation. I would prefer to be explicit about "rejecting a promise"
versus "throwing an exception", even through I understand that they are
conceptually analogous...

https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/n...
File LayoutTests/http/tests/notifications/notification-data-property.html
(right):

https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/notification-data-property.html:19: if
(value.constructor.toString().indexOf("Object") > -1)
You can use some slightly nicer checks here;

if (Array.isArray(value))
  // array
else if (typeof value == 'object')
  // object
else
  // anything else

https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/notification-data-property.html:42: var
notification = new Notification('Title', { data: function() { return 1; } });
micro nit: indent +1 space

https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/notification-data-property.html:45: },
'Checks the data of several type property exposed on the Notification object.');
micro nit: indent -4 spaces

https://codereview.chromium.org/993893002/diff/80001/Source/bindings/modules/...
File Source/bindings/modules/v8/custom/V8NotificationCustom.cpp (right):

https://codereview.chromium.org/993893002/diff/80001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/V8NotificationCustom.cpp:8: #include
"bindings/core/v8/SerializedScriptValue.h"
nit: you can drop this include

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:169: WebNotificationData
notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
m_data.get());
Why does the embedder have to know about the data for non-persistent
notifications?

I think we should just pass a nullptr here.

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/Notification.h:120: void
setData(PassRefPtr<SerializedScriptValue> data) { m_data = data; }
nit: please place this under setSilent(). Generally, I try to follow the order
of the IDL file. Same with the getter and the member variable.

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:43: //
FIXME: Unifying these code paths at some point in the future
micro nit: Unifying -> Unify. It would also be good to mention *which* code
paths. What about:

// FIXME: Unify the code path here with the Notification.create() function.

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:48:
return ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(scriptState->isolate(), "No data property can
be serialized"));
Per the specification:
    "4. Set notification's data to a structured clone of options's data. Rethrow
any exceptions."

I don't agree with the specification here. I think it's a bit weird that we have
a function returning a promise that also throws exceptions, that's very
inconvenient.

I filed https://github.com/whatwg/notifications/issues/40 to track this.


Let's assume for a moment that the change gets accepted, and any exceptions
thrown by SerializedScriptValueFactory need to result in a rejected
ScriptPromise.

This means that the [RaisesException] annotation should be removed from the IDL
file. That means that you won't have an ExceptionState object anymore. I wonder
if TrackExceptionState is appropriate to use in this case?

https://codereview.chromium.org/993893002/diff/80001/Source/web/WebSerialized...
File Source/web/WebSerializedScriptValue.cpp (right):

https://codereview.chromium.org/993893002/diff/80001/Source/web/WebSerialized...
Source/web/WebSerializedScriptValue.cpp:85:
WebSerializedScriptValue::WebSerializedScriptValue(SerializedScriptValue* value)
Why do we need this?

In ServiceWorkerRegistrationNotifications::showNotification() you're using
RefPtr<>.release(), which gives us a PassRefPtr, and therefore should hit the
normal constructor.

[haraken, 2015-03-12 01:22:46]

haraken@chromium.org changed reviewers:
+ jl@opera.com, yhirano@chromium.org

[haraken, 2015-03-12 01:22:47]

> +haraken
> 
> Thank you, that looks much better already! I've got mostly nits, and one
larger
> comment.
> 
> Haraken-san, could you review the bindings and provide some guidance for the
> following situation? (See ServiceWorkerRegistrationNotifications.cpp.)
> 
> - SWR.showNotification() returns a Promise.
> - The C++ implementation uses SerializedScriptValueFactory to apply the
> structured clone algorithm and create a wire string for the "data" property.
> - We would like to catch this exception, and rather than throwing it,
rejecting
> the promise with it.
> 
> Would the following pattern be an acceptable use of TrackExceptionState?
> 
> =====
> TrackExceptionState exceptionState;
> SerializedScriptValueFactory::instance().create(...)
> if (exceptionState.hadException())
>   return exceptionState.reject(scriptState);
> =====

I think this is fine, but I want to have yhirano take another look.

> I see that the bindings do have an ExceptionState, and automatically convert
> thrown exceptions to rejected promises.
> 
> Unless we use [RaisesException] though, this ExceptionState won't be available
> to the implementation. I would prefer to be explicit about "rejecting a
promise"
> versus "throwing an exception", even through I understand that they are
> conceptually analogous...

Yeah, [RaisesException] might not be a good name. We might want to replace
[RaisesException] with [CallWith=ExceptionState], which makes it clear that we
just want to call the binding callback with ExceptionState. Jens?

[Peter Beverloo, 2015-03-12 01:43:52]

On 2015/03/12 01:22:47, haraken wrote:
> > I see that the bindings do have an ExceptionState, and automatically convert
> > thrown exceptions to rejected promises.
> > 
> > Unless we use [RaisesException] though, this ExceptionState won't be
available
> > to the implementation. I would prefer to be explicit about "rejecting a
> promise"
> > versus "throwing an exception", even through I understand that they are
> > conceptually analogous...
> 
> Yeah, [RaisesException] might not be a good name. We might want to replace
> [RaisesException] with [CallWith=ExceptionState], which makes it clear that we
> just want to call the binding callback with ExceptionState. Jens?

I just found the section in the WebIDL specification that defines this behavior,
which is very well hidden in a massive algorithm :-(.

http://heycam.github.io/webidl/#es-operations

Having [CallWith=ExceptionState] would be great, and is much clearer about the
intent than [RaisesException] :-).

Thank you!

[yhirano, 2015-03-12 02:16:31]

On 2015/03/12 01:22:47, haraken wrote:
> > +haraken
> > 
> > Thank you, that looks much better already! I've got mostly nits, and one
> larger
> > comment.
> > 
> > Haraken-san, could you review the bindings and provide some guidance for the
> > following situation? (See ServiceWorkerRegistrationNotifications.cpp.)
> > 
> > - SWR.showNotification() returns a Promise.
> > - The C++ implementation uses SerializedScriptValueFactory to apply the
> > structured clone algorithm and create a wire string for the "data" property.
> > - We would like to catch this exception, and rather than throwing it,
> rejecting
> > the promise with it.
> > 
> > Would the following pattern be an acceptable use of TrackExceptionState?
> > 
> > =====
> > TrackExceptionState exceptionState;
> > SerializedScriptValueFactory::instance().create(...)
> > if (exceptionState.hadException())
> >   return exceptionState.reject(scriptState);
> > =====
> 
> I think this is fine, but I want to have yhirano take another look.
> 

ExceptionState::reject creates a promise based on ExceptionState::m_exception.
TrackExceptionState::throwX functions don't set it, so I don't think we can use
TrackExceptionState with ExceptionState::reject.

[haraken, 2015-03-12 02:22:18]

On 2015/03/12 02:16:31, yhirano wrote:
> On 2015/03/12 01:22:47, haraken wrote:
> > > +haraken
> > > 
> > > Thank you, that looks much better already! I've got mostly nits, and one
> > larger
> > > comment.
> > > 
> > > Haraken-san, could you review the bindings and provide some guidance for
the
> > > following situation? (See ServiceWorkerRegistrationNotifications.cpp.)
> > > 
> > > - SWR.showNotification() returns a Promise.
> > > - The C++ implementation uses SerializedScriptValueFactory to apply the
> > > structured clone algorithm and create a wire string for the "data"
property.
> > > - We would like to catch this exception, and rather than throwing it,
> > rejecting
> > > the promise with it.
> > > 
> > > Would the following pattern be an acceptable use of TrackExceptionState?
> > > 
> > > =====
> > > TrackExceptionState exceptionState;
> > > SerializedScriptValueFactory::instance().create(...)
> > > if (exceptionState.hadException())
> > >   return exceptionState.reject(scriptState);
> > > =====
> > 
> > I think this is fine, but I want to have yhirano take another look.
> > 
> 
> ExceptionState::reject creates a promise based on ExceptionState::m_exception.
> TrackExceptionState::throwX functions don't set it, so I don't think we can
use
> TrackExceptionState with ExceptionState::reject.

Ah, I was misreading. yhirano-san is right; TrackExceptionState wouldn't work
(and we want to deprecate TrackExceptionState).

Sorry, I'm getting confused. What's a reason we cannot pass an ExceptionState to
the binding callback and use it in ServiceWorkerRegistrationNotifications.cpp?
Or will [CallWith=ExceptionState] solve the problem?

[Peter Beverloo, 2015-03-12 02:28:50]

Thank you for your input!!

On 2015/03/12 02:22:18, haraken wrote:
> On 2015/03/12 02:16:31, yhirano wrote:
> > ExceptionState::reject creates a promise based on
ExceptionState::m_exception.
> > TrackExceptionState::throwX functions don't set it, so I don't think we can
> use
> > TrackExceptionState with ExceptionState::reject.
> 
> Ah, I was misreading. yhirano-san is right; TrackExceptionState wouldn't work
> (and we want to deprecate TrackExceptionState).
> 
> Sorry, I'm getting confused. What's a reason we cannot pass an ExceptionState
to
> the binding callback and use it in ServiceWorkerRegistrationNotifications.cpp?
> Or will [CallWith=ExceptionState] solve the problem?

Indeed, I missed that too.

Using [RaisesException] indeed is the right solution for now. I have added a
comment in ServiceWorkerRegistrationNotifications.cpp clarifying the needed
change.

Is the rest of the bindings portion of this CL correct?

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:48:
return ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(scriptState->isolate(), "No data property can
be serialized"));
Scrap the above :-).

Right now you're throwing a TypeError of your own, where the specification
defines that we have to re-throw exceptions caused by serializing the |data|
property.

You could do this as follows:
==========
data = SerializedScriptValueFactory...
if (exceptionState.hadException())
  return exceptionState.reject(scriptState);
==========

[haraken, 2015-03-12 02:32:25]

On 2015/03/12 02:28:50, Peter Beverloo wrote:
> Thank you for your input!!
> 
> On 2015/03/12 02:22:18, haraken wrote:
> > On 2015/03/12 02:16:31, yhirano wrote:
> > > ExceptionState::reject creates a promise based on
> ExceptionState::m_exception.
> > > TrackExceptionState::throwX functions don't set it, so I don't think we
can
> > use
> > > TrackExceptionState with ExceptionState::reject.
> > 
> > Ah, I was misreading. yhirano-san is right; TrackExceptionState wouldn't
work
> > (and we want to deprecate TrackExceptionState).
> > 
> > Sorry, I'm getting confused. What's a reason we cannot pass an
ExceptionState
> to
> > the binding callback and use it in
ServiceWorkerRegistrationNotifications.cpp?
> > Or will [CallWith=ExceptionState] solve the problem?
> 
> Indeed, I missed that too.
> 
> Using [RaisesException] indeed is the right solution for now. I have added a
> comment in ServiceWorkerRegistrationNotifications.cpp clarifying the needed
> change.
> 
> Is the rest of the bindings portion of this CL correct?

Why do you need a custom binding?

[Sanghyun Park, 2015-03-12 02:58:47]

Thank you for review :)

https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/n...
File LayoutTests/http/tests/notifications/notification-data-property.html
(right):

https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/notification-data-property.html:19: if
(value.constructor.toString().indexOf("Object") > -1)
On 2015/03/12 01:16:26, Peter Beverloo wrote:
> You can use some slightly nicer checks here;
> 
> if (Array.isArray(value))
>   // array
> else if (typeof value == 'object')
>   // object
> else
>   // anything else

This is better. I'll fix this.

https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/notification-data-property.html:42: var
notification = new Notification('Title', { data: function() { return 1; } });
On 2015/03/12 01:16:26, Peter Beverloo wrote:
> micro nit: indent +1 space

Done.

https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/notification-data-property.html:45: },
'Checks the data of several type property exposed on the Notification object.');
On 2015/03/12 01:16:26, Peter Beverloo wrote:
> micro nit: indent -4 spaces

Done.

https://codereview.chromium.org/993893002/diff/80001/Source/bindings/modules/...
File Source/bindings/modules/v8/custom/V8NotificationCustom.cpp (right):

https://codereview.chromium.org/993893002/diff/80001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/V8NotificationCustom.cpp:8: #include
"bindings/core/v8/SerializedScriptValue.h"
On 2015/03/12 01:16:26, Peter Beverloo wrote:
> nit: you can drop this include

I'll remove this.

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:169: WebNotificationData
notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
m_data.get());
On 2015/03/12 01:16:26, Peter Beverloo wrote:
> Why does the embedder have to know about the data for non-persistent
> notifications?
> 
> I think we should just pass a nullptr here.

I understand about your meant.
non-persistent do not need to send this data.
But I cannot use "nullptr", because [1] constructor's parameter is  reference
type. "const WebSerializedScriptValue&" 
[1] WebNotificationData(const WebString& title, Direction direction, const
WebString& lang, const WebString& body, const WebString& tag, const WebURL&
icon, bool silent, const WebSerializedScriptValue& data)

So, How about use WebNotificationData dummy instead of nullptr?

for example)
WebSerializedScriptValue data;
WebNotificationData notificationData(m_title, dir, m_lang, m_body, m_tag,
m_iconUrl, m_silent, data);

WDYT?

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/Notification.h:120: void
setData(PassRefPtr<SerializedScriptValue> data) { m_data = data; }
On 2015/03/12 01:16:26, Peter Beverloo wrote:
> nit: please place this under setSilent(). Generally, I try to follow the order
> of the IDL file. Same with the getter and the member variable.

I see. I'll move this under setSlient().

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:43: //
FIXME: Unifying these code paths at some point in the future
On 2015/03/12 01:16:26, Peter Beverloo wrote:
> micro nit: Unifying -> Unify. It would also be good to mention *which* code
> paths. What about:
> 
> // FIXME: Unify the code path here with the Notification.create() function.

I'll fix this.

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:48:
return ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(scriptState->isolate(), "No data property can
be serialized"));
On 2015/03/12 02:28:50, Peter Beverloo wrote:
> Scrap the above :-).
> 
> Right now you're throwing a TypeError of your own, where the specification
> defines that we have to re-throw exceptions caused by serializing the |data|
> property.
> 
> You could do this as follows:
> ==========
> data = SerializedScriptValueFactory...
> if (exceptionState.hadException())
>   return exceptionState.reject(scriptState);
> ==========

I see.
Thank you so much for finding solution.
I'll fix this.

https://codereview.chromium.org/993893002/diff/80001/Source/web/WebSerialized...
File Source/web/WebSerializedScriptValue.cpp (right):

https://codereview.chromium.org/993893002/diff/80001/Source/web/WebSerialized...
Source/web/WebSerializedScriptValue.cpp:85:
WebSerializedScriptValue::WebSerializedScriptValue(SerializedScriptValue* value)
On 2015/03/12 01:16:26, Peter Beverloo wrote:
> Why do we need this?
> 
> In ServiceWorkerRegistrationNotifications::showNotification() you're using
> RefPtr<>.release(), which gives us a PassRefPtr, and therefore should hit the
> normal constructor.

This function is for sending "data" in non-persistent notification.
but non-persistance do not use data in WebSerializedScriptValue anymore.
So, I'll remove this.

[Sanghyun Park, 2015-03-12 05:03:40]

On 2015/03/12 02:32:25, haraken wrote:
> On 2015/03/12 02:28:50, Peter Beverloo wrote:
> > Thank you for your input!!
> > 
> > On 2015/03/12 02:22:18, haraken wrote:
> > > On 2015/03/12 02:16:31, yhirano wrote:
> > > > ExceptionState::reject creates a promise based on
> > ExceptionState::m_exception.
> > > > TrackExceptionState::throwX functions don't set it, so I don't think we
> can
> > > use
> > > > TrackExceptionState with ExceptionState::reject.
> > > 
> > > Ah, I was misreading. yhirano-san is right; TrackExceptionState wouldn't
> work
> > > (and we want to deprecate TrackExceptionState).
> > > 
> > > Sorry, I'm getting confused. What's a reason we cannot pass an
> ExceptionState
> > to
> > > the binding callback and use it in
> ServiceWorkerRegistrationNotifications.cpp?
> > > Or will [CallWith=ExceptionState] solve the problem?
> > 
> > Indeed, I missed that too.
> > 
> > Using [RaisesException] indeed is the right solution for now. I have added a
> > comment in ServiceWorkerRegistrationNotifications.cpp clarifying the needed
> > change.
> > 
> > Is the rest of the bindings portion of this CL correct?
> 
> Why do you need a custom binding?

This notification.data is set to a structured clone of NotificationOptions's any
data property(ScriptValue),
so that notification's data is set by SerializedScriptValue in Notification
Object.

But SerializedScripValue cannot convert to V8vReturnValue directly, when
notification.data getter is called.
So I made V8NotificationCustom for converting like V8MessageEventCustom.
If there is other way, please let me know.

[haraken, 2015-03-12 05:07:18]

On 2015/03/12 05:03:40, sh919.park wrote:
> On 2015/03/12 02:32:25, haraken wrote:
> > On 2015/03/12 02:28:50, Peter Beverloo wrote:
> > > Thank you for your input!!
> > > 
> > > On 2015/03/12 02:22:18, haraken wrote:
> > > > On 2015/03/12 02:16:31, yhirano wrote:
> > > > > ExceptionState::reject creates a promise based on
> > > ExceptionState::m_exception.
> > > > > TrackExceptionState::throwX functions don't set it, so I don't think
we
> > can
> > > > use
> > > > > TrackExceptionState with ExceptionState::reject.
> > > > 
> > > > Ah, I was misreading. yhirano-san is right; TrackExceptionState wouldn't
> > work
> > > > (and we want to deprecate TrackExceptionState).
> > > > 
> > > > Sorry, I'm getting confused. What's a reason we cannot pass an
> > ExceptionState
> > > to
> > > > the binding callback and use it in
> > ServiceWorkerRegistrationNotifications.cpp?
> > > > Or will [CallWith=ExceptionState] solve the problem?
> > > 
> > > Indeed, I missed that too.
> > > 
> > > Using [RaisesException] indeed is the right solution for now. I have added
a
> > > comment in ServiceWorkerRegistrationNotifications.cpp clarifying the
needed
> > > change.
> > > 
> > > Is the rest of the bindings portion of this CL correct?
> > 
> > Why do you need a custom binding?
> 
> This notification.data is set to a structured clone of NotificationOptions's
any
> data property(ScriptValue),
> so that notification's data is set by SerializedScriptValue in Notification
> Object.
> 
> But SerializedScripValue cannot convert to V8vReturnValue directly, when
> notification.data getter is called.
> So I made V8NotificationCustom for converting like V8MessageEventCustom.
> If there is other way, please let me know.

Would you give me a diff between auto-generated code and your custom bindings? I
guess that's a common scenario for SerializedScriptValue, so it seems strange
that the auto-generated code doesn't support it.

[Sanghyun Park, 2015-03-12 05:19:39]

On 2015/03/12 05:07:18, haraken wrote:
> On 2015/03/12 05:03:40, sh919.park wrote:
> > On 2015/03/12 02:32:25, haraken wrote:
> > > On 2015/03/12 02:28:50, Peter Beverloo wrote:
> > > > Thank you for your input!!
> > > > 
> > > > On 2015/03/12 02:22:18, haraken wrote:
> > > > > On 2015/03/12 02:16:31, yhirano wrote:
> > > > > > ExceptionState::reject creates a promise based on
> > > > ExceptionState::m_exception.
> > > > > > TrackExceptionState::throwX functions don't set it, so I don't think
> we
> > > can
> > > > > use
> > > > > > TrackExceptionState with ExceptionState::reject.
> > > > > 
> > > > > Ah, I was misreading. yhirano-san is right; TrackExceptionState
wouldn't
> > > work
> > > > > (and we want to deprecate TrackExceptionState).
> > > > > 
> > > > > Sorry, I'm getting confused. What's a reason we cannot pass an
> > > ExceptionState
> > > > to
> > > > > the binding callback and use it in
> > > ServiceWorkerRegistrationNotifications.cpp?
> > > > > Or will [CallWith=ExceptionState] solve the problem?
> > > > 
> > > > Indeed, I missed that too.
> > > > 
> > > > Using [RaisesException] indeed is the right solution for now. I have
added
> a
> > > > comment in ServiceWorkerRegistrationNotifications.cpp clarifying the
> needed
> > > > change.
> > > > 
> > > > Is the rest of the bindings portion of this CL correct?
> > > 
> > > Why do you need a custom binding?
> > 
> > This notification.data is set to a structured clone of NotificationOptions's
> any
> > data property(ScriptValue),
> > so that notification's data is set by SerializedScriptValue in Notification
> > Object.
> > 
> > But SerializedScripValue cannot convert to V8vReturnValue directly, when
> > notification.data getter is called.
> > So I made V8NotificationCustom for converting like V8MessageEventCustom.
> > If there is other way, please let me know.
> 
> Would you give me a diff between auto-generated code and your custom bindings?
I
> guess that's a common scenario for SerializedScriptValue, so it seems strange
> that the auto-generated code doesn't support it.

original file : custom is not applied in Notification.idl
custom file : "Custom=Getter" is applied in Notification.idl

--- original.cpp	2015-03-12 14:15:17.470805090 +0900
+++ custom.cpp	2015-03-12 14:15:46.070805627 +0900
@@ -1,7 +1,12 @@
-static void dataAttributeGetter(const v8::PropertyCallbackInfo<v8::Value>&
info)
+void V8Notification::dataAttributeGetterCustom(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     v8::Local<v8::Object> holder = info.Holder();
     Notification* impl = V8Notification::toImpl(holder);
-    v8SetReturnValue(info, impl->data().v8Value());
+    if (!impl->data()) {
+        v8SetReturnValueNull(info);
+        return;
+    }
+
+    v8SetReturnValue(info, impl->data()->deserialize(info.GetIsolate()));
 }

[haraken, 2015-03-12 06:03:29]

On 2015/03/12 05:19:39, sh919.park wrote:
> On 2015/03/12 05:07:18, haraken wrote:
> > On 2015/03/12 05:03:40, sh919.park wrote:
> > > On 2015/03/12 02:32:25, haraken wrote:
> > > > On 2015/03/12 02:28:50, Peter Beverloo wrote:
> > > > > Thank you for your input!!
> > > > > 
> > > > > On 2015/03/12 02:22:18, haraken wrote:
> > > > > > On 2015/03/12 02:16:31, yhirano wrote:
> > > > > > > ExceptionState::reject creates a promise based on
> > > > > ExceptionState::m_exception.
> > > > > > > TrackExceptionState::throwX functions don't set it, so I don't
think
> > we
> > > > can
> > > > > > use
> > > > > > > TrackExceptionState with ExceptionState::reject.
> > > > > > 
> > > > > > Ah, I was misreading. yhirano-san is right; TrackExceptionState
> wouldn't
> > > > work
> > > > > > (and we want to deprecate TrackExceptionState).
> > > > > > 
> > > > > > Sorry, I'm getting confused. What's a reason we cannot pass an
> > > > ExceptionState
> > > > > to
> > > > > > the binding callback and use it in
> > > > ServiceWorkerRegistrationNotifications.cpp?
> > > > > > Or will [CallWith=ExceptionState] solve the problem?
> > > > > 
> > > > > Indeed, I missed that too.
> > > > > 
> > > > > Using [RaisesException] indeed is the right solution for now. I have
> added
> > a
> > > > > comment in ServiceWorkerRegistrationNotifications.cpp clarifying the
> > needed
> > > > > change.
> > > > > 
> > > > > Is the rest of the bindings portion of this CL correct?
> > > > 
> > > > Why do you need a custom binding?
> > > 
> > > This notification.data is set to a structured clone of
NotificationOptions's
> > any
> > > data property(ScriptValue),
> > > so that notification's data is set by SerializedScriptValue in
Notification
> > > Object.
> > > 
> > > But SerializedScripValue cannot convert to V8vReturnValue directly, when
> > > notification.data getter is called.
> > > So I made V8NotificationCustom for converting like V8MessageEventCustom.
> > > If there is other way, please let me know.
> > 
> > Would you give me a diff between auto-generated code and your custom
bindings?
> I
> > guess that's a common scenario for SerializedScriptValue, so it seems
strange
> > that the auto-generated code doesn't support it.
> 
> original file : custom is not applied in Notification.idl
> custom file : "Custom=Getter" is applied in Notification.idl
> 
> --- original.cpp	2015-03-12 14:15:17.470805090 +0900
> +++ custom.cpp	2015-03-12 14:15:46.070805627 +0900
> @@ -1,7 +1,12 @@
> -static void dataAttributeGetter(const v8::PropertyCallbackInfo<v8::Value>&
> info)
> +void V8Notification::dataAttributeGetterCustom(const
> v8::PropertyCallbackInfo<v8::Value>& info)
>  {
>      v8::Local<v8::Object> holder = info.Holder();
>      Notification* impl = V8Notification::toImpl(holder);
> -    v8SetReturnValue(info, impl->data().v8Value());
> +    if (!impl->data()) {
> +        v8SetReturnValueNull(info);
> +        return;
> +    }
> +
> +    v8SetReturnValue(info, impl->data()->deserialize(info.GetIsolate()));
>  }

Can we make impl->data() return a SerializedScriptValue (that can be null) and
just use the auto-generated code?

[Sanghyun Park, 2015-03-12 06:36:37]

On 2015/03/12 06:03:29, haraken wrote:
> On 2015/03/12 05:19:39, sh919.park wrote:
> > On 2015/03/12 05:07:18, haraken wrote:
> > > On 2015/03/12 05:03:40, sh919.park wrote:
> > > > On 2015/03/12 02:32:25, haraken wrote:
> > > > > On 2015/03/12 02:28:50, Peter Beverloo wrote:
> > > > > > Thank you for your input!!
> > > > > > 
> > > > > > On 2015/03/12 02:22:18, haraken wrote:
> > > > > > > On 2015/03/12 02:16:31, yhirano wrote:
> > > > > > > > ExceptionState::reject creates a promise based on
> > > > > > ExceptionState::m_exception.
> > > > > > > > TrackExceptionState::throwX functions don't set it, so I don't
> think
> > > we
> > > > > can
> > > > > > > use
> > > > > > > > TrackExceptionState with ExceptionState::reject.
> > > > > > > 
> > > > > > > Ah, I was misreading. yhirano-san is right; TrackExceptionState
> > wouldn't
> > > > > work
> > > > > > > (and we want to deprecate TrackExceptionState).
> > > > > > > 
> > > > > > > Sorry, I'm getting confused. What's a reason we cannot pass an
> > > > > ExceptionState
> > > > > > to
> > > > > > > the binding callback and use it in
> > > > > ServiceWorkerRegistrationNotifications.cpp?
> > > > > > > Or will [CallWith=ExceptionState] solve the problem?
> > > > > > 
> > > > > > Indeed, I missed that too.
> > > > > > 
> > > > > > Using [RaisesException] indeed is the right solution for now. I have
> > added
> > > a
> > > > > > comment in ServiceWorkerRegistrationNotifications.cpp clarifying the
> > > needed
> > > > > > change.
> > > > > > 
> > > > > > Is the rest of the bindings portion of this CL correct?
> > > > > 
> > > > > Why do you need a custom binding?
> > > > 
> > > > This notification.data is set to a structured clone of
> NotificationOptions's
> > > any
> > > > data property(ScriptValue),
> > > > so that notification's data is set by SerializedScriptValue in
> Notification
> > > > Object.
> > > > 
> > > > But SerializedScripValue cannot convert to V8vReturnValue directly, when
> > > > notification.data getter is called.
> > > > So I made V8NotificationCustom for converting like V8MessageEventCustom.
> > > > If there is other way, please let me know.
> > > 
> > > Would you give me a diff between auto-generated code and your custom
> bindings?
> > I
> > > guess that's a common scenario for SerializedScriptValue, so it seems
> strange
> > > that the auto-generated code doesn't support it.
> > 
> > original file : custom is not applied in Notification.idl
> > custom file : "Custom=Getter" is applied in Notification.idl
> > 
> > --- original.cpp	2015-03-12 14:15:17.470805090 +0900
> > +++ custom.cpp	2015-03-12 14:15:46.070805627 +0900
> > @@ -1,7 +1,12 @@
> > -static void dataAttributeGetter(const v8::PropertyCallbackInfo<v8::Value>&
> > info)
> > +void V8Notification::dataAttributeGetterCustom(const
> > v8::PropertyCallbackInfo<v8::Value>& info)
> >  {
> >      v8::Local<v8::Object> holder = info.Holder();
> >      Notification* impl = V8Notification::toImpl(holder);
> > -    v8SetReturnValue(info, impl->data().v8Value());
> > +    if (!impl->data()) {
> > +        v8SetReturnValueNull(info);
> > +        return;
> > +    }
> > +
> > +    v8SetReturnValue(info, impl->data()->deserialize(info.GetIsolate()));
> >  }
> 
> Can we make impl->data() return a SerializedScriptValue (that can be null) and
> just use the auto-generated code?

Yes.

but I bethink oneself, it's too strange.
I think that modify other way without using custom.
I can move these code in Notification.cpp. 

for example) 
ScriptValue Notification::data(ScriptState* scriptState) {
    if (!m_data)
        return ScriptValue::createNull(scriptState);

    return ScriptValue(scriptState,
m_data->deserialize(scriptState->isolate()));
}

is this better than using custom binding?

[Jens Widell, 2015-03-12 08:43:44]

On 2015/03/12 01:22:47, haraken wrote:
> Yeah, [RaisesException] might not be a good name. We might want to replace
> [RaisesException] with [CallWith=ExceptionState], which makes it clear that we
> just want to call the binding callback with ExceptionState. Jens?

Having [CallWith=ExceptionState] seems fine to me. It's clearer than
[RaisesException] (e.g. since lack of [RaisesException] doesn't mean a call
can't raise an exception).

OTOH, [RaisesException] is used a lot, and [CallWith=ExceptionState] is more
verbose.

[haraken, 2015-03-12 08:50:03]

On 2015/03/12 06:36:37, sh919.park wrote:
> On 2015/03/12 06:03:29, haraken wrote:
> > On 2015/03/12 05:19:39, sh919.park wrote:
> > > On 2015/03/12 05:07:18, haraken wrote:
> > > > On 2015/03/12 05:03:40, sh919.park wrote:
> > > > > On 2015/03/12 02:32:25, haraken wrote:
> > > > > > On 2015/03/12 02:28:50, Peter Beverloo wrote:
> > > > > > > Thank you for your input!!
> > > > > > > 
> > > > > > > On 2015/03/12 02:22:18, haraken wrote:
> > > > > > > > On 2015/03/12 02:16:31, yhirano wrote:
> > > > > > > > > ExceptionState::reject creates a promise based on
> > > > > > > ExceptionState::m_exception.
> > > > > > > > > TrackExceptionState::throwX functions don't set it, so I don't
> > think
> > > > we
> > > > > > can
> > > > > > > > use
> > > > > > > > > TrackExceptionState with ExceptionState::reject.
> > > > > > > > 
> > > > > > > > Ah, I was misreading. yhirano-san is right; TrackExceptionState
> > > wouldn't
> > > > > > work
> > > > > > > > (and we want to deprecate TrackExceptionState).
> > > > > > > > 
> > > > > > > > Sorry, I'm getting confused. What's a reason we cannot pass an
> > > > > > ExceptionState
> > > > > > > to
> > > > > > > > the binding callback and use it in
> > > > > > ServiceWorkerRegistrationNotifications.cpp?
> > > > > > > > Or will [CallWith=ExceptionState] solve the problem?
> > > > > > > 
> > > > > > > Indeed, I missed that too.
> > > > > > > 
> > > > > > > Using [RaisesException] indeed is the right solution for now. I
have
> > > added
> > > > a
> > > > > > > comment in ServiceWorkerRegistrationNotifications.cpp clarifying
the
> > > > needed
> > > > > > > change.
> > > > > > > 
> > > > > > > Is the rest of the bindings portion of this CL correct?
> > > > > > 
> > > > > > Why do you need a custom binding?
> > > > > 
> > > > > This notification.data is set to a structured clone of
> > NotificationOptions's
> > > > any
> > > > > data property(ScriptValue),
> > > > > so that notification's data is set by SerializedScriptValue in
> > Notification
> > > > > Object.
> > > > > 
> > > > > But SerializedScripValue cannot convert to V8vReturnValue directly,
when
> > > > > notification.data getter is called.
> > > > > So I made V8NotificationCustom for converting like
V8MessageEventCustom.
> > > > > If there is other way, please let me know.
> > > > 
> > > > Would you give me a diff between auto-generated code and your custom
> > bindings?
> > > I
> > > > guess that's a common scenario for SerializedScriptValue, so it seems
> > strange
> > > > that the auto-generated code doesn't support it.
> > > 
> > > original file : custom is not applied in Notification.idl
> > > custom file : "Custom=Getter" is applied in Notification.idl
> > > 
> > > --- original.cpp	2015-03-12 14:15:17.470805090 +0900
> > > +++ custom.cpp	2015-03-12 14:15:46.070805627 +0900
> > > @@ -1,7 +1,12 @@
> > > -static void dataAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>&
> > > info)
> > > +void V8Notification::dataAttributeGetterCustom(const
> > > v8::PropertyCallbackInfo<v8::Value>& info)
> > >  {
> > >      v8::Local<v8::Object> holder = info.Holder();
> > >      Notification* impl = V8Notification::toImpl(holder);
> > > -    v8SetReturnValue(info, impl->data().v8Value());
> > > +    if (!impl->data()) {
> > > +        v8SetReturnValueNull(info);
> > > +        return;
> > > +    }
> > > +
> > > +    v8SetReturnValue(info, impl->data()->deserialize(info.GetIsolate()));
> > >  }
> > 
> > Can we make impl->data() return a SerializedScriptValue (that can be null)
and
> > just use the auto-generated code?
> 
> Yes.
> 
> but I bethink oneself, it's too strange.
> I think that modify other way without using custom.
> I can move these code in Notification.cpp. 
> 
> for example) 
> ScriptValue Notification::data(ScriptState* scriptState) {
>     if (!m_data)
>         return ScriptValue::createNull(scriptState);
> 
>     return ScriptValue(scriptState,
> m_data->deserialize(scriptState->isolate()));
> }
> 
> is this better than using custom binding?

Yes, that's better. Custom bindings has been a source of security bugs, so we
want to avoid it as much as possible.

[haraken, 2015-03-12 08:50:52]

On 2015/03/12 08:43:44, Jens Widell wrote:
> On 2015/03/12 01:22:47, haraken wrote:
> > Yeah, [RaisesException] might not be a good name. We might want to replace
> > [RaisesException] with [CallWith=ExceptionState], which makes it clear that
we
> > just want to call the binding callback with ExceptionState. Jens?
> 
> Having [CallWith=ExceptionState] seems fine to me. It's clearer than
> [RaisesException] (e.g. since lack of [RaisesException] doesn't mean a call
> can't raise an exception).
> 
> OTOH, [RaisesException] is used a lot, and [CallWith=ExceptionState] is more
> verbose.

Just to confirm, but we can replace [RaisesException] with
[CallWith=ExceptionState], right (i.e., we don't need both)?

[Sanghyun Park, 2015-03-12 09:04:20]

On 2015/03/12 08:50:03, haraken wrote:
> On 2015/03/12 06:36:37, sh919.park wrote:
> > On 2015/03/12 06:03:29, haraken wrote:
> > > On 2015/03/12 05:19:39, sh919.park wrote:
> > > > On 2015/03/12 05:07:18, haraken wrote:
> > > > > On 2015/03/12 05:03:40, sh919.park wrote:
> > > > > > On 2015/03/12 02:32:25, haraken wrote:
> > > > > > > On 2015/03/12 02:28:50, Peter Beverloo wrote:
> > > > > > > > Thank you for your input!!
> > > > > > > > 
> > > > > > > > On 2015/03/12 02:22:18, haraken wrote:
> > > > > > > > > On 2015/03/12 02:16:31, yhirano wrote:
> > > > > > > > > > ExceptionState::reject creates a promise based on
> > > > > > > > ExceptionState::m_exception.
> > > > > > > > > > TrackExceptionState::throwX functions don't set it, so I
don't
> > > think
> > > > > we
> > > > > > > can
> > > > > > > > > use
> > > > > > > > > > TrackExceptionState with ExceptionState::reject.
> > > > > > > > > 
> > > > > > > > > Ah, I was misreading. yhirano-san is right;
TrackExceptionState
> > > > wouldn't
> > > > > > > work
> > > > > > > > > (and we want to deprecate TrackExceptionState).
> > > > > > > > > 
> > > > > > > > > Sorry, I'm getting confused. What's a reason we cannot pass an
> > > > > > > ExceptionState
> > > > > > > > to
> > > > > > > > > the binding callback and use it in
> > > > > > > ServiceWorkerRegistrationNotifications.cpp?
> > > > > > > > > Or will [CallWith=ExceptionState] solve the problem?
> > > > > > > > 
> > > > > > > > Indeed, I missed that too.
> > > > > > > > 
> > > > > > > > Using [RaisesException] indeed is the right solution for now. I
> have
> > > > added
> > > > > a
> > > > > > > > comment in ServiceWorkerRegistrationNotifications.cpp clarifying
> the
> > > > > needed
> > > > > > > > change.
> > > > > > > > 
> > > > > > > > Is the rest of the bindings portion of this CL correct?
> > > > > > > 
> > > > > > > Why do you need a custom binding?
> > > > > > 
> > > > > > This notification.data is set to a structured clone of
> > > NotificationOptions's
> > > > > any
> > > > > > data property(ScriptValue),
> > > > > > so that notification's data is set by SerializedScriptValue in
> > > Notification
> > > > > > Object.
> > > > > > 
> > > > > > But SerializedScripValue cannot convert to V8vReturnValue directly,
> when
> > > > > > notification.data getter is called.
> > > > > > So I made V8NotificationCustom for converting like
> V8MessageEventCustom.
> > > > > > If there is other way, please let me know.
> > > > > 
> > > > > Would you give me a diff between auto-generated code and your custom
> > > bindings?
> > > > I
> > > > > guess that's a common scenario for SerializedScriptValue, so it seems
> > > strange
> > > > > that the auto-generated code doesn't support it.
> > > > 
> > > > original file : custom is not applied in Notification.idl
> > > > custom file : "Custom=Getter" is applied in Notification.idl
> > > > 
> > > > --- original.cpp	2015-03-12 14:15:17.470805090 +0900
> > > > +++ custom.cpp	2015-03-12 14:15:46.070805627 +0900
> > > > @@ -1,7 +1,12 @@
> > > > -static void dataAttributeGetter(const
> v8::PropertyCallbackInfo<v8::Value>&
> > > > info)
> > > > +void V8Notification::dataAttributeGetterCustom(const
> > > > v8::PropertyCallbackInfo<v8::Value>& info)
> > > >  {
> > > >      v8::Local<v8::Object> holder = info.Holder();
> > > >      Notification* impl = V8Notification::toImpl(holder);
> > > > -    v8SetReturnValue(info, impl->data().v8Value());
> > > > +    if (!impl->data()) {
> > > > +        v8SetReturnValueNull(info);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    v8SetReturnValue(info,
impl->data()->deserialize(info.GetIsolate()));
> > > >  }
> > > 
> > > Can we make impl->data() return a SerializedScriptValue (that can be null)
> and
> > > just use the auto-generated code?
> > 
> > Yes.
> > 
> > but I bethink oneself, it's too strange.
> > I think that modify other way without using custom.
> > I can move these code in Notification.cpp. 
> > 
> > for example) 
> > ScriptValue Notification::data(ScriptState* scriptState) {
> >     if (!m_data)
> >         return ScriptValue::createNull(scriptState);
> > 
> >     return ScriptValue(scriptState,
> > m_data->deserialize(scriptState->isolate()));
> > }
> > 
> > is this better than using custom binding?
> 
> Yes, that's better. Custom bindings has been a source of security bugs, so we
> want to avoid it as much as possible.

Okay. 
Thank you so much. 
I'll fix this.

[Jens Widell, 2015-03-12 09:10:50]

On 2015/03/12 08:50:52, haraken wrote:
> On 2015/03/12 08:43:44, Jens Widell wrote:
> > On 2015/03/12 01:22:47, haraken wrote:
> > > Yeah, [RaisesException] might not be a good name. We might want to replace
> > > [RaisesException] with [CallWith=ExceptionState], which makes it clear
that
> we
> > > just want to call the binding callback with ExceptionState. Jens?
> > 
> > Having [CallWith=ExceptionState] seems fine to me. It's clearer than
> > [RaisesException] (e.g. since lack of [RaisesException] doesn't mean a call
> > can't raise an exception).
> > 
> > OTOH, [RaisesException] is used a lot, and [CallWith=ExceptionState] is more
> > verbose.
> 
> Just to confirm, but we can replace [RaisesException] with
> [CallWith=ExceptionState], right (i.e., we don't need both)?

I see no reason to have both.

[Sanghyun Park, 2015-03-12 11:52:58]

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:169: WebNotificationData
notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
m_data.get());
On 2015/03/12 02:58:47, sh919.park wrote:
> On 2015/03/12 01:16:26, Peter Beverloo wrote:
> > Why does the embedder have to know about the data for non-persistent
> > notifications?
> > 
> > I think we should just pass a nullptr here.
> 
> I understand about your meant.
> non-persistent do not need to send this data.
> But I cannot use "nullptr", because [1] constructor's parameter is  reference
> type. "const WebSerializedScriptValue&" 
> [1] WebNotificationData(const WebString& title, Direction direction, const
> WebString& lang, const WebString& body, const WebString& tag, const WebURL&
> icon, bool silent, const WebSerializedScriptValue& data)
> 
> So, How about use WebNotificationData dummy instead of nullptr?
> 
> for example)
> WebSerializedScriptValue data;
> WebNotificationData notificationData(m_title, dir, m_lang, m_body, m_tag,
> m_iconUrl, m_silent, data);
> 
> WDYT?

When I think of that, above suggested way is not good.
In my opinion, to divide WebNotificationData's constructor from non-persistant
and persistant is better.
What do you think about this?

[Sanghyun Park, 2015-03-12 12:12:43]

Patchset #3 (id:100001) has been deleted

[Sanghyun Park, 2015-03-12 12:39:36]

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/993893002/diff/80001/Source/modules/notificat...
Source/modules/notifications/Notification.cpp:169: WebNotificationData
notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
m_data.get());
On 2015/03/12 11:52:58, sh919.park wrote:
> On 2015/03/12 02:58:47, sh919.park wrote:
> > On 2015/03/12 01:16:26, Peter Beverloo wrote:
> > > Why does the embedder have to know about the data for non-persistent
> > > notifications?
> > > 
> > > I think we should just pass a nullptr here.
> > 
> > I understand about your meant.
> > non-persistent do not need to send this data.
> > But I cannot use "nullptr", because [1] constructor's parameter is 
reference
> > type. "const WebSerializedScriptValue&" 
> > [1] WebNotificationData(const WebString& title, Direction direction, const
> > WebString& lang, const WebString& body, const WebString& tag, const WebURL&
> > icon, bool silent, const WebSerializedScriptValue& data)
> > 
> > So, How about use WebNotificationData dummy instead of nullptr?
> > 
> > for example)
> > WebSerializedScriptValue data;
> > WebNotificationData notificationData(m_title, dir, m_lang, m_body, m_tag,
> > m_iconUrl, m_silent, data);
> > 
> > WDYT?
> 
> When I think of that, above suggested way is not good.
> In my opinion, to divide WebNotificationData's constructor from non-persistant
> and persistant is better.
> What do you think about this?

Oops. I was wrong. we can use PassRefPtr<SerializedScriptValue>(nullptr).;; I'll
fix this.

[Peter Beverloo, 2015-03-12 20:30:46]

Mostly lg, but we have a layering violation we need to deal with first. You can
discard the comment in Notification.cpp if you choose to follow my suggestion of
splitting this patch up.

Please also add yourself to the AUTHORS file in Chromium.

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

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:171: // No send data property in
non-persistent notification.
No -> Don't.

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:172: WebNotificationData
notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
PassRefPtr<SerializedScriptValue>(nullptr));
The reason that this doesn't work is that the WebNotificationData constructor
takes a *Web*SerializedScriptValue, not a SerializedScriptValue.

It would be clearer if you'd write this like:

WebSerializedScriptValue emptySerializedScriptValue;
WebNotificationData notificationData(..., emptySerializedScriptValue);

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.h:128: void
setSerializedData(PassRefPtr<SerializedScriptValue> data) {
m_dataAsSerializedScriptValue = data; }
nit: the setter's name should match the member it sets.

Renaming m_dataAsSerializedScriptValue to m_serializedData SGTM.

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

https://codereview.chromium.org/993893002/diff/120001/public/platform/modules...
public/platform/modules/notifications/WebNotificationData.h:10: #include
"public/web/WebSerializedScriptValue.h"
public/platform/ (where this files live in) cannot depend on public/web/ :-(.

There's a FIXME in WebSerializedScriptValue about it being in platform.

Because that's another larger issue, perhaps you can consider reverting the
changes in ServiceWorkerRegistrationNotifications.* and WebNotificationData.h in
this patch, and landing the implementation for non-persistent notifications
first.

Then, as a follow-up, pursue moving WebSerializedScriptValue to platform and
updating this code path to pass a WebSerializedScriptValue to the embedder?

[Sanghyun Park, 2015-03-13 02:57:07]

Thank you for review. 
I have one doubt about layering (WebNotificationData.h).
Please refer my comment in WebNotificationData.h
If my understanding is wrong, I'll split the this patch.

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

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:171: // No send data property in
non-persistent notification.
On 2015/03/12 20:30:46, Peter Beverloo wrote:
> No -> Don't.

Okay, I'll fix

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:172: WebNotificationData
notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
PassRefPtr<SerializedScriptValue>(nullptr));
On 2015/03/12 20:30:45, Peter Beverloo wrote:
> The reason that this doesn't work is that the WebNotificationData constructor
> takes a *Web*SerializedScriptValue, not a SerializedScriptValue.
> 
> It would be clearer if you'd write this like:
> 
> WebSerializedScriptValue emptySerializedScriptValue;
> WebNotificationData notificationData(..., emptySerializedScriptValue);

Okay, I'll fix this also.:)

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.h:128: void
setSerializedData(PassRefPtr<SerializedScriptValue> data) {
m_dataAsSerializedScriptValue = data; }
On 2015/03/12 20:30:46, Peter Beverloo wrote:
> nit: the setter's name should match the member it sets.
> 
> Renaming m_dataAsSerializedScriptValue to m_serializedData SGTM.

I'll change this name

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

https://codereview.chromium.org/993893002/diff/120001/public/platform/modules...
public/platform/modules/notifications/WebNotificationData.h:10: #include
"public/web/WebSerializedScriptValue.h"
On 2015/03/12 20:30:46, Peter Beverloo wrote:
> public/platform/ (where this files live in) cannot depend on public/web/ :-(.
> 
> There's a FIXME in WebSerializedScriptValue about it being in platform.
> 
> Because that's another larger issue, perhaps you can consider reverting the
> changes in ServiceWorkerRegistrationNotifications.* and WebNotificationData.h
in
> this patch, and landing the implementation for non-persistent notifications
> first.
> 
> Then, as a follow-up, pursue moving WebSerializedScriptValue to platform and
> updating this code path to pass a WebSerializedScriptValue to the embedder?

I refer the this document about this.
[1] http://www.chromium.org/blink/public-c-api
"shows the overall dependency layers. Things higher up in the diagram depend
only on things below them." is described in this document.
According to the above, "Blink public platform API" can depend on  "Blink public
web API".

If my understanding is wrong, please let me know..

[Sanghyun Park, 2015-03-13 04:13:05]

On 2015/03/12 20:30:46, Peter Beverloo wrote:
> Mostly lg, but we have a layering violation we need to deal with first. You
can
> discard the comment in Notification.cpp if you choose to follow my suggestion
of
> splitting this patch up.
> 
> Please also add yourself to the AUTHORS file in Chromium.

This is patch for blink git.
But I should make Chromium patch for adding to the AUTHORS.
Do you meant that make patch in chromium for this(add authors)?

> 
>
https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
> File Source/modules/notifications/Notification.cpp (right):
> 
>
https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
> Source/modules/notifications/Notification.cpp:171: // No send data property in
> non-persistent notification.
> No -> Don't.
> 
>
https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
> Source/modules/notifications/Notification.cpp:172: WebNotificationData
> notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent,
> PassRefPtr<SerializedScriptValue>(nullptr));
> The reason that this doesn't work is that the WebNotificationData constructor
> takes a *Web*SerializedScriptValue, not a SerializedScriptValue.
> 
> It would be clearer if you'd write this like:
> 
> WebSerializedScriptValue emptySerializedScriptValue;
> WebNotificationData notificationData(..., emptySerializedScriptValue);
> 
>
https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
> File Source/modules/notifications/Notification.h (right):
> 
>
https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
> Source/modules/notifications/Notification.h:128: void
> setSerializedData(PassRefPtr<SerializedScriptValue> data) {
> m_dataAsSerializedScriptValue = data; }
> nit: the setter's name should match the member it sets.
> 
> Renaming m_dataAsSerializedScriptValue to m_serializedData SGTM.
> 
>
https://codereview.chromium.org/993893002/diff/120001/public/platform/modules...
> File public/platform/modules/notifications/WebNotificationData.h (right):
> 
>
https://codereview.chromium.org/993893002/diff/120001/public/platform/modules...
> public/platform/modules/notifications/WebNotificationData.h:10: #include
> "public/web/WebSerializedScriptValue.h"
> public/platform/ (where this files live in) cannot depend on public/web/ :-(.
> 
> There's a FIXME in WebSerializedScriptValue about it being in platform.
> 
> Because that's another larger issue, perhaps you can consider reverting the
> changes in ServiceWorkerRegistrationNotifications.* and WebNotificationData.h
in
> this patch, and landing the implementation for non-persistent notifications
> first.
> 
> Then, as a follow-up, pursue moving WebSerializedScriptValue to platform and
> updating this code path to pass a WebSerializedScriptValue to the embedder?

[vivekg, 2015-03-13 04:21:48]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-03-13 04:21:49]

https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/notification-data-property.html
(right):

https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/notification-data-property.html:1:
<!doctype html>
nit: We prefer <!DOCTYPE html>

https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/notification-data-property.html:21: else if
(typeof value == 'object')
nit: replace '==' => '===' i.e. typeof value === 'object'

https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/notification-data-property.html:34: var
cars = new Array("Saab", "Volvo", "BMW");
Prefer using ' over " in all applicable javascript code snippets as can be seen
in the google code style guide [1]

[1]
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.h:34: #include
"bindings/core/v8/SerializedScriptValue.h"
nit: SerializedScriptValue can be forward declared.

[Sanghyun Park, 2015-03-13 05:03:08]

Dear Vivekg, Thanks for review.

https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/notification-data-property.html
(right):

https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/notification-data-property.html:1:
<!doctype html>
On 2015/03/13 04:21:49, vivekg_ wrote:
> nit: We prefer <!DOCTYPE html>

Okay I'll fix this.

https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/notification-data-property.html:21: else if
(typeof value == 'object')
On 2015/03/13 04:21:49, vivekg_ wrote:
> nit: replace '==' => '===' i.e. typeof value === 'object'

ditto

https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/notification-data-property.html:34: var
cars = new Array("Saab", "Volvo", "BMW");
On 2015/03/13 04:21:49, vivekg_ wrote:
> Prefer using ' over " in all applicable javascript code snippets as can be
seen
> in the google code style guide [1]
> 
> [1]
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

I see. I'll modify double-quotes to single-quotes about string.

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.h:34: #include
"bindings/core/v8/SerializedScriptValue.h"
On 2015/03/13 04:21:49, vivekg_ wrote:
> nit: SerializedScriptValue can be forward declared.

This header is required, becuase build break is occurred in "void
setSerializedData(PassRefPtr<SerializedScriptValue> data) {
m_dataAsSerializedScriptValue = data; }"
Because 'WTF::RefPtr<T>& WTF::RefPtr<T>::operator=(const WTF::PassRefPtr<T>&)
operator need destructor of object.

If you think that forward declared is better, I'll move this function's
definition to source file.

[vivekg, 2015-03-13 07:13:15]

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
Source/modules/notifications/Notification.h:34: #include
"bindings/core/v8/SerializedScriptValue.h"
On 2015/03/13 05:03:08, sh919.park wrote:
> On 2015/03/13 04:21:49, vivekg_ wrote:
> > nit: SerializedScriptValue can be forward declared.
> 
> This header is required, becuase build break is occurred in "void
> setSerializedData(PassRefPtr<SerializedScriptValue> data) {
> m_dataAsSerializedScriptValue = data; }"
> Because 'WTF::RefPtr<T>& WTF::RefPtr<T>::operator=(const WTF::PassRefPtr<T>&)
> operator need destructor of object.
> 
> If you think that forward declared is better, I'll move this function's
> definition to source file.

No that won't be necessary. Let it be as is because this is a trivial setter no
need to move to cpp.

[Sanghyun Park, 2015-03-13 07:40:08]

On 2015/03/13 07:13:15, vivekg_ wrote:
>
https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
> File Source/modules/notifications/Notification.h (right):
> 
>
https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifica...
> Source/modules/notifications/Notification.h:34: #include
> "bindings/core/v8/SerializedScriptValue.h"
> On 2015/03/13 05:03:08, sh919.park wrote:
> > On 2015/03/13 04:21:49, vivekg_ wrote:
> > > nit: SerializedScriptValue can be forward declared.
> > 
> > This header is required, becuase build break is occurred in "void
> > setSerializedData(PassRefPtr<SerializedScriptValue> data) {
> > m_dataAsSerializedScriptValue = data; }"
> > Because 'WTF::RefPtr<T>& WTF::RefPtr<T>::operator=(const
WTF::PassRefPtr<T>&)
> > operator need destructor of object.
> > 
> > If you think that forward declared is better, I'll move this function's
> > definition to source file.
> 
> No that won't be necessary. Let it be as is because this is a trivial setter
no
> need to move to cpp.

I see, I'll keep this. :)

[Sanghyun Park, 2015-03-13 07:58:08]

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

https://codereview.chromium.org/993893002/diff/120001/public/platform/modules...
public/platform/modules/notifications/WebNotificationData.h:10: #include
"public/web/WebSerializedScriptValue.h"
On 2015/03/13 02:57:07, sh919.park wrote:
> On 2015/03/12 20:30:46, Peter Beverloo wrote:
> > public/platform/ (where this files live in) cannot depend on public/web/
:-(.
> > 
> > There's a FIXME in WebSerializedScriptValue about it being in platform.
> > 
> > Because that's another larger issue, perhaps you can consider reverting the
> > changes in ServiceWorkerRegistrationNotifications.* and
WebNotificationData.h
> in
> > this patch, and landing the implementation for non-persistent notifications
> > first.
> > 
> > Then, as a follow-up, pursue moving WebSerializedScriptValue to platform and
> > updating this code path to pass a WebSerializedScriptValue to the embedder?
> 
> I refer the this document about this.
> [1] http://www.chromium.org/blink/public-c-api
> "shows the overall dependency layers. Things higher up in the diagram depend
> only on things below them." is described in this document.
> According to the above, "Blink public platform API" can depend on  "Blink
public
> web API".
> 
> If my understanding is wrong, please let me know..
> 
> 

Oops. I'm misunderstanding, At first, I'll split the patch.
and I'll consider about this.

[Peter Beverloo, 2015-03-13 12:12:07]

notifications lgtm, thank you!!

You'll need another OWNER to approve RuntimeEnabledFeatures.in.

https://codereview.chromium.org/993893002/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/notification-data-property.html
(right):

https://codereview.chromium.org/993893002/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/notification-data-property.html:13: if
(window.testRunner)
nit: this isn't really necessary. The test would work without permission too.

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

https://codereview.chromium.org/993893002/diff/140001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:283: ScriptValue
Notification::data(ScriptState* scriptState)
nit: can this be const?

https://codereview.chromium.org/993893002/diff/140001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/140001/Source/modules/notifica...
Source/modules/notifications/Notification.h:92: SerializedScriptValue*
serializedData() const { return m_serializedData.get(); }
nit: I'd prefer to have serializedData() separate (perhaps around line 96?)
since it's not a IDL getter.

[haraken, 2015-03-13 12:36:34]

LGTM

[Sanghyun Park, 2015-03-13 17:47:31]

Thank you so much.
I cannot complete this patch without your help. :)
I'll upload patch for commit after fixing about your comment and rebase.

https://codereview.chromium.org/993893002/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/notification-data-property.html
(right):

https://codereview.chromium.org/993893002/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/notification-data-property.html:13: if
(window.testRunner)
On 2015/03/13 12:12:07, Peter Beverloo wrote:
> nit: this isn't really necessary. The test would work without permission too.

 I'll remove this.

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

https://codereview.chromium.org/993893002/diff/140001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:283: ScriptValue
Notification::data(ScriptState* scriptState)
On 2015/03/13 12:12:07, Peter Beverloo wrote:
> nit: can this be const?

I'll add this keyword

https://codereview.chromium.org/993893002/diff/140001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/993893002/diff/140001/Source/modules/notifica...
Source/modules/notifications/Notification.h:92: SerializedScriptValue*
serializedData() const { return m_serializedData.get(); }
On 2015/03/13 12:12:07, Peter Beverloo wrote:
> nit: I'd prefer to have serializedData() separate (perhaps around line 96?)
> since it's not a IDL getter.

I'll move this line to under "KURL iconURL() const" line.

[Sanghyun Park, 2015-03-13 18:57:35]

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

[Sanghyun Park, 2015-03-13 18:57:36]

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

[commit-bot: I haz the power, 2015-03-13 18:57:51]

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

[Sanghyun Park, 2015-03-14 02:37:07]

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

[Sanghyun Park, 2015-03-14 03:22:37]

Dear Peter.

Layouttest had fails.
So I fix about these.
Could you review about this?

And could you give permission of  try bot build to me?

[Peter Beverloo, 2015-03-14 15:20:21]

On 2015/03/14 03:22:37, Sanghyun Park wrote:
> Dear Peter.
> 
> Layouttest had fails.
> So I fix about these.
> Could you review about this?
> 
> And could you give permission of  try bot build to me?

Still lgtm!

[Peter Beverloo, 2015-03-14 15:20:32]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-14 15:20:32]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/993893002/#ps180001
(title: "Fix layout test failures.")

[commit-bot: I haz the power, 2015-03-14 15:20:43]

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

[commit-bot: I haz the power, 2015-03-14 16:45:06]

Committed patchset #6 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191882