Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1054573002:
Implement support for notification.vibrate (Closed)
|
Created:
5 years, 8 months ago by Sanghyun Park Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-notifications_chromium.org, peter+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement support for notification.vibrate
This patch implements support for notification.vibrate, which allow a
Web developer to set the "vibrate" key when creating a notification,
that will set up a vibration.
This operation depends on the device capability.
Currently this only works on Android.
The Web Notification specification:
https://notifications.spec.whatwg.org/#dom-notificationoptions-vibrate
BUG=442132
Committed: https://crrev.com/1593bf8e67c4e99be94a85bec217463961a9c07c
Cr-Commit-Position: refs/heads/master@{#329082}
Patch Set 1 : #
Total comments: 51
Patch Set 2 : #
Total comments: 16
Patch Set 3 : #
Total comments: 19
Patch Set 4 : #
Total comments: 7
Patch Set 5 : #
Total comments: 9
Patch Set 6 : #
Total comments: 8
Patch Set 7 : #
Total comments: 8
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Messages
Total messages: 49 (21 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
sh919.park@samsung.com changed reviewers: + avi@chromium.org, dcheng@chromium.org, peter@chromium.org
dcheng@chromium.org: Please review changes in avi@chromium.org: Please review changes in peter@chromium.org: Please review changes in
https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... File content/browser/notifications/notification_database_data.proto (right): https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:24: // Next tag: 8 ? https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:39: optional bytes data = 9; I am definitely no expert in protos, but from my fuzzy recollections, I thought you never renumbered items. Where is this proto used? https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... File content/public/common/platform_notification_data.h (right): https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... content/public/common/platform_notification_data.h:54: // Vibrate pettern for notification. typo: pattern https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... content/public/common/platform_notification_data.h:55: std::vector<unsigned> vibrate; Use of unsigned ints is prohibited by the style guide. Use int. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... ui/message_center/notification.h:53: std::vector<unsigned> vibrate; ditto
peter@chromium.org changed reviewers: + timvolodine@chromium.org
Hi Sanghyun, Here's my review. Please note that I'm out of office on Thursday and Friday, so further replies may be a bit delayed. In the future, could I ask you to start with the feature reviewer (me, in this case, +tim) before adding OWNERS? That'll make the process a little bit smoother - generally the owners are really busy people. Thanks! https://codereview.chromium.org/1054573002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:306: * @param vibrate Vibrate pettern for notification. Pass in an array of ints that are the Can we keep this vague ("Vibration pattern for the Notification.") and refer to the Android documentation for the Notification object instead? @see http://developer.android.com/reference/android/app/Notification.html https://codereview.chromium.org/1054573002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:354: // If vibrate is presented and silent set false, notification should use developer's We should assert for vibration to be empty here when |silent| is set to true. No need to play nice since the only code that hits this is first-party stuff in Chrome. https://codereview.chromium.org/1054573002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:355: // vibrate value. "If a vibration pattern is set, the notification should use the provided pattern rather than the defaulting to system settings." https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_android.cc (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_android.cc:121: std::vector<int64> pattern; nit: Please use int64_t everywhere. int64 is deprecated. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_android.cc:122: if (!notification.vibrate().empty()) { We can simplify this block by using: ScopedJavaLocalRef<jlongArray> vibration_pattern = base::android::ToJavaLongArray(env, notification.vibrate()); https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_browsertest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:90: const unsigned kNotificationVibrate[] = { 100, 200, 300 }; Please use either int or int64_t here. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:275: WebNotificationOptionsVibrate) { Could we include this in the WebNotificationOptionsReflection test? Until the Message Center supports vibrations on other platforms, there's little point in testing it individually. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_unittest.cc:20: const unsigned kNotificationVibrate[] = { 100, 200, 300 }; nit: blank line https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_unittest.cc:20: const unsigned kNotificationVibrate[] = { 100, 200, 300 }; Please use either int or int64_t here. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_unittest.cc:161: std::vector<unsigned> vibrate_pattern( int/int64_t (whatever you choose to use in the //content API) https://codereview.chromium.org/1054573002/diff/160001/chrome/test/data/notif... File chrome/test/data/notifications/platform_notification_service.html (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/test/data/notif... chrome/test/data/notifications/platform_notification_service.html:65: function DisplayPersistentNotificationVibrate() { Dito - Please just include a pattern in DisplayPersistentAllOptionsNotification(), there's no need for a new function. https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... File content/browser/notifications/notification_database_data.proto (right): https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:37: repeated uint32 vibrate = 7 [packed=true]; uint32 should match the type you choose to use in the //content API. https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:39: optional bytes data = 9; On 2015/04/21 17:28:47, Avi wrote: > I am definitely no expert in protos, but from my fuzzy recollections, I thought > you never renumbered items. Where is this proto used? Indeed - please don't do this. You can insert the |vibrate| entry right where it is now, but give it number 9. https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... File content/browser/notifications/notification_database_data_conversions.cc (right): https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data_conversions.cc:42: for (int i = 0; i < payload.vibrate_size(); ++i) google::protobuf::RepeatedField exposes iterators which can do this for us, rather than iterating manually. Perhaps you'd prefer this? if (payload.vibrate_size()) { notification_data->vibrate.assign(payload.vibrate().begin(), payload.vibrate().end()); } https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... File content/browser/notifications/notification_database_data_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data_unittest.cc:26: std::vector<unsigned> vibrate_pattern( dito re: types in this file. https://codereview.chromium.org/1054573002/diff/160001/content/child/notifica... File content/child/notifications/notification_data_conversions.cc (right): https://codereview.chromium.org/1054573002/diff/160001/content/child/notifica... content/child/notifications/notification_data_conversions.cc:27: platform_data.vibrate.assign( nit: please format this as follows: platform_data.vibrate.assign(web_data.vibrate.begin(), web_data.vibrate.end()); https://codereview.chromium.org/1054573002/diff/160001/content/child/notifica... File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/content/child/notifica... content/child/notifications/notification_data_conversions_unittest.cc:27: std::vector<unsigned> vibrate_pattern( dito re: types in this file. https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... File content/public/common/platform_notification_data.h (right): https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... content/public/common/platform_notification_data.h:54: // Vibrate pettern for notification. On 2015/04/21 17:28:47, Avi wrote: > typo: pattern I'd prefer the following comment: // Vibration pattern for the notification, following the syntax of the // Vibration API. https://www.w3.org/TR/vibration/ https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... content/public/common/platform_notification_data.h:55: std::vector<unsigned> vibrate; On 2015/04/21 17:28:47, Avi wrote: > Use of unsigned ints is prohibited by the style guide. Use int. > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types We clamp the individual vibration indices to [0, blink::kVibrationDurationMax] in Blink, which currently matches [0, 10000]. Using std::vector<int> here seems good to me, but please change this consistently throughout the CL. https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... ui/message_center/notification.h:109: std::vector<unsigned> vibrate() const { return optional_fields_.vibrate; } const& https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... ui/message_center/notification.h:110: void set_vibrate(const std::vector<unsigned>& vibrate) { Have you given a thought about vibration on non-Android platforms? We don't need to fix it, but it'd be nice to eventually have an answer for it. Blink currently limits the sequence to 99 entries, times 4 bytes plus a bit of overhead, so the memory cost of supporting this on desktop would max out at ~400 bytes. That SGTM.
Dear Peter and Avi. Thanks for kind reviews!! I'll fix these. Thank you very much!! https://codereview.chromium.org/1054573002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:306: * @param vibrate Vibrate pettern for notification. Pass in an array of ints that are the On 2015/04/21 17:58:22, Peter Beverloo wrote: > Can we keep this vague ("Vibration pattern for the Notification.") and refer to > the Android documentation for the Notification object instead? > > @see http://developer.android.com/reference/android/app/Notification.html Okay, I'll double-check about this comment. https://codereview.chromium.org/1054573002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:354: // If vibrate is presented and silent set false, notification should use developer's On 2015/04/21 17:58:22, Peter Beverloo wrote: > We should assert for vibration to be empty here when |silent| is set to true. No > need to play nice since the only code that hits this is first-party stuff in > Chrome. I agree your opinion. I'll add assert check. https://codereview.chromium.org/1054573002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:355: // vibrate value. On 2015/04/21 17:58:22, Peter Beverloo wrote: > "If a vibration pattern is set, the notification should use the provided pattern > rather than the defaulting to system settings." Done. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_android.cc (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_android.cc:121: std::vector<int64> pattern; On 2015/04/21 17:58:22, Peter Beverloo wrote: > nit: Please use int64_t everywhere. int64 is deprecated. Done. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_android.cc:122: if (!notification.vibrate().empty()) { On 2015/04/21 17:58:22, Peter Beverloo wrote: > We can simplify this block by using: > > ScopedJavaLocalRef<jlongArray> vibration_pattern = > base::android::ToJavaLongArray(env, notification.vibrate()); We couldn't use ToJavaLongArray function directly. Because we should add temp value at the begginning of vibrate value. In Android platform, the first value is waiting time unlike blink. Please refer the [1]. If we do not insert temp value in this file, I should add this temp value in java file. Can I move this logic to NotificationUIManager.java? [1] http://developer.android.com/reference/android/os/Vibrator.html https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_browsertest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:90: const unsigned kNotificationVibrate[] = { 100, 200, 300 }; On 2015/04/21 17:58:23, Peter Beverloo wrote: > Please use either int or int64_t here. Done. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:275: WebNotificationOptionsVibrate) { On 2015/04/21 17:58:23, Peter Beverloo wrote: > Could we include this in the WebNotificationOptionsReflection test? > > Until the Message Center supports vibrations on other platforms, there's little > point in testing it individually. I cannot use WebNotificationOptionsReflection test. Because, silent set "true" in WebNotificationOptionsReflection. So I made other thing. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_unittest.cc:20: const unsigned kNotificationVibrate[] = { 100, 200, 300 }; On 2015/04/21 17:58:23, Peter Beverloo wrote: > nit: blank line Done. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_unittest.cc:161: std::vector<unsigned> vibrate_pattern( On 2015/04/21 17:58:23, Peter Beverloo wrote: > int/int64_t (whatever you choose to use in the //content API) Done. https://codereview.chromium.org/1054573002/diff/160001/chrome/test/data/notif... File chrome/test/data/notifications/platform_notification_service.html (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/test/data/notif... chrome/test/data/notifications/platform_notification_service.html:65: function DisplayPersistentNotificationVibrate() { On 2015/04/21 17:58:23, Peter Beverloo wrote: > Dito - Please just include a pattern in > DisplayPersistentAllOptionsNotification(), there's no need for a new function. Ditto. I tried to add vibrate property in DisplayPersistentAllOptionsNotification, but silent set true in function. So I made other function... https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... File content/browser/notifications/notification_database_data.proto (right): https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:37: repeated uint32 vibrate = 7 [packed=true]; On 2015/04/21 17:58:23, Peter Beverloo wrote: > uint32 should match the type you choose to use in the //content API. Done. https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:39: optional bytes data = 9; On 2015/04/21 17:28:47, Avi wrote: > I am definitely no expert in protos, but from my fuzzy recollections, I thought > you never renumbered items. Where is this proto used? I didn't know this. I'll add number for vibrate. This proto is used for serializing and storing in leveldb. https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:39: optional bytes data = 9; On 2015/04/21 17:58:23, Peter Beverloo wrote: > On 2015/04/21 17:28:47, Avi wrote: > > I am definitely no expert in protos, but from my fuzzy recollections, I > thought > > you never renumbered items. Where is this proto used? > > Indeed - please don't do this. You can insert the |vibrate| entry right where it > is now, but give it number 9. I see, I'll use number 9. https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... File content/browser/notifications/notification_database_data_conversions.cc (right): https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data_conversions.cc:42: for (int i = 0; i < payload.vibrate_size(); ++i) On 2015/04/21 17:58:23, Peter Beverloo wrote: > google::protobuf::RepeatedField exposes iterators which can do this for us, > rather than iterating manually. Perhaps you'd prefer this? > > if (payload.vibrate_size()) { > notification_data->vibrate.assign(payload.vibrate().begin(), > payload.vibrate().end()); > } I'd like to use iterators too :) I'll modify this. https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... File content/browser/notifications/notification_database_data_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/content/browser/notifi... content/browser/notifications/notification_database_data_unittest.cc:26: std::vector<unsigned> vibrate_pattern( On 2015/04/21 17:58:23, Peter Beverloo wrote: > dito re: types in this file. Done. https://codereview.chromium.org/1054573002/diff/160001/content/child/notifica... File content/child/notifications/notification_data_conversions.cc (right): https://codereview.chromium.org/1054573002/diff/160001/content/child/notifica... content/child/notifications/notification_data_conversions.cc:27: platform_data.vibrate.assign( On 2015/04/21 17:58:23, Peter Beverloo wrote: > nit: please format this as follows: > > platform_data.vibrate.assign(web_data.vibrate.begin(), > web_data.vibrate.end()); Done. https://codereview.chromium.org/1054573002/diff/160001/content/child/notifica... File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/content/child/notifica... content/child/notifications/notification_data_conversions_unittest.cc:27: std::vector<unsigned> vibrate_pattern( On 2015/04/21 17:58:23, Peter Beverloo wrote: > dito re: types in this file. Done. https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... File content/public/common/platform_notification_data.h (right): https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... content/public/common/platform_notification_data.h:54: // Vibrate pettern for notification. On 2015/04/21 17:28:47, Avi wrote: > typo: pattern Done. https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... content/public/common/platform_notification_data.h:54: // Vibrate pettern for notification. On 2015/04/21 17:28:47, Avi wrote: > typo: pattern Done. https://codereview.chromium.org/1054573002/diff/160001/content/public/common/... content/public/common/platform_notification_data.h:55: std::vector<unsigned> vibrate; On 2015/04/21 17:58:23, Peter Beverloo wrote: > On 2015/04/21 17:28:47, Avi wrote: > > Use of unsigned ints is prohibited by the style guide. Use int. > > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types > > We clamp the individual vibration indices to [0, blink::kVibrationDurationMax] > in Blink, which currently matches [0, 10000]. Using std::vector<int> here seems > good to me, but please change this consistently throughout the CL. Thanks for letting me know . I didn't know about [1]. We'll refer [1]document in detail. [1] "https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types". https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... ui/message_center/notification.h:53: std::vector<unsigned> vibrate; On 2015/04/21 17:28:47, Avi wrote: > ditto Done. https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... ui/message_center/notification.h:109: std::vector<unsigned> vibrate() const { return optional_fields_.vibrate; } On 2015/04/21 17:58:23, Peter Beverloo wrote: > const& Done. https://codereview.chromium.org/1054573002/diff/160001/ui/message_center/noti... ui/message_center/notification.h:110: void set_vibrate(const std::vector<unsigned>& vibrate) { On 2015/04/21 17:58:24, Peter Beverloo wrote: > Have you given a thought about vibration on non-Android platforms? We don't need > to fix it, but it'd be nice to eventually have an answer for it. I just think that this will be used in future. For example, recently macbook's touchpad support vibrate, If this vibrate's APIs are opened, this function will is useful. But not now. > > Blink currently limits the sequence to 99 entries, times 4 bytes plus a bit of > overhead, so the memory cost of supporting this on desktop would max out at ~400 > bytes. That SGTM. I didn't think like you, but your comment is very nice!! I'll consider about your comments. Thanks!
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Thank you! I've got a few more comments. https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_android.cc (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_android.cc:122: if (!notification.vibrate().empty()) { On 2015/04/23 11:08:25, Sanghyun Park wrote: > On 2015/04/21 17:58:22, Peter Beverloo wrote: > > We can simplify this block by using: > > > > ScopedJavaLocalRef<jlongArray> vibration_pattern = > > base::android::ToJavaLongArray(env, notification.vibrate()); > > We couldn't use ToJavaLongArray function directly. > Because we should add temp value at the begginning of vibrate value. > In Android platform, the first value is waiting time unlike blink. > Please refer the [1]. > > If we do not insert temp value in this file, I should add this temp value in > java file. > Can I move this logic to NotificationUIManager.java? > > [1] http://developer.android.com/reference/android/os/Vibrator.html Yes - having this in Java SGTM. (It changes common structures to platform-specific structures as close as we can to the usage.) https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_browsertest.cc (right): https://codereview.chromium.org/1054573002/diff/160001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:275: WebNotificationOptionsVibrate) { On 2015/04/23 11:08:26, Sanghyun Park wrote: > On 2015/04/21 17:58:23, Peter Beverloo wrote: > > Could we include this in the WebNotificationOptionsReflection test? > > > > Until the Message Center supports vibrations on other platforms, there's > little > > point in testing it individually. > > I cannot use WebNotificationOptionsReflection test. > Because, silent set "true" in WebNotificationOptionsReflection. > So I made other thing. I see! Thanks :-) https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:306: * @param vibrate Vibration pattern for the Notification. A notification that vibrates is nit: Remove the "A notification...heads-up notification." bit please. What behavior Android gives us when using a vibration pattern will be completely up to the version and the OEM running on the device. What I meant with the "@see ..." remark in my previous comment is that we can have a new line in this block-comment, after line 308, which says: @see https://developer.android.com/reference/android/app/Notification.html That way people can find additional information about displaying a notification and usage of the properties on that page. https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:354: // If a vibration pattern is set, the notification should use the provided pattern Could we perhaps extract this block to a separate method? It's quite a bit of extra logic, part of which could have some unit tests as well (see testGetOriginFromTag() in chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java). https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:367: for (int i = 0; i < vibrate.length; ++i) { Check out System.arraycopy() :-) https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:370: defaults &= ~Notification.DEFAULT_VIBRATE; nit: I'd have this line as the first line in the if-clause (line 362) since it relates to the code just above there, maintaining chronology. https://codereview.chromium.org/1054573002/diff/220001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_browsertest.cc (right): https://codereview.chromium.org/1054573002/diff/220001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:292: ASSERT_EQ(arraysize(kNotificationVibrate), notification.vibrate().size()); What do you think about renaming "vibrate" to "vibration_pattern" in Chromium, both in the message_center::Notification object and in PlatformNotificationData? The naming would not match the Web exposed API, but it would make reading this code much clearer. For example, accessing the vector now looks like: notification.vibrate() When reading the code, I'd expect that to.. vibrate the notification, whatever that might mean. Using "vibration" might still be slightly unclear on *what* it is, whereas "vibration_pattern" is both clear and unambiguous. https://codereview.chromium.org/1054573002/diff/220001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/220001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:298: #if defined(OS_ANDROID) Sorry, I didn't mean that you had to make this Android-only, I meant that I'm not concerned about using ~400 bytes in the very worst-case scenario per visible Notification elsewhere. Could you revert these changes and keep it as you had it in PS1? https://codereview.chromium.org/1054573002/diff/220001/content/browser/notifi... File content/browser/notifications/notification_database_data.proto (right): https://codereview.chromium.org/1054573002/diff/220001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:39: repeated int32 vibrate = 9 [packed=true]; Please keep the ordering synchronized with PlatformNotificationData, e.g. have "vibrate" above "silent". (But keep the tag "9"!) https://codereview.chromium.org/1054573002/diff/220001/content/child/notifica... File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/220001/content/child/notifica... content/child/notifications/notification_data_conversions_unittest.cc:41: blink::WebVector<unsigned>(vibrate_pattern), It would be good to quickly update the Blink API with s/unsigned/int/ now that there are no Chromium users yet - that will be much harder to do later on!
Thanks for your reviews! I'll update patch.:) https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:306: * @param vibrate Vibration pattern for the Notification. A notification that vibrates is On 2015/04/26 23:26:41, Peter Beverloo wrote: > nit: Remove the "A notification...heads-up notification." bit please. What > behavior Android gives us when using a vibration pattern will be completely up > to the version and the OEM running on the device. > > What I meant with the "@see ..." remark in my previous comment is that we can > have a new line in this block-comment, after line 308, which says: > > @see https://developer.android.com/reference/android/app/Notification.html > > That way people can find additional information about displaying a notification > and usage of the properties on that page. I missunderstood about this. I'll add "@see https://developer.android.com/reference/android/app/Notification.html" Thanks! https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:354: // If a vibration pattern is set, the notification should use the provided pattern On 2015/04/26 23:26:41, Peter Beverloo wrote: > Could we perhaps extract this block to a separate method? It's quite a bit of > extra logic, part of which could have some unit tests as well (see > testGetOriginFromTag() in > chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java). Sure, I'll make method and add testcase. https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:367: for (int i = 0; i < vibrate.length; ++i) { On 2015/04/26 23:26:41, Peter Beverloo wrote: > Check out System.arraycopy() :-) Unfortunately, we cannot use "arraycopy()". For using arraycopy(), src type is matched with dst type. The src(vibrate) is int array, but dst(pattern) is long array Notification.Builder.setDefaults's parameter has long type.. I didn't prefer this also, I want to find solution. but I cannot yet. https://codereview.chromium.org/1054573002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:370: defaults &= ~Notification.DEFAULT_VIBRATE; On 2015/04/26 23:26:41, Peter Beverloo wrote: > nit: I'd have this line as the first line in the if-clause (line 362) since it > relates to the code just above there, maintaining chronology. Done. https://codereview.chromium.org/1054573002/diff/220001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_browsertest.cc (right): https://codereview.chromium.org/1054573002/diff/220001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:292: ASSERT_EQ(arraysize(kNotificationVibrate), notification.vibrate().size()); On 2015/04/26 23:26:41, Peter Beverloo wrote: > What do you think about renaming "vibrate" to "vibration_pattern" in Chromium, > both in the message_center::Notification object and in PlatformNotificationData? > > The naming would not match the Web exposed API, but it would make reading this > code much clearer. For example, accessing the vector now looks like: > > notification.vibrate() > > When reading the code, I'd expect that to.. vibrate the notification, whatever > that might mean. Using "vibration" might still be slightly unclear on *what* it > is, whereas "vibration_pattern" is both clear and unambiguous. I like "vibration_pattern", too :) I'll update https://codereview.chromium.org/1054573002/diff/220001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/220001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:298: #if defined(OS_ANDROID) On 2015/04/26 23:26:41, Peter Beverloo wrote: > Sorry, I didn't mean that you had to make this Android-only, I meant that I'm > not concerned about using ~400 bytes in the very worst-case scenario per visible > Notification elsewhere. > > Could you revert these changes and keep it as you had it in PS1? Okay I'll change to PS1. https://codereview.chromium.org/1054573002/diff/220001/content/browser/notifi... File content/browser/notifications/notification_database_data.proto (right): https://codereview.chromium.org/1054573002/diff/220001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:39: repeated int32 vibrate = 9 [packed=true]; On 2015/04/26 23:26:41, Peter Beverloo wrote: > Please keep the ordering synchronized with PlatformNotificationData, e.g. have > "vibrate" above "silent". (But keep the tag "9"!) Done. https://codereview.chromium.org/1054573002/diff/220001/content/child/notifica... File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/220001/content/child/notifica... content/child/notifications/notification_data_conversions_unittest.cc:41: blink::WebVector<unsigned>(vibrate_pattern), On 2015/04/26 23:26:41, Peter Beverloo wrote: > It would be good to quickly update the Blink API with s/unsigned/int/ now that > there are no Chromium users yet - that will be much harder to do later on! Okay!, first of all I'll upload patch to update blink API.
Patchset #3 (id:240001) has been deleted
Patchset #3 (id:260001) has been deleted
Hi Sanghyun Park, Thank you for the update! The remainder of my comments are mostly about naming. Sorry for the delay. It's very difficult to find out that you uploaded a new patch set if you don't post a comment after uploading it. Generally, people share their responses to comments *after* uploading a new patch set. That gives the reviewer an indication that not just the responses are available, but also that they can be seen in the new patch set. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:296: * has been marked as being silent, for example because it's low priority. "for example because it's low priority." -> we don't support priority, so please don't add that here. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:303: * Returns 0 if silent is true. No need for lines 303-306 in the description, one can read the code. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:324: * In Android platform, the first value indicates the number of nit: the linebreaks are kind of odd in this comment. What about: * Android takes a long array where the first entry indicates the number of milliseconds to wait * prior to starting the vibration, whereas Chrome follows the syntax of the Web Vibration API. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:328: * @param vibratePattern Vibration pattern for the Notification. --> Vibration pattern following the Web Vibration API syntax. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:329: * @return The generated vibration pattern for Android. --> Vibration pattern following the Android syntax. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:354: * @param vibratePattern Vibration pattern for the Notification. --> Vibration pattern following the Web Vibration syntax. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:360: String title, String body, Bitmap icon, int[] vibratePattern, boolean silent) { nit: rename this to vibrationPattern? https://codereview.chromium.org/1054573002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java (right): https://codereview.chromium.org/1054573002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:404: NotificationUIManager.makeVibratePattern(new int[] {100, 200, 300}))); You should be able to do this: import org.junit.Assert; Assert.assertArrayEquals(...) https://codereview.chromium.org/1054573002/diff/280001/content/public/common/... File content/public/common/platform_notification_data.h (right): https://codereview.chromium.org/1054573002/diff/280001/content/public/common/... content/public/common/platform_notification_data.h:56: std::vector<int> vibrate_pattern; Could you tell me why you chose to go with "vibrate_pattern" rather than "vibration_pattern"? The former doesn't really work in English when used in a sentence: "The vibrate pattern is empty" vs. "The vibration pattern is empty".
> patch set if you don't post a comment after uploading it. > Generally, people share their responses to comments *after* uploading a new > patch set. That gives the reviewer an indication that not just the responses are > available, but also that they can be seen in the new patch set. Thanks for letting me know. I will do this in the future. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java (right): https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:296: * has been marked as being silent, for example because it's low priority. On 2015/05/01 17:17:59, Peter Beverloo wrote: > "for example because it's low priority." -> we don't support priority, so please > don't add that here. Done. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:303: * Returns 0 if silent is true. On 2015/05/01 17:17:59, Peter Beverloo wrote: > No need for lines 303-306 in the description, one can read the code. Okay, I'll remove these lines. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:324: * In Android platform, the first value indicates the number of On 2015/05/01 17:17:59, Peter Beverloo wrote: > nit: the linebreaks are kind of odd in this comment. > > What about: > > * Android takes a long array where the first entry indicates the number of > milliseconds to wait > * prior to starting the vibration, whereas Chrome follows the syntax of the > Web Vibration API. Done. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:328: * @param vibratePattern Vibration pattern for the Notification. On 2015/05/01 17:17:59, Peter Beverloo wrote: > --> Vibration pattern following the Web Vibration API syntax. Done. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:329: * @return The generated vibration pattern for Android. On 2015/05/01 17:17:59, Peter Beverloo wrote: > --> Vibration pattern following the Android syntax. Done. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:354: * @param vibratePattern Vibration pattern for the Notification. On 2015/05/01 17:17:59, Peter Beverloo wrote: > --> Vibration pattern following the Web Vibration syntax. Done. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:360: String title, String body, Bitmap icon, int[] vibratePattern, boolean silent) { On 2015/05/01 17:17:59, Peter Beverloo wrote: > nit: rename this to vibrationPattern? Done. https://codereview.chromium.org/1054573002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java (right): https://codereview.chromium.org/1054573002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:404: NotificationUIManager.makeVibratePattern(new int[] {100, 200, 300}))); On 2015/05/01 17:17:59, Peter Beverloo wrote: > You should be able to do this: > > import org.junit.Assert; > > Assert.assertArrayEquals(...) Thanks for your information. Then, as I know, junit is only used in "chrome_junit_tests" currently. but this file is chrome_shell_test;; For using junit, we should add junit package in chrome_shell_test_apk in chrome_test.gypi. Can I add this package in chrome_shell_test_apk? https://codereview.chromium.org/1054573002/diff/280001/content/public/common/... File content/public/common/platform_notification_data.h (right): https://codereview.chromium.org/1054573002/diff/280001/content/public/common/... content/public/common/platform_notification_data.h:56: std::vector<int> vibrate_pattern; On 2015/05/01 17:17:59, Peter Beverloo wrote: > Could you tell me why you chose to go with "vibrate_pattern" rather than > "vibration_pattern"? The former doesn't really work in English when used in a > sentence: > > "The vibrate pattern is empty" vs. "The vibration pattern is empty". Oops, this is my misstake. I'll rename to vibration pattern.
Patchset #4 (id:300001) has been deleted
Patchset #4 (id:320001) has been deleted
Dear Peter. Please take another look. Thanks!
Cool, thanks! One last comment :-) https://codereview.chromium.org/1054573002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java (right): https://codereview.chromium.org/1054573002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:404: NotificationUIManager.makeVibratePattern(new int[] {100, 200, 300}))); On 2015/05/04 07:00:44, Sanghyun Park wrote: > On 2015/05/01 17:17:59, Peter Beverloo wrote: > > You should be able to do this: > > > > import org.junit.Assert; > > > > Assert.assertArrayEquals(...) > > Thanks for your information. > Then, as I know, junit is only used in "chrome_junit_tests" currently. > but this file is chrome_shell_test;; > For using junit, we should add junit package in chrome_shell_test_apk in > chrome_test.gypi. > Can I add this package in chrome_shell_test_apk? No need to do that as part of this change. https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_browsertest.cc (right): https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:32: const int kNotificationVibrationPattern[] = { 100, 200, 300 }; nit: blank line above this one, to distinguish it from the fields which the comment (:29) applies to. https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:326: notification.set_vibration_pattern(notification_data.vibration_pattern); The final comment I have is that I'd like us to verify the validity of this information before using it, since it's coming from the renderer and we don't trust the renderer. Concretely, what about matching device/vibration/vibration_manager_impl_android.cc: add two file local constants (kMinimumVibrationDurationMs and kMaximumVibrationDurationMs) and clamp all entries in the vibration pattern to it? Something like: ===== std::vector<int> vibration_pattern; for (int pattern_entry : vibration_pattern) { pattern_entry = std::min(std::max(kMinimumVibrationDurationMs, pattern_entry), kMaximumVibrationDurationMs); vibration_pattern.push_back(pattern_entry); } notification.set_vibration_pattern(vibration_pattern); https://codereview.chromium.org/1054573002/diff/340001/content/child/notifica... File content/child/notifications/notification_data_conversions.cc (right): https://codereview.chromium.org/1054573002/diff/340001/content/child/notifica... content/child/notifications/notification_data_conversions.cc:28: web_data.vibrate.end()); nit: +2 spaces indent.
Patchset #5 (id:360001) has been deleted
Thanks for your reviews. :) I uploaded new patch. https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_browsertest.cc (right): https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:32: const int kNotificationVibrationPattern[] = { 100, 200, 300 }; On 2015/05/05 13:11:56, Peter Beverloo wrote: > nit: blank line above this one, to distinguish it from the fields which the > comment (:29) applies to. Okay, I'll add blank line. https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_browsertest.cc:240: ASSERT_EQ(arraysize(WebNotificationOptionsVibrationPattern), Oops.. Sorry. I used wrong name(WebNotificationOptionsVibrationPattern). This is changed to kNotificationVibrationPattern. https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/340001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:326: notification.set_vibration_pattern(notification_data.vibration_pattern); On 2015/05/05 13:11:56, Peter Beverloo wrote: > The final comment I have is that I'd like us to verify the validity of this > information before using it, since it's coming from the renderer and we don't > trust the renderer. > > Concretely, what about matching > device/vibration/vibration_manager_impl_android.cc: add two file local constants > (kMinimumVibrationDurationMs and kMaximumVibrationDurationMs) and clamp all > entries in the vibration pattern to it? Something like: > > ===== > > std::vector<int> vibration_pattern; > for (int pattern_entry : vibration_pattern) { > pattern_entry = std::min(std::max(kMinimumVibrationDurationMs, pattern_entry), > kMaximumVibrationDurationMs); > > vibration_pattern.push_back(pattern_entry); > } > > notification.set_vibration_pattern(vibration_pattern); Sure, I'll add these local constants in this file and add these code for clamp. https://codereview.chromium.org/1054573002/diff/340001/content/child/notifica... File content/child/notifications/notification_data_conversions.cc (right): https://codereview.chromium.org/1054573002/diff/340001/content/child/notifica... content/child/notifications/notification_data_conversions.cc:28: web_data.vibrate.end()); On 2015/05/05 13:11:56, Peter Beverloo wrote: > nit: +2 spaces indent. Done.
lgtm! Thank you for your patience! Now comes the OWNERs dance.. https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:330: // Though the Blink implementation already sanitizes vibration times, don't nit: Theoretically this doesn't have to come from Blink. What about: // Make sure that the vibration values are within reasonable bounds. https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:330: // Though the Blink implementation already sanitizes vibration times, don't optional nit: You could add a unit test for this in PlatformNotificationServiceTest.
This seems pretty simple to me. LGTM with question. https://codereview.chromium.org/1054573002/diff/380001/content/browser/notifi... File content/browser/notifications/notification_database_data.proto (right): https://codereview.chromium.org/1054573002/diff/380001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:37: repeated int32 vibration_pattern = 9 [packed=true]; Why not arrange these in order?
https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:331: // trust any values passed from the client. I'd prefer to see this sanitization moved to the IPC message handler.
Dear Peter, Avi and dcheng. Thanks so much for review!! :) https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:330: // Though the Blink implementation already sanitizes vibration times, don't On 2015/05/05 20:42:44, Peter Beverloo wrote: > nit: Theoretically this doesn't have to come from Blink. What about: > > // Make sure that the vibration values are within reasonable bounds. Done. https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:331: // trust any values passed from the client. On 2015/05/05 21:56:33, dcheng wrote: > I'd prefer to see this sanitization moved to the IPC message handler. 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? https://codereview.chromium.org/1054573002/diff/380001/content/browser/notifi... File content/browser/notifications/notification_database_data.proto (right): https://codereview.chromium.org/1054573002/diff/380001/content/browser/notifi... content/browser/notifications/notification_database_data.proto:37: repeated int32 vibration_pattern = 9 [packed=true]; On 2015/05/05 20:53:59, Avi wrote: > Why not arrange these in order? This order is in accordance with the notification specification. [1] https://notifications.spec.whatwg.org/#idl-index
https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:331: // trust any values passed from the client. On 2015/05/06 14:53:25, Sanghyun Park wrote: > On 2015/05/05 21:56:33, dcheng wrote: > > I'd prefer to see this sanitization moved to the IPC message handler. > > 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? 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.
Patchset #6 (id:400001) has been deleted
https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1054573002/diff/380001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:331: // trust any values passed from the client. On 2015/05/06 15:35:17, Peter Beverloo wrote: > On 2015/05/06 14:53:25, Sanghyun Park wrote: > > On 2015/05/05 21:56:33, dcheng wrote: > > > I'd prefer to see this sanitization moved to the IPC message handler. > > > > 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? > > 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. Okay, I see. I upload patch about this. Dear Daniel and Peter thanks for your guide and detail explanation :)
Patchset #6 (id:420001) has been deleted
https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... File content/browser/notifications/notification_database_data_conversions.cc (right): https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... content/browser/notifications/notification_database_data_conversions.cc:42: if (payload.vibration_pattern().size()) { empty() instead of size() makes this clearer Otherwise, write size() > 0 https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... File content/browser/notifications/notification_database_data_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... content/browser/notifications/notification_database_data_unittest.cc:78: ASSERT_EQ(vibration_pattern.size(), You could do something like: EXPECT_THAT(copied_notification_data.vibration_patterns, ElementsAre(kNotificationVibrationPattern)); I don't feel strongly about this, but it might be slightly easier to write. https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... File content/browser/notifications/notification_message_filter.cc (right): https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:25: content::PlatformNotificationData sanitizeNotificationData( SanitizeNotificationData, to follow Chrome/Google style. https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:52: } Convention is to annotate the closing brace with // namespace.
Dear Daniel. Thanks for your comments. I had fix these. Please take another look. https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... File content/browser/notifications/notification_database_data_conversions.cc (right): https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... content/browser/notifications/notification_database_data_conversions.cc:42: if (payload.vibration_pattern().size()) { On 2015/05/06 18:43:27, dcheng wrote: > empty() instead of size() makes this clearer > > Otherwise, write size() > 0 google::protobuf::RepeatedField does not have "empty()" function.;; So, I'll modify to size() > 0 https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... File content/browser/notifications/notification_database_data_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... content/browser/notifications/notification_database_data_unittest.cc:78: ASSERT_EQ(vibration_pattern.size(), On 2015/05/06 18:43:27, dcheng wrote: > You could do something like: > EXPECT_THAT(copied_notification_data.vibration_patterns, > ElementsAre(kNotificationVibrationPattern)); > > I don't feel strongly about this, but it might be slightly easier to write. Your suggestion is better than this code. :) I'll change this https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... File content/browser/notifications/notification_message_filter.cc (right): https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:25: content::PlatformNotificationData sanitizeNotificationData( On 2015/05/06 18:43:27, dcheng wrote: > SanitizeNotificationData, to follow Chrome/Google style. Oops, It's my mistake. I'll change. https://codereview.chromium.org/1054573002/diff/440001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:52: } On 2015/05/06 18:43:27, dcheng wrote: > Convention is to annotate the closing brace with // namespace. Done.
https://codereview.chromium.org/1054573002/diff/460001/content/browser/notifi... File content/browser/notifications/notification_message_filter.cc (right): https://codereview.chromium.org/1054573002/diff/460001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:20: namespace { Move the unnamed namespace into the content namespace below, and you won't need the content:: qualifier. https://codereview.chromium.org/1054573002/diff/460001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:34: sanitized_data.icon = notification_data.icon; I wonder if this would be more future proof if you did this: content::PlatformNotificationData sanitized_data = notification_data; for (int& pattern : sanitized_data.vibration_pattern) { pattern = std::min(kMaximumVibriationDurationMs, std::max(kMinimumVibrationDurationMs, pattern)); } return sanitized_data; https://codereview.chromium.org/1054573002/diff/460001/content/child/notifica... File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/460001/content/child/notifica... content/child/notifications/notification_data_conversions_unittest.cc:57: EXPECT_EQ(vibration_pattern[i], platform_data.vibration_pattern[i]); Does EXPECT_THAT(...) work here too? https://codereview.chromium.org/1054573002/diff/460001/content/child/notifica... content/child/notifications/notification_data_conversions_unittest.cc:109: ASSERT_EQ(vibration_pattern.size(), web_data.vibrate.size()); Ditto
Thanks !! I had updated about theses. https://codereview.chromium.org/1054573002/diff/460001/content/browser/notifi... File content/browser/notifications/notification_message_filter.cc (right): https://codereview.chromium.org/1054573002/diff/460001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:20: namespace { On 2015/05/07 22:08:40, dcheng wrote: > Move the unnamed namespace into the content namespace below, and you won't need > the content:: qualifier. Done. https://codereview.chromium.org/1054573002/diff/460001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:34: sanitized_data.icon = notification_data.icon; Okay, I'll modify this https://codereview.chromium.org/1054573002/diff/460001/content/child/notifica... File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1054573002/diff/460001/content/child/notifica... content/child/notifications/notification_data_conversions_unittest.cc:57: EXPECT_EQ(vibration_pattern[i], platform_data.vibration_pattern[i]); On 2015/05/07 22:08:40, dcheng wrote: > Does EXPECT_THAT(...) work here too? Done. https://codereview.chromium.org/1054573002/diff/460001/content/child/notifica... content/child/notifications/notification_data_conversions_unittest.cc:109: ASSERT_EQ(vibration_pattern.size(), web_data.vibrate.size()); On 2015/05/07 22:08:40, dcheng wrote: > Ditto EXPECT_THAT does not support web_data.vibrate type (WebVector<int>). So, we cannot use EXPECT_THAT;;;
https://codereview.chromium.org/1054573002/diff/480001/content/browser/notifi... File content/browser/notifications/notification_message_filter.cc (right): https://codereview.chromium.org/1054573002/diff/480001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:29: PlatformNotificationData sanitized_data; My point was that you could make use of the copy constructor for PlatformNotificationData and avoid duplicating all these fields. That also makes this code more future proof, when you want to add further fields.
Dear Daniel. Thanks for your advise. PTAL :) https://codereview.chromium.org/1054573002/diff/480001/content/browser/notifi... File content/browser/notifications/notification_message_filter.cc (right): https://codereview.chromium.org/1054573002/diff/480001/content/browser/notifi... content/browser/notifications/notification_message_filter.cc:29: PlatformNotificationData sanitized_data; On 2015/05/08 20:22:27, dcheng wrote: > My point was that you could make use of the copy constructor for > PlatformNotificationData and avoid duplicating all these fields. That also makes > this code more future proof, when you want to add further fields. Oops. Sorry, I misunderstand. Thank for your detailed explanation.
lgtm
Dear All. Thanks so much for your help about this. :)
The CQ bit was checked by sh919.park@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/1054573002/#ps500001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1054573002/500001
Message was sent while issue was closed.
Committed patchset #9 (id:500001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1593bf8e67c4e99be94a85bec217463961a9c07c Cr-Commit-Position: refs/heads/master@{#329082} |
