Add the data attribute to the Notification object

Source/modules/notifications/Notification.cpp

+

[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.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 13 matching lines @@
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #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"
 #include "core/dom/ScopedWindowFocusAllowedIndicator.h"
 #include "core/events/Event.h"
 #include "core/frame/UseCounter.h"
 #include "modules/notifications/NotificationOptions.h"
 #include "modules/notifications/NotificationPermissionClient.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/UserGestureIndicator.h"
@@ skipping 33 matching lines @@
     notification->setTag(options.tag());
     notification->setLang(options.lang());
     notification->setDir(options.dir());
     notification->setSilent(options.silent());
     if (options.hasIcon()) {
         KURL iconUrl = options.icon().isEmpty() ? KURL() : context->completeURL(options.icon());
         if (!iconUrl.isEmpty() && iconUrl.isValid())
             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;
     UseCounter::count(context, feature);
 
     notification->scheduleShow();
     notification->suspendIfNeeded();
     return notification;
 }
 
 Notification* Notification::create(ExecutionContext* context, const String& persistentId, const WebNotificationData& data)
 {
     Notification* notification = new Notification(data.title, context);
 
     notification->setPersistentId(persistentId);
     notification->setDir(data.direction == WebNotificationData::DirectionLeftToRight ? "ltr" : "rtl");
     notification->setLang(data.lang);
     notification->setBody(data.body);
     notification->setTag(data.tag);
     notification->setSilent(data.silent);
 
     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;
 }
 
 Notification::Notification(const String& title, ExecutionContext* context)
     : ActiveDOMObject(context)
     , m_title(title)
     , m_dir("auto")
     , m_silent(false)
@@ skipping 21 matching lines @@
     if (Notification::checkPermission(executionContext()) != WebNotificationPermissionAllowed) {
         dispatchErrorEvent();
         return;
     }
 
     SecurityOrigin* origin = executionContext()->securityOrigin();
     ASSERT(origin);
 
     // 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;
 }
 
 void Notification::close()
 {
     if (m_state != NotificationStateShowing)
         return;
 
@@ skipping 104 matching lines @@
     return m_state == NotificationStateShowing || m_asyncRunner.isActive();
 }
 
 DEFINE_TRACE(Notification)
 {
     RefCountedGarbageCollectedEventTargetWithInlineData<Notification>::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
 } // namespace blink