Add the data attribute to the Notification object

Source/modules/notifications/Notification.cpp

Index: Source/modules/notifications/Notification.cpp
diff --git a/Source/modules/notifications/Notification.cpp b/Source/modules/notifications/Notification.cpp
index 0a8cf69dae08118ca53c7ea87a16e0f59bf85c07..b916441eefa1c2d9f0e295c29693320e59307d55 100644
--- a/Source/modules/notifications/Notification.cpp
+++ b/Source/modules/notifications/Notification.cpp
@@ -1,3 +1,4 @@
+

[Peter Beverloo, 2015/03/10 23:34:26]

nit: no blank line

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

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

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
@@ -31,7 +32,9 @@
 #include "config.h"
 #include "modules/notifications/Notification.h"
 
+#include "bindings/core/v8/ScriptValue.h"
 #include "bindings/core/v8/ScriptWrappable.h"
+#include "bindings/core/v8/SerializedScriptValueFactory.h"
 #include "core/dom/Document.h"
 #include "core/dom/ExecutionContext.h"
 #include "core/dom/ExecutionContextTask.h"
@@ -85,6 +88,11 @@ Notification* Notification::create(ExecutionContext* context, const String& titl
             notification->setIconUrl(iconUrl);
     }
 
+    if (options.hasData()) {
+        notification->setData(SerializedScriptValueFactory::instance().create(options.data(), 0, exceptionState, options.data().isolate()));

[Peter Beverloo, 2015/03/10 23:34:26]

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?

[Peter Beverloo, 2015/03/10 23:34:26]

nit: s/0/nullptr/

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

I'll fix this.

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

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?
Thanks,  I'll move this code to 77 line.

+        notification->data()->registerMemoryAllocatedWithCurrentScriptContext();

[Peter Beverloo, 2015/03/10 23:34:26]

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.

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

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

+    }
+
     String insecureOriginMessage;
     UseCounter::Feature feature = context->securityOrigin()->canAccessFeatureRequiringSecureOrigin(insecureOriginMessage)
         ? UseCounter::NotificationSecureOrigin : UseCounter::NotificationInsecureOrigin;
@@ -109,6 +117,11 @@ Notification* Notification::create(ExecutionContext* context, const String& pers
     if (!data.icon.isEmpty())
         notification->setIconUrl(data.icon);
 
+    if (!data.data.isNull()) {
+        notification->setData(data.data);
+        notification->data()->registerMemoryAllocatedWithCurrentScriptContext();
+    }
+
     notification->setState(NotificationStateShowing);
     notification->suspendIfNeeded();
     return notification;
@@ -150,7 +163,7 @@ void Notification::show()
 
     // FIXME: Do CSP checks on the associated notification icon.
     WebNotificationData::Direction dir = m_dir == "rtl" ? WebNotificationData::DirectionRightToLeft : WebNotificationData::DirectionLeftToRight;
-    WebNotificationData notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent);
+    WebNotificationData notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent, m_data.release());

[Peter Beverloo, 2015/03/10 23:34:26]

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
};
=========================

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

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

     notificationManager()->show(WebSerializedOrigin(*origin), notificationData, this);
 
     m_state = NotificationStateShowing;