Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1042513002:
Add the vibrate attribute to the Notification object (Closed)
|
Created:
5 years, 9 months ago by Sanghyun Park Modified:
5 years, 8 months ago CC:
blink-reviews, mlamouri+watch-blink_chromium.org, mvanouwerkerk+watch_chromium.org, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd the vibrate attribute to the Notification object
Authors can now supply a vibration pattern similar to
"navigator.vibrate()", which is to be applied
when a Notification gets shown.
The Web Notification specification:
https://notifications.spec.whatwg.org/#dom-notificationoptions-vibrate
BUG=442132
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193953
Patch Set 1 : #
Total comments: 21
Patch Set 2 : #
Total comments: 2
Patch Set 3 : WebNotificationVibrationData is renamed WebNotificationVibratePattern #
Total comments: 25
Patch Set 4 : Rebase #
Total comments: 13
Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 6
Patch Set 7 : #
Total comments: 7
Patch Set 8 #Messages
Total messages: 62 (29 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
sh919.park@samsung.com changed reviewers: + peter@chromium.org
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: + mvanouwerkerk@chromium.org
mvanouwerkerk@chromium.org: Please review changes in PTAL.
peter@chromium.org changed reviewers: + timvolodine@chromium.org
+timvolodine for Vibration Hi Sanghyun Park, Thank you for the patch! I've attached a few early comments. What is your thought on surfacing the vibration pattern back to the developer (like all other properties)? There's a TODO in the specification, but I wonder if we perhaps could drive that to a conclusion? Additionally, I'd like Tim to take a look at the vibration part. He'll be able to judge that much better than I can :). Thanks! https://codereview.chromium.org/1042513002/diff/160001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/notification-properties.html (right): https://codereview.chromium.org/1042513002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:69: // Tests that sequence<unsigned long> input no throws exceptions. sequence<unsigned long> doesn't mean anything to JavaScript. Perhaps describe that it must be a valid vibration sequence? https://codereview.chromium.org/1042513002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:74: // Verifying the exception throwing behavior, when slient set true and vibrate is presented. Could you add a test for ServiceWorkerRegistration.showNotification() as well, which should reject on invalid patterns? https://codereview.chromium.org/1042513002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:76: var notification = new Notification('Title', { nit: Please be consistent with ' vs. " for defining strings in a file. https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/Notification.cpp:83: exceptionState.throwTypeError("If options's silent is true, options's vibrate should not be presented"); What about: "Notifications must not have a vibration pattern when they are set to be silent."? https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... File Source/modules/notifications/NotificationOptions.idl (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:13: typedef (unsigned long or sequence<unsigned long>) VibratePattern; Why can't we share this definition with the Vibration API? https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:8: nit: no blank line https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:72: NavigatorVibration::VibrationPattern vibrate; Perhaps it makes sense to look into unifying the methods first? This is another bunch of duplicated code :-(. https://codereview.chromium.org/1042513002/diff/160001/Source/modules/vibrati... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:152: NavigatorVibration::VibrationPattern NavigatorVibration::verifyVibrationPattern(const VibrationPattern& pattern) s/verify/sanitize? I'd expect a verifyFoo method to return a boolean. https://codereview.chromium.org/1042513002/diff/160001/public/platform/module... File public/platform/modules/notifications/WebNotificationData.h (right): https://codereview.chromium.org/1042513002/diff/160001/public/platform/module... 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, const WebVector<unsigned>& vibrate, bool silent, const WebString& data) nit: you'll need another temporary constructor, since some places in Chromium will start using the |data| member Real Soon(tm).
in addition to Peter's comments ;) https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/Notification.cpp:87: NavigatorVibration::VibrationPattern pattern; How about introducing a method static VibrationPattern NavigatorVibration::sanitizeVibrationPattern(const UnsignedLongOrUnsignedLongSequence&) using UnionTypesModules.h? That way you could save a couple of ifs here and in ServiceWorkerRegistrationNotifications.cpp. https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... File Source/modules/notifications/NotificationOptions.idl (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:13: typedef (unsigned long or sequence<unsigned long>) VibratePattern; On 2015/04/01 14:26:29, Peter Beverloo wrote: > Why can't we share this definition with the Vibration API? +1
Dear Peter and Tim. Thanks for your comments. Please refer below comment. https://codereview.chromium.org/1042513002/diff/160001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/notification-properties.html (right): https://codereview.chromium.org/1042513002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:69: // Tests that sequence<unsigned long> input no throws exceptions. On 2015/04/01 14:26:29, Peter Beverloo wrote: > sequence<unsigned long> doesn't mean anything to JavaScript. Perhaps describe > that it must be a valid vibration sequence? I'll fix this. https://codereview.chromium.org/1042513002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:74: // Verifying the exception throwing behavior, when slient set true and vibrate is presented. On 2015/04/01 14:26:29, Peter Beverloo wrote: > Could you add a test for ServiceWorkerRegistration.showNotification() as well, > which should reject on invalid patterns? Okay I'll add test of ServiceWorkerRegistration.showNotification() also. https://codereview.chromium.org/1042513002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:76: var notification = new Notification('Title', { On 2015/04/01 14:26:29, Peter Beverloo wrote: > nit: Please be consistent with ' vs. " for defining strings in a file. Done. https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/Notification.cpp:83: exceptionState.throwTypeError("If options's silent is true, options's vibrate should not be presented"); On 2015/04/01 14:26:29, Peter Beverloo wrote: > What about: > > "Notifications must not have a vibration pattern when they are set to be > silent."? Yes. Following Spec, this is described like below. "If options’s silent is true, and either options’s sound is present or options’s vibrate is present, throw a TypeError exception." [1] https://notifications.spec.whatwg.org/#create-a-notification https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/Notification.cpp:87: NavigatorVibration::VibrationPattern pattern; On 2015/04/01 16:27:16, timvolodine wrote: > How about introducing a method > static VibrationPattern NavigatorVibration::sanitizeVibrationPattern(const > UnsignedLongOrUnsignedLongSequence&) > using UnionTypesModules.h? That way you could save a couple of ifs here and in > ServiceWorkerRegistrationNotifications.cpp. Thanks for your advice. :) I agree your opinion. https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... File Source/modules/notifications/NotificationOptions.idl (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:13: typedef (unsigned long or sequence<unsigned long>) VibratePattern; On 2015/04/01 14:26:29, Peter Beverloo wrote: > Why can't we share this definition with the Vibration API? Unfortunately, NavigatorVibration.idl did not support VibratePattern by unionType for now, because of [Clamp] issue. (As I know, unionType does not support [Clamp].) NavigatorVibration.idl uses the overloading function of the two types for now. So, I had no choice. https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:8: On 2015/04/01 14:26:29, Peter Beverloo wrote: > nit: no blank line Done. https://codereview.chromium.org/1042513002/diff/160001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:72: NavigatorVibration::VibrationPattern vibrate; On 2015/04/01 14:26:29, Peter Beverloo wrote: > Perhaps it makes sense to look into unifying the methods first? This is another > bunch of duplicated code :-(. I'll also your opinion. But, if we'll support vibrate attribute of notification object, I think we should not sanitize vibrate pattern in blink. Because, we need to keep the user's settings of vibrate for persistent notification. WDYT? https://codereview.chromium.org/1042513002/diff/160001/Source/modules/vibrati... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/1042513002/diff/160001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:152: NavigatorVibration::VibrationPattern NavigatorVibration::verifyVibrationPattern(const VibrationPattern& pattern) On 2015/04/01 14:26:29, Peter Beverloo wrote: > s/verify/sanitize? I'd expect a verifyFoo method to return a boolean. Done. https://codereview.chromium.org/1042513002/diff/160001/public/platform/module... File public/platform/modules/notifications/WebNotificationData.h (right): https://codereview.chromium.org/1042513002/diff/160001/public/platform/module... 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, const WebVector<unsigned>& vibrate, bool silent, const WebString& data) On 2015/04/01 14:26:29, Peter Beverloo wrote: > nit: you'll need another temporary constructor, since some places in Chromium > will start using the |data| member Real Soon(tm). Done.
On 2015/04/01 14:26:29, Peter Beverloo wrote: > +timvolodine for Vibration > > Hi Sanghyun Park, > > Thank you for the patch! I've attached a few early comments. > > What is your thought on surfacing the vibration pattern back to the developer > (like all other properties)? There's a TODO in the specification, but I wonder > if we perhaps could drive that to a conclusion? Dear Peter I also have considered about this. but I excluded this, since I could not decide, Thank you for proposing this. I like your opinion. :) I'll make vibrate attribute. But If we support vibrate attribute, we have a issue. As mentioned above, we cannot sanitize vibrate data in blink, since we should keep user's settting vibrate. WDYT?? > > Additionally, I'd like Tim to take a look at the vibration part. He'll be able > to judge that much better than I can :). > > Thanks! >
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
I updated new patch. Please refer below comments. Thanks https://codereview.chromium.org/1042513002/diff/220001/public/platform/WebVib... File public/platform/WebVibration.h (right): https://codereview.chromium.org/1042513002/diff/220001/public/platform/WebVib... public/platform/WebVibration.h:40: The reason of moving this value into WebVibration is for using in chromium layer. Notification.vibrate cannot sanitize vibrate pattern in blink, becuase we keep user's setting of Notification.vibrate in blink. So we need this value to sanitize vibrate pattern in chomium layer. https://codereview.chromium.org/1042513002/diff/220001/public/platform/module... File public/platform/modules/notifications/WebNotificationVibrationData.h (right): https://codereview.chromium.org/1042513002/diff/220001/public/platform/module... public/platform/modules/notifications/WebNotificationVibrationData.h:17: TypeUnsignedLongSequence This structure is necessary to return vibrate value as set by the user. This value will be serialized in order to store in the notification database.
sh919.park@samsung.com changed reviewers: + japhet@chromium.org
japhet@chromium.org: Please review changes in
Hi Sanghyun Park, Sorry for the delay, we had a few national holidays here in the United Kingdom. Please find attached some comments. Additionally, please send an Intent to Implement to blink-dev about implementing this feature, as it's not currently covered by any other intent. https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html (right): https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html:14: var scope = 'resources/spec/' + location.pathname, why /spec/? https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html:32: assert_equals(error.message, 'If options\'s silent is true, options\'s vibrate should not be presented.'); "If options's silent is true, options's vibrate should not be presented." --> "If the notification is set to silent while a vibration pattern has been provided, the Promise is expected to reject." Please make similar changes elsewhere, as "option's vibrate should not be presented" isn't clear. https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate.html (right): https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate.html:41: // FIXME: Verify that the correct vibrate is returned or not. This test doesn't actually test anything. Can we remove it? https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.cpp:81: // If options’s silent is true, and options’s vibrate is present, throw a TypeError exception. ’ -> ' https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.cpp:83: exceptionState.throwTypeError("If options's silent is true, options's vibrate should not be presented"); "Silent notifications must not specify vibration patterns" https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.cpp:136: } This seems overly complicated to me. We should always pass up an unsigned long sequence to the embedder as that's how they will be using it. (Ask yourself this: How would the embedder behave differently based on the type?) https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.cpp:174: returnValue.setUnsignedLongSequence(m_vibrate.getAsUnsignedLongSequence()); Ah, I see why you're doing this. This is your interpretation of the unresolved spec issue on how to expose the "vibration" attribute back to the developer. I don't think maintaining the single-value or sequence state is worth the added complexity of keeping track of this distance. Rather, we should just always return a sequence if a pattern is set, or NULL if no pattern has been set. https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... File Source/modules/notifications/Notification.idl (right): https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.idl:61: [RuntimeEnabled=NotificationExperimental] readonly attribute (unsigned long or sequence<unsigned long>) vibrate; See my other comment - let's not maintain the type of argument passed throughout the stack, at least until a decision has been made on how to support this in the specification. https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... File Source/modules/notifications/NotificationOptions.idl (right): https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:19: // FIXME: The Union type VibrationPattern is not supported in NavigatorVibration. nit: We switched to attributed TODOs rather than unattributed FIXMEs. TODO(sh919.park): Foobarbaz https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:20: // If VibrationPattern is supported, we need to change with this. I think it would make sense to fix Issue 240176 first, that should be a very trivial ~20 line patch, and introduce the VibrationPattern union typedef in NavigatorVibration. Then we adopt that immediately. https://codereview.chromium.org/1042513002/diff/240001/public/platform/module... File public/platform/modules/notifications/WebNotificationVibratePattern.h (right): https://codereview.chromium.org/1042513002/diff/240001/public/platform/module... public/platform/modules/notifications/WebNotificationVibratePattern.h:13: struct WebNotificationVibratePattern { As said elsewhere, we shouldn't have our own struct for this. A WebVector<unsigned> will do fine.
Thanks so much for your comments :) > Please find attached some comments. Additionally, please send an Intent to Implement to blink-dev about implementing this feature, as it's not currently covered by any other intent. Okay I'll send an Intent about this. Thanks. https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html (right): https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html:14: var scope = 'resources/spec/' + location.pathname, On 2015/04/07 11:37:31, Peter Beverloo wrote: > why /spec/? This is my fault. I'll update to scope. https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html:32: assert_equals(error.message, 'If options\'s silent is true, options\'s vibrate should not be presented.'); On 2015/04/07 11:37:31, Peter Beverloo wrote: > "If options's silent is true, options's vibrate should not be presented." > --> > "If the notification is set to silent while a vibration pattern has been > provided, the Promise is expected to reject." > > Please make similar changes elsewhere, as "option's vibrate should not be > presented" isn't clear. Okay, I'll update. https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate.html (right): https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate.html:41: // FIXME: Verify that the correct vibrate is returned or not. On 2015/04/07 11:37:31, Peter Beverloo wrote: > This test doesn't actually test anything. Can we remove it? Sure, After the this implementation is completed until chromium, I will add the test. https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.cpp:81: // If options’s silent is true, and options’s vibrate is present, throw a TypeError exception. On 2015/04/07 11:37:31, Peter Beverloo wrote: > ’ -> ' Done. https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.cpp:83: exceptionState.throwTypeError("If options's silent is true, options's vibrate should not be presented"); On 2015/04/07 11:37:31, Peter Beverloo wrote: > "Silent notifications must not specify vibration patterns" Done. https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.cpp:174: returnValue.setUnsignedLongSequence(m_vibrate.getAsUnsignedLongSequence()); On 2015/04/07 11:37:31, Peter Beverloo wrote: > Ah, I see why you're doing this. This is your interpretation of the unresolved > spec issue on how to expose the "vibration" attribute back to the developer. > > I don't think maintaining the single-value or sequence state is worth the added > complexity of keeping track of this distance. > > Rather, we should just always return a sequence if a pattern is set, or NULL if > no pattern has been set. Okay, I will fix to only return sequence type. https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... File Source/modules/notifications/Notification.idl (right): https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/Notification.idl:61: [RuntimeEnabled=NotificationExperimental] readonly attribute (unsigned long or sequence<unsigned long>) vibrate; On 2015/04/07 11:37:31, Peter Beverloo wrote: > See my other comment - let's not maintain the type of argument passed throughout > the stack, at least until a decision has been made on how to support this in the > specification. Done. https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... File Source/modules/notifications/NotificationOptions.idl (right): https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:19: // FIXME: The Union type VibrationPattern is not supported in NavigatorVibration. On 2015/04/07 11:37:31, Peter Beverloo wrote: > nit: We switched to attributed TODOs rather than unattributed FIXMEs. > > TODO(sh919.park): Foobarbaz Okay I'll fix this. https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:20: // If VibrationPattern is supported, we need to change with this. On 2015/04/07 11:37:31, Peter Beverloo wrote: > I think it would make sense to fix Issue 240176 first, that should be a very > trivial ~20 line patch, and introduce the VibrationPattern union typedef in > NavigatorVibration. Then we adopt that immediately. I tried to make VibrationPattern union type like [1]. typedef ([Clamp] unsigned long or sequence<unsigned long>) VibratePattern; But Union type does not support [Clamp] extended attribute. So I think that we need to fix idl parser in order to make VibratePattern union type. Currently, I am finding solution to support [Clamp] extended attribute in union. I'm not familiar about idl parser, so it will take some time. So, Can I write "FIXME" in this file? After implementing extended attribute in union, I'll fix this. https://codereview.chromium.org/1042513002/diff/240001/public/platform/module... File public/platform/modules/notifications/WebNotificationVibratePattern.h (right): https://codereview.chromium.org/1042513002/diff/240001/public/platform/module... public/platform/modules/notifications/WebNotificationVibratePattern.h:13: struct WebNotificationVibratePattern { On 2015/04/07 11:37:31, Peter Beverloo wrote: > As said elsewhere, we shouldn't have our own struct for this. A > WebVector<unsigned> will do fine. Okay I'll remove this.
https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html (right): https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html:32: assert_equals(error.message, 'If options\'s silent is true, options\'s vibrate should not be presented.'); On 2015/04/08 08:00:02, Sanghyun Park wrote: > On 2015/04/07 11:37:31, Peter Beverloo wrote: > > "If options's silent is true, options's vibrate should not be presented." > > --> > > "If the notification is set to silent while a vibration pattern has been > > provided, the Promise is expected to reject." > > > > Please make similar changes elsewhere, as "option's vibrate should not be > > presented" isn't clear. > > Okay, I'll update. > This error.message should be same with throwTypeError message in Notification.cpp. Can I modify this message to "Silent notifications must not specify vibration patterns"?
https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html (right): https://codereview.chromium.org/1042513002/diff/240001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/serviceworkerregistration-document-vibrate-throw.html:32: assert_equals(error.message, 'If options\'s silent is true, options\'s vibrate should not be presented.'); On 2015/04/08 16:37:58, Sanghyun Park wrote: > On 2015/04/08 08:00:02, Sanghyun Park wrote: > > On 2015/04/07 11:37:31, Peter Beverloo wrote: > > > "If options's silent is true, options's vibrate should not be presented." > > > --> > > > "If the notification is set to silent while a vibration pattern has been > > > provided, the Promise is expected to reject." > > > > > > Please make similar changes elsewhere, as "option's vibrate should not be > > > presented" isn't clear. > > > > Okay, I'll update. > > > > This error.message should be same with throwTypeError message in > Notification.cpp. > Can I modify this message to "Silent notifications must not specify vibration > patterns"? SGTM https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... File Source/modules/notifications/NotificationOptions.idl (right): https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:20: // If VibrationPattern is supported, we need to change with this. On 2015/04/08 08:00:03, Sanghyun Park wrote: > On 2015/04/07 11:37:31, Peter Beverloo wrote: > > I think it would make sense to fix Issue 240176 first, that should be a very > > trivial ~20 line patch, and introduce the VibrationPattern union typedef in > > NavigatorVibration. Then we adopt that immediately. > > I tried to make VibrationPattern union type like [1]. > typedef ([Clamp] unsigned long or sequence<unsigned long>) VibratePattern; > > But Union type does not support [Clamp] extended attribute. > So I think that we need to fix idl parser in order to make VibratePattern union > type. > > Currently, I am finding solution to support [Clamp] extended attribute in union. > I'm not familiar about idl parser, so it will take some time. > > So, Can I write "FIXME" in this file? After implementing extended attribute in > union, I'll fix this. Ok, but please make sure that there's a bug. Also, s/FIXME/TODO(yourname)/
https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... File Source/modules/notifications/NotificationOptions.idl (right): https://codereview.chromium.org/1042513002/diff/240001/Source/modules/notific... Source/modules/notifications/NotificationOptions.idl:20: // If VibrationPattern is supported, we need to change with this. On 2015/04/08 16:40:01, Peter Beverloo wrote: > On 2015/04/08 08:00:03, Sanghyun Park wrote: > > On 2015/04/07 11:37:31, Peter Beverloo wrote: > > > I think it would make sense to fix Issue 240176 first, that should be a very > > > trivial ~20 line patch, and introduce the VibrationPattern union typedef in > > > NavigatorVibration. Then we adopt that immediately. > > > > I tried to make VibrationPattern union type like [1]. > > typedef ([Clamp] unsigned long or sequence<unsigned long>) VibratePattern; > > > > But Union type does not support [Clamp] extended attribute. > > So I think that we need to fix idl parser in order to make VibratePattern > union > > type. > > > > Currently, I am finding solution to support [Clamp] extended attribute in > union. > > I'm not familiar about idl parser, so it will take some time. > > > > So, Can I write "FIXME" in this file? After implementing extended attribute in > > union, I'll fix this. > > Ok, but please make sure that there's a bug. Also, s/FIXME/TODO(yourname)/ Sure, I've search the bug, I could not find something like this. I'll use Issue 240176, when I fix this.
Patchset #4 (id:260001) has been deleted
Dear Peter, I sent an intent about this [1]. [1] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pFC0Qj6tvA8 Could you confirm this? Thanks
Patchset #5 (id:300001) has been deleted
Thank you for the Intent to Implement - it looks good to me, I replied to the thread. Your patch mostly looks good to me, but there's one comment about sanitizing the vibration pattern I'd like you to have a look at. Perhaps Tim might have some ideas in this area as well. https://codereview.chromium.org/1042513002/diff/320001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/notification-properties.html (right): https://codereview.chromium.org/1042513002/diff/320001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:35: assert_equals(notification.vibrate, null); nit: Please add a value to |options| for this assert, like the other ones. https://codereview.chromium.org/1042513002/diff/320001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:76: assert_array_equals([1000], vibrateNotification.vibrate); nit: In testharness, the argument order for assertions is assert(actual, expected); This should thus read: assert_array_equals(vibrateNotification.vibrate, [1000]); Dito regarding the new assertions below. https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... File Source/modules/notifications/Notification.h (right): https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... Source/modules/notifications/Notification.h:103: static Vector<unsigned> sanitizeVibrationPattern(const UnsignedLongOrUnsignedLongSequence&); I don't think this should be the responsibility of anything in modules/notifications/, not least of all because it doesn't implement the specification or any of the sanity checks. The NavigatorVibration::vibrate() method implements the sanitation algorithm as specified by the Vibration API. Why don't you extract that to a static method and use it here (and in SWRNotifications)? http://dev.w3.org/2009/dap/vibration/#dfn-validate-and-normalize https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp (right): https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:70: return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(scriptState->isolate(), "Slient notification must not specify vibration patterns")); nit: Sentences end with a period. https://codereview.chromium.org/1042513002/diff/320001/public/platform/WebVib... File public/platform/WebVibration.h (right): https://codereview.chromium.org/1042513002/diff/320001/public/platform/WebVib... public/platform/WebVibration.h:40: const unsigned kVibrationPatternLengthMax = 99; Note that you're exposing this in the Blink API, but aren't actually using it anywhere new. If you group all vibration pattern sanitation to NavigatorVibration then it can remain file-private to that file, and you don't have to touch this part of the API.
Dear Peter. Thanks for your comments. https://codereview.chromium.org/1042513002/diff/320001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/notification-properties.html (right): https://codereview.chromium.org/1042513002/diff/320001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:35: assert_equals(notification.vibrate, null); On 2015/04/09 13:03:17, Peter Beverloo wrote: > nit: Please add a value to |options| for this assert, like the other ones. Currently options.silent set true in this. If vibrate is set by any values, typeError is occurred.. And if I change options.silent value to "false", this page does not have test for checking silent. So, I made other test for vibrate in line 66. + var vibrateNotification = new Notification("My Notification", { + vibrate: 1000 + }); + assert_array_equals(pattern, sequenceVibrateNotification.vibrate); How about remove this line? WDYT? https://codereview.chromium.org/1042513002/diff/320001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:76: assert_array_equals([1000], vibrateNotification.vibrate); On 2015/04/09 13:03:17, Peter Beverloo wrote: > nit: In testharness, the argument order for assertions is assert(actual, > expected); > > This should thus read: > assert_array_equals(vibrateNotification.vibrate, [1000]); > > Dito regarding the new assertions below. Done. https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... File Source/modules/notifications/Notification.h (right): https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... Source/modules/notifications/Notification.h:103: static Vector<unsigned> sanitizeVibrationPattern(const UnsignedLongOrUnsignedLongSequence&); On 2015/04/09 13:03:17, Peter Beverloo wrote: > I don't think this should be the responsibility of anything in > modules/notifications/, not least of all because it doesn't implement the > specification or any of the sanity checks. > > The NavigatorVibration::vibrate() method implements the sanitation algorithm as > specified by the Vibration API. Why don't you extract that to a static method > and use it here (and in SWRNotifications)? > > http://dev.w3.org/2009/dap/vibration/#dfn-validate-and-normalize If we use sanitation algorithm in Vibration API, we cannot keep user's setting value in vibrate. So I made other things in Notification class. For example, If we use sanitation algorithm in Vibration API, when User set [1000, 2000, 10000, 200] in NotificationOptions's vibrate property, Notification.vibrate will return [1000, 2000, 10000]. Becuase sanitation algorithm delete last valuse, when vibrate's count is even. Then I thought again, we don't need to keep user's setting no matched with sanitation algorithms. I'll fix to use Vibration API's sanitation algorithms. https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp (right): https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:70: return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(scriptState->isolate(), "Slient notification must not specify vibration patterns")); On 2015/04/09 13:03:17, Peter Beverloo wrote: > nit: Sentences end with a period. Done. https://codereview.chromium.org/1042513002/diff/320001/public/platform/WebVib... File public/platform/WebVibration.h (right): https://codereview.chromium.org/1042513002/diff/320001/public/platform/WebVib... public/platform/WebVibration.h:40: const unsigned kVibrationPatternLengthMax = 99; On 2015/04/09 13:03:17, Peter Beverloo wrote: > Note that you're exposing this in the Blink API, but aren't actually using it > anywhere new. > > If you group all vibration pattern sanitation to NavigatorVibration then it can > remain file-private to that file, and you don't have to touch this part of the > API. Okay, I'll remove this.
https://codereview.chromium.org/1042513002/diff/320001/LayoutTests/http/tests... File LayoutTests/http/tests/notifications/notification-properties.html (right): https://codereview.chromium.org/1042513002/diff/320001/LayoutTests/http/tests... LayoutTests/http/tests/notifications/notification-properties.html:35: assert_equals(notification.vibrate, null); On 2015/04/09 13:48:05, Sanghyun Park wrote: > On 2015/04/09 13:03:17, Peter Beverloo wrote: > > nit: Please add a value to |options| for this assert, like the other ones. > > Currently options.silent set true in this. > If vibrate is set by any values, typeError is occurred.. > > And if I change options.silent value to "false", this page does not have test > for checking silent. > So, I made other test for vibrate in line 66. > + var vibrateNotification = new Notification("My Notification", { > + vibrate: 1000 > + }); > + assert_array_equals(pattern, sequenceVibrateNotification.vibrate); > > How about remove this line? > > WDYT? That seems good to me! https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... File Source/modules/notifications/Notification.h (right): https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... Source/modules/notifications/Notification.h:103: static Vector<unsigned> sanitizeVibrationPattern(const UnsignedLongOrUnsignedLongSequence&); On 2015/04/09 13:48:05, Sanghyun Park wrote: > On 2015/04/09 13:03:17, Peter Beverloo wrote: > > I don't think this should be the responsibility of anything in > > modules/notifications/, not least of all because it doesn't implement the > > specification or any of the sanity checks. > > > > The NavigatorVibration::vibrate() method implements the sanitation algorithm > as > > specified by the Vibration API. Why don't you extract that to a static method > > and use it here (and in SWRNotifications)? > > > > http://dev.w3.org/2009/dap/vibration/#dfn-validate-and-normalize > > If we use sanitation algorithm in Vibration API, we cannot keep user's setting > value in vibrate. > So I made other things in Notification class. > > For example, If we use sanitation algorithm in Vibration API, when User set > [1000, 2000, 10000, 200] in NotificationOptions's vibrate property, > Notification.vibrate will return [1000, 2000, 10000]. > Becuase sanitation algorithm delete last valuse, when vibrate's count is even. > > Then I thought again, we don't need to keep user's setting no matched with > sanitation algorithms. > > I'll fix to use Vibration API's sanitation algorithms. Yes, and that's exactly the point :-). We don't have to return exactly what the developer provided - we don't do that in some other cases either (e.g. directionality and the icon URL), as long as it results in a valid value representing the same thing. Thanks for moving to using Vibration's sanitation algorithm.
https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... File Source/modules/notifications/Notification.h (right): https://codereview.chromium.org/1042513002/diff/320001/Source/modules/notific... Source/modules/notifications/Notification.h:103: static Vector<unsigned> sanitizeVibrationPattern(const UnsignedLongOrUnsignedLongSequence&); On 2015/04/09 13:53:07, Peter Beverloo wrote: > On 2015/04/09 13:48:05, Sanghyun Park wrote: > > On 2015/04/09 13:03:17, Peter Beverloo wrote: > > > I don't think this should be the responsibility of anything in > > > modules/notifications/, not least of all because it doesn't implement the > > > specification or any of the sanity checks. > > > > > > The NavigatorVibration::vibrate() method implements the sanitation algorithm > > as > > > specified by the Vibration API. Why don't you extract that to a static > method > > > and use it here (and in SWRNotifications)? > > > > > > http://dev.w3.org/2009/dap/vibration/#dfn-validate-and-normalize > > > > If we use sanitation algorithm in Vibration API, we cannot keep user's setting > > value in vibrate. > > So I made other things in Notification class. > > > > For example, If we use sanitation algorithm in Vibration API, when User set > > [1000, 2000, 10000, 200] in NotificationOptions's vibrate property, > > Notification.vibrate will return [1000, 2000, 10000]. > > Becuase sanitation algorithm delete last valuse, when vibrate's count is even. > > > > Then I thought again, we don't need to keep user's setting no matched with > > sanitation algorithms. > > > > I'll fix to use Vibration API's sanitation algorithms. > > Yes, and that's exactly the point :-). We don't have to return exactly what the > developer provided - we don't do that in some other cases either (e.g. > directionality and the icon URL), as long as it results in a valid value > representing the same thing. > > Thanks for moving to using Vibration's sanitation algorithm. Thanks for letting me know about this, too. :)
Dear Peter, Please Take Another Look, when you have time. :)
thanks Sanghyun for the vibration refactoring looks much better now, just a few questions/comments below. https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... Source/modules/notifications/Notification.cpp:39: #include "bindings/modules/v8/UnionTypesModules.h" is this still needed? https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... File Source/modules/notifications/Notification.idl (right): https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... Source/modules/notifications/Notification.idl:59: [RuntimeEnabled=NotificationExperimental] readonly attribute sequence<unsigned long>? vibrate; why is "sequence<unsigned long>" used here and "unsigned long or sequence<unsigned long>" in NotificationOptions.idl? is this intentional? https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp (right): https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:71: return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(scriptState->isolate(), "Slient notification must not specify vibration patterns.")); Slient -> Silent ? https://codereview.chromium.org/1042513002/diff/340001/Source/modules/vibrati... File Source/modules/vibration/NavigatorVibration.h (right): https://codereview.chromium.org/1042513002/diff/340001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.h:58: static VibrationPattern sanitizeVibrationPattern(const VibrationPattern&); does this method have to be public? if not, maybe you can hide it completely and put this in an anonymous namespace inside NavigatorVibration.cpp
Dear Tim. Thank you for review. :) https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... Source/modules/notifications/Notification.cpp:39: #include "bindings/modules/v8/UnionTypesModules.h" On 2015/04/13 11:55:18, timvolodine wrote: > is this still needed? Oops, This is my fault. I'll remove.. https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... File Source/modules/notifications/Notification.idl (right): https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... Source/modules/notifications/Notification.idl:59: [RuntimeEnabled=NotificationExperimental] readonly attribute sequence<unsigned long>? vibrate; On 2015/04/13 11:55:18, timvolodine wrote: > why is "sequence<unsigned long>" used here and "unsigned long or > sequence<unsigned long>" in NotificationOptions.idl? is this intentional? It has not been decided how to expose the vibration attribute back to the developer in spec[1]. I and Peter were decided to support only sequence type . If you want to know details, please refer the Peter's comment :) [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682. [2] https://codereview.chromium.org/1042513002/patch/240001/250007 https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp (right): https://codereview.chromium.org/1042513002/diff/340001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:71: return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(scriptState->isolate(), "Slient notification must not specify vibration patterns.")); On 2015/04/13 11:55:19, timvolodine wrote: > Slient -> Silent ? Thank you I'll fix. https://codereview.chromium.org/1042513002/diff/340001/Source/modules/vibrati... File Source/modules/vibration/NavigatorVibration.h (right): https://codereview.chromium.org/1042513002/diff/340001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.h:58: static VibrationPattern sanitizeVibrationPattern(const VibrationPattern&); On 2015/04/13 11:55:19, timvolodine wrote: > does this method have to be public? > if not, maybe you can hide it completely and put this in an anonymous namespace > inside NavigatorVibration.cpp Okay, I'll remove this.
Patchset #4 (id:280001) has been deleted
Patchset #6 (id:360001) has been deleted
notifications lgtm % the following nits. Please be sure to get Tim's approval for Vibration. https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... Source/modules/notifications/Notification.cpp:82: exceptionState.throwTypeError("Silent notification must not specify vibration patterns."); nit: notification -> notifications https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp (right): https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:71: return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(scriptState->isolate(), "Silent notification must not specify vibration patterns.")); nit: notification -> notifications https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:96: WebNotificationData notification(title, dir, options.lang(), options.body(), options.tag(), iconUrl, NavigatorVibration::sanitizeVibrationPattern(options.vibrate()), options.silent(), dataAsWireBytes); nit: Please extract this into a variable to improve readability.
Thank you!!! I'll fix this . :)) https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... Source/modules/notifications/Notification.cpp:82: exceptionState.throwTypeError("Silent notification must not specify vibration patterns."); On 2015/04/13 15:05:18, Peter Beverloo wrote: > nit: notification -> notifications Done. https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp (right): https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:71: return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(scriptState->isolate(), "Silent notification must not specify vibration patterns.")); On 2015/04/13 15:05:18, Peter Beverloo wrote: > nit: notification -> notifications Done. https://codereview.chromium.org/1042513002/diff/380001/Source/modules/notific... Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:96: WebNotificationData notification(title, dir, options.lang(), options.body(), options.tag(), iconUrl, NavigatorVibration::sanitizeVibrationPattern(options.vibrate()), options.silent(), dataAsWireBytes); On 2015/04/13 15:05:18, Peter Beverloo wrote: > nit: Please extract this into a variable to improve readability. Okay I'll make variable about vibrate.
Dear Tim. Please take another look about vibration part. Thanks
somewhat restating my previous comment about the anonymous namespace, vibration lgtm with the comments below addressed. thanks! https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:33: const unsigned kVibrationPatternLengthMax = 99; btw this can probably also go into the anonymous namespace https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:35: static NavigatorVibration::VibrationPattern sanitizeVibrationPatternInternal(const NavigatorVibration::VibrationPattern& pattern) no need for 'static' here, and why not put this method in an anonymous namespace?
https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:33: const unsigned kVibrationPatternLengthMax = 99; On 2015/04/16 12:38:01, timvolodine wrote: > btw this can probably also go into the anonymous namespace (Constants are file-local by default, no need for them to be in an anonymous namespace. This also goes for strings and all.) https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:35: static NavigatorVibration::VibrationPattern sanitizeVibrationPatternInternal(const NavigatorVibration::VibrationPattern& pattern) On 2015/04/16 12:38:01, timvolodine wrote: > no need for 'static' here, and why not put this method in an anonymous > namespace? +1, please use an anonymous namespace for the function.
Dear Tim and Peter. Thanks for your comments. :) https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:33: const unsigned kVibrationPatternLengthMax = 99; On 2015/04/16 12:46:41, Peter Beverloo wrote: > On 2015/04/16 12:38:01, timvolodine wrote: > > btw this can probably also go into the anonymous namespace > > (Constants are file-local by default, no need for them to be in an anonymous > namespace. This also goes for strings and all.) This constants is used in sanitizedVibrationPatternInternal function. I think this also should go into anonymous namespace for using this value in sanitizedVibrationPatternInternal. WDYT? https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:35: static NavigatorVibration::VibrationPattern sanitizeVibrationPatternInternal(const NavigatorVibration::VibrationPattern& pattern) On 2015/04/16 12:46:41, Peter Beverloo wrote: > On 2015/04/16 12:38:01, timvolodine wrote: > > no need for 'static' here, and why not put this method in an anonymous > > namespace? > > +1, please use an anonymous namespace for the function. I just want to avoid using same blink namespace in this function. I'll remove 'static' and move to anonymous space.
https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... Source/modules/vibration/NavigatorVibration.cpp:35: static NavigatorVibration::VibrationPattern sanitizeVibrationPatternInternal(const NavigatorVibration::VibrationPattern& pattern) On 2015/04/16 12:46:41, Peter Beverloo wrote: > On 2015/04/16 12:38:01, timvolodine wrote: > > no need for 'static' here, and why not put this method in an anonymous > > namespace? > > +1, please use an anonymous namespace for the function. yes, and just to clarify: I meant you can drop static if you use an anonymous namespace ;)
On 2015/04/16 13:17:30, Sanghyun Park wrote: > Dear Tim and Peter. > > Thanks for your comments. :) > > https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... > File Source/modules/vibration/NavigatorVibration.cpp (right): > > https://codereview.chromium.org/1042513002/diff/400001/Source/modules/vibrati... > Source/modules/vibration/NavigatorVibration.cpp:33: const unsigned > kVibrationPatternLengthMax = 99; > On 2015/04/16 12:46:41, Peter Beverloo wrote: > > On 2015/04/16 12:38:01, timvolodine wrote: > > > btw this can probably also go into the anonymous namespace > > > > (Constants are file-local by default, no need for them to be in an anonymous > > namespace. This also goes for strings and all.) > > This constants is used in sanitizedVibrationPatternInternal function. > I think this also should go into anonymous namespace for using this value in > sanitizedVibrationPatternInternal. > > WDYT? Sure. SGTM
Patchset #8 (id:420001) has been deleted
Patchset #8 (id:440001) has been deleted
Patchset #8 (id:460001) has been deleted
Patchset #8 (id:480001) has been deleted
Patchset #8 (id:500001) has been deleted
Patchset #9 (id:540001) has been deleted
Patchset #8 (id:520001) has been deleted
Dear Peter and Tim. Thanks for review about this. NavigatorVibration files were not on LGTM even though Tim give LGTM to me . Do I need Michael's review also? Thanks
Patchset #9 (id:580001) has been deleted
Patchset #8 (id:560001) has been deleted
lgtm
On 2015/04/17 11:28:01, Michael van Ouwerkerk wrote: > lgtm Thanks all!!
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, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/1042513002/#ps600001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1042513002/600001
Message was sent while issue was closed.
Committed patchset #8 (id:600001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193953 |
