Implement support for notification.vibrate

chrome/browser/notifications/platform_notification_service_impl.cc

Index: chrome/browser/notifications/platform_notification_service_impl.cc
diff --git a/chrome/browser/notifications/platform_notification_service_impl.cc b/chrome/browser/notifications/platform_notification_service_impl.cc
index a2127fe92a2ad24aec9d16f314c23036f85f60ff..7a2d959191430d017b63092a8b8a430cd9c1edf1 100644
--- a/chrome/browser/notifications/platform_notification_service_impl.cc
+++ b/chrome/browser/notifications/platform_notification_service_impl.cc
@@ -49,6 +49,9 @@ using message_center::NotifierId;
 
 namespace {
 
+const int kMinimumVibrationDurationMs = 1;  // 1 millisecond
+const int kMaximumVibrationDurationMs = 10000;  // 10 seconds
+
 // Callback to provide when deleting the data associated with persistent Web
 // Notifications from the notification database.
 void OnPersistentNotificationDataDeleted(bool success) {
@@ -323,6 +326,18 @@ Notification PlatformNotificationServiceImpl::CreateNotificationFromData(
       display_source, notification_data.tag, delegate);
 
   notification.set_context_message(display_source);
+
+  // Though the Blink implementation already sanitizes vibration times, don't

[Peter Beverloo, 2015/05/05 20:42:44]

nit: Theoretically this doesn't have to come from Blink. What about:

// Make sure that the vibration values are within reasonable bounds.

[Peter Beverloo, 2015/05/05 20:42:44]

optional nit: You could add a unit test for this in
PlatformNotificationServiceTest.

[Sanghyun Park, 2015/05/06 14:53:25]

Done.

+  // trust any values passed from the client.

[dcheng, 2015/05/05 21:56:33]

I'd prefer to see this sanitization moved to the IPC message handler.

[Sanghyun Park, 2015/05/06 14:53:25]

If logic to use vibrate before this sanitization is added in the future in
NotificationMessageFilter(IPC handling), dcheng's suggestion is  better than
this.

But I think it will not happen, because This function is handling the
notification's data.
And DisplayNotification(use this CreateNotificationFromData function) is called
in IPC message handler function.

@Peter.
WDYT?

[Peter Beverloo, 2015/05/06 15:35:17]

I agree with Daniel's suggestion.

Daniel prefers to see the validation in NotificationMessageFilter because the
primary reason it's there is that we don't trust data coming from the renderer -
other consumers in the browser process are (generally) trusted.

It's most common to do this validation in the message filter because that's
(roughly) where the data comes in, which makes it much easier to validate that
the data is *always* safe to use, rather than only when actually displaying the
notification.

To illustrate this - we would currently store an invalid pattern in the
notification database, and deserialize this again to the developer when
re-creating the notification. When the validation/sanitation would be done early
in the message filter, we would store the sanitized pattern in the database
instead.

[Sanghyun Park, 2015/05/06 17:33:32]

Okay, I see.
I upload patch about this.

Dear Daniel and Peter thanks for your guide and detail explanation :)

+  std::vector<int> vibration_pattern;
+  for (int pattern_entry : notification_data.vibration_pattern) {
+    pattern_entry = std::min(kMaximumVibrationDurationMs,
+        std::max(kMinimumVibrationDurationMs, pattern_entry));
+
+    vibration_pattern.push_back(pattern_entry);
+  }
+  notification.set_vibration_pattern(vibration_pattern);
+
   notification.set_silent(notification_data.silent);
 
   // Web Notifications do not timeout.