Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16781002:
Vibration API: plumbing from Blink to Java. (Closed)
|
Created:
7 years, 6 months ago by Michael van Ouwerkerk Modified:
7 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jamesr Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionVibration API: plumbing from Blink to Java.
Intent to implement & ship thread:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/vibration$20api/blink-dev/hH9bJGWKAbk/AFPov-g5VMMJ
BUG=222504
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212670
Patch Set 1 #
Total comments: 38
Patch Set 2 : Address review comments. #Patch Set 3 : rebase #
Total comments: 28
Patch Set 4 : Address review comments. #Patch Set 5 : Use 'unsigned int' everywhere. #
Total comments: 6
Patch Set 6 : Lazily instantiate VibrationService in VibrationMessageFilter. #
Total comments: 18
Patch Set 7 : Use int32_t. #Patch Set 8 : Ensure duration is greater than 0. #Patch Set 9 : Rebase. #Patch Set 10 : Use int32 instead of int32_t because windows broke. #Patch Set 11 : . #
Total comments: 2
Patch Set 12 : Use int64_t. #Patch Set 13 : Use int64, not int64_t. #
Total comments: 13
Patch Set 14 : Alternative approach to multi platform. #Patch Set 15 : Fold VibrationService into VibrationMessageFilter. #
Total comments: 4
Patch Set 16 : Set java pointer in OnVibrate. #Patch Set 17 : Explicitly permit including WebVibration from content/browser/ #
Total comments: 6
Patch Set 18 : Move vibration_message_filter.(h|cc) to content/browser/android/. #
Total comments: 1
Patch Set 19 : Rebase. #Patch Set 20 : Rebase. #Messages
Total messages: 40 (0 generated)
Hi Peter, could you take a look please? Thanks!
I don't like the factory-like implementation you implemented in here, it's uncommon and not necessary. Maybe you can have the VibrationService class define a static Create() method which returns a VibrationService*, and implement that in the platform specific files? You'd still need an ifdef before calling the Create() method though, and have to be able to gracefully handle NULL.. https://codereview.chromium.org/16781002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/16781002/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_process_host_impl.cc:715: channel_->AddFilter(new VibrationBrowserMessageFilter(NewVibrationService())); It seems odd for the message filter to own the vibration service. Is this a common practice? Looking at the calls above, it isn't. It'd be good to know how, for example, the Gamepad message filter does this. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_browser_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.cc:14: const unsigned kVibrationDurationMax = 10000; kMaximumVibrationDurationMs https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.cc:19: // anyway so we can decide whether to expose navigator.vibrate or not. I think this reasoning would be better stored in a bug. Would something like the following work for you? // TODO(mvanouwerkerk): Consider instantiating this lazily. See crbug.com/XXXXX https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_browser_message_filter.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. This file already is in content/browser/, I'd say we should skip "Browser" in the class' name and just name it VibrationMessageFilter (unless you need a renderer-side message filter too)? https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.h:14: class RenderProcessHost; nit: Unused forward declaration. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.h:18: VibrationBrowserMessageFilter(scoped_ptr<VibrationService> vibration_service); nit: Pass the scoped_ptr<> as a reference, if you decide to keep ownership here. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.h:27: void OnVibrate(unsigned time); nit: maybe naming the time argument "milliseconds" might be clearer to what the contents will be? https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service.h:12: virtual ~VibrationService() {} nit: empty line below this one. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service.h:13: virtual void Vibrate(unsigned time) = 0; Since this is an empty interface, let's add some comments to clarify what the expected behavior is (like the Platform API: units, what happens when a vibration is already active, can CancelVibration() be called when no vibration is active?) https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service_factory.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_factory.h:11: #include "content/browser/vibration/vibration_service_impl_empty.h" Do we need an empty implementation? Can't we just return NULL if there is no appropriate implementation and then not handle it in the message filter? Having a factory seems a bit overkill, and is different from the normal paradigms we use for supporting multiple platforms. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service_impl_android.cc (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.cc:13: content::Java_VibrationService_create( nit: we're in content, the "content::" namespace prefix is not necessary. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.cc:14: base::android::AttachCurrentThread(), nit: you use base::android::AttachCurrentThread() three times. Would it warrant a using statement at the top? https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.cc:22: Java_VibrationService_vibrate( nit: indenting. You can indent this line like the more common: Java_VibrationService_vibrate(base::android::AttachCurrentThread(), java_vibration_service_.obj(), time); https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.cc:29: Java_VibrationService_cancelVibration( dito https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service_impl_android.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.h:18: virtual void Vibrate(unsigned time) OVERRIDE; nit: empty line above here, perhaps a comment to clarify that these implement the VibrationService? https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.h:22: base::android::ScopedJavaGlobalRef<jobject> java_vibration_service_; j_vibration_service_ https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service_impl_empty.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_empty.h:13: class VibrationServiceImplEmpty : public VibrationService { As said earlier on, let's not have this. https://codereview.chromium.org/16781002/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/VibrationService.java (right): https://codereview.chromium.org/16781002/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:16: private Vibrator mVibrator; This can be final. https://codereview.chromium.org/16781002/diff/1/content/renderer/renderer_web... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/16781002/diff/1/content/renderer/renderer_web... content/renderer/renderer_webkitplatformsupport_impl.h:123: virtual void vibrate(unsigned time) OVERRIDE; nit: white line above here to improve readability.
Thanks for the review! Please take another look. https://codereview.chromium.org/16781002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/16781002/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_process_host_impl.cc:715: channel_->AddFilter(new VibrationBrowserMessageFilter(NewVibrationService())); On 2013/06/13 11:27:45, Peter Beverloo wrote: > It seems odd for the message filter to own the vibration service. Is this a > common practice? Looking at the calls above, it isn't. It'd be good to know > how, for example, the Gamepad message filter does this. Most of the above code gets the dependencies from storage_partition_impl_. I don't think VibrationService belongs there. I don't see anything particularly wrong with VibrationBrowserMessageFilter owning the service, nothing else needs to access it so the ownership makes sense. Weaker alternatives: * the filter instantiates the service, does not receive it in its constructor * the service is a singleton and provides and owns an instance https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_browser_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.cc:14: const unsigned kVibrationDurationMax = 10000; On 2013/06/13 11:27:45, Peter Beverloo wrote: > kMaximumVibrationDurationMs Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.cc:19: // anyway so we can decide whether to expose navigator.vibrate or not. On 2013/06/13 11:27:45, Peter Beverloo wrote: > I think this reasoning would be better stored in a bug. Would something like the > following work for you? > > // TODO(mvanouwerkerk): Consider instantiating this lazily. See crbug.com/XXXXX Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_browser_message_filter.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/06/13 11:27:45, Peter Beverloo wrote: > This file already is in content/browser/, I'd say we should skip "Browser" in > the class' name and just name it VibrationMessageFilter (unless you need a > renderer-side message filter too)? Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.h:14: class RenderProcessHost; On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: Unused forward declaration. Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.h:18: VibrationBrowserMessageFilter(scoped_ptr<VibrationService> vibration_service); On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: Pass the scoped_ptr<> as a reference, if you decide to keep ownership here. Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_browser_message_filter.h:27: void OnVibrate(unsigned time); On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: maybe naming the time argument "milliseconds" might be clearer to what the > contents will be? Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service.h:12: virtual ~VibrationService() {} On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: empty line below this one. Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service.h:13: virtual void Vibrate(unsigned time) = 0; On 2013/06/13 11:27:45, Peter Beverloo wrote: > Since this is an empty interface, let's add some comments to clarify what the > expected behavior is (like the Platform API: units, what happens when a > vibration is already active, can CancelVibration() be called when no vibration > is active?) Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service_factory.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_factory.h:11: #include "content/browser/vibration/vibration_service_impl_empty.h" On 2013/06/13 11:27:45, Peter Beverloo wrote: > Do we need an empty implementation? Can't we just return NULL if there is no > appropriate implementation and then not handle it in the message filter? Having > a factory seems a bit overkill, and is different from the normal paradigms we > use for supporting multiple platforms. Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service_impl_android.cc (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.cc:13: content::Java_VibrationService_create( On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: we're in content, the "content::" namespace prefix is not necessary. Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.cc:14: base::android::AttachCurrentThread(), On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: you use base::android::AttachCurrentThread() three times. Would it warrant > a using statement at the top? Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.cc:22: Java_VibrationService_vibrate( On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: indenting. You can indent this line like the more common: > > Java_VibrationService_vibrate(base::android::AttachCurrentThread(), > java_vibration_service_.obj(), > time); Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.cc:29: Java_VibrationService_cancelVibration( On 2013/06/13 11:27:45, Peter Beverloo wrote: > dito Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service_impl_android.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.h:18: virtual void Vibrate(unsigned time) OVERRIDE; On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: empty line above here, perhaps a comment to clarify that these implement > the VibrationService? Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_android.h:22: base::android::ScopedJavaGlobalRef<jobject> java_vibration_service_; On 2013/06/13 11:27:45, Peter Beverloo wrote: > j_vibration_service_ Done. https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... File content/browser/vibration/vibration_service_impl_empty.h (right): https://codereview.chromium.org/16781002/diff/1/content/browser/vibration/vib... content/browser/vibration/vibration_service_impl_empty.h:13: class VibrationServiceImplEmpty : public VibrationService { On 2013/06/13 11:27:45, Peter Beverloo wrote: > As said earlier on, let's not have this. Done. https://codereview.chromium.org/16781002/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/VibrationService.java (right): https://codereview.chromium.org/16781002/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:16: private Vibrator mVibrator; On 2013/06/13 11:27:45, Peter Beverloo wrote: > This can be final. Done. https://codereview.chromium.org/16781002/diff/1/content/renderer/renderer_web... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/16781002/diff/1/content/renderer/renderer_web... content/renderer/renderer_webkitplatformsupport_impl.h:123: virtual void vibrate(unsigned time) OVERRIDE; On 2013/06/13 11:27:45, Peter Beverloo wrote: > nit: white line above here to improve readability. Done.
Marcus, could you please take a look at this patch? I'm working my way up the OWNERS food chain :-)
nice, thanks! just a few nits and one small suggestion for the factory function location: https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:14: const unsigned kMaximumVibrationDurationMs = 10000; nit: int https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:5: #ifndef CONTENT_VIBRATION_VIBRATION_MESSAGE_FILTER_H_ nit: CONTENT_BROWSER_... https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:16: VibrationMessageFilter(VibrationService* vibration_service); nit: explicit. also, since this is taking ownership, scoped_ptr<VibrationService> vibration_service https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:25: void OnVibrate(unsigned milliseconds); int? https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service.cc (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service.cc:16: #if defined(OS_ANDROID) probably better to #if !defined(OS_ANDROID) the whole function, remove from 8-10, and then just have this implementation in the android itself. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service.h (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service.h:23: virtual void CancelVibration() = 0; nit: private: DISALLOW_COPY_AND_ASSIGN(VibrationService); https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service_impl_android.cc (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service_impl_android.cc:14: j_vibration_service_.Reset( nit: unindent https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service_impl_android.h (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service_impl_android.h:24: base::android::ScopedJavaGlobalRef<jobject> j_vibration_service_; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service_impl_android.h:27: bool RegisterVibrationService(JNIEnv* env); make it a static function in the class itself, and just call it "Register" https://codereview.chromium.org/16781002/diff/21001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/VibrationService.java (right): https://codereview.chromium.org/16781002/diff/21001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:13: @JNINamespace("content") nit: /** * This is the implementation of the C++ counterpart VibrationService. */ https://codereview.chromium.org/16781002/diff/21001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:25: mVibrator.vibrate(milliseconds); should we have some kind of throttling at this level? the 10s max is good, but how about some kind of exponential backoff to prevent pages from constantly buzzing the device? https://codereview.chromium.org/16781002/diff/21001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:29: public void cancelVibration() { nit: make these whole lot private.. we tend to use the java qualifiers just for java code..
Thanks for the quick review Marcus! Please take another look. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:14: const unsigned kMaximumVibrationDurationMs = 10000; On 2013/06/20 14:21:04, bulach wrote: > nit: int Why int? Did you mean 'unsigned int'? The vibration duration is an 'unsigned' from Blink up to Java. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:5: #ifndef CONTENT_VIBRATION_VIBRATION_MESSAGE_FILTER_H_ On 2013/06/20 14:21:04, bulach wrote: > nit: CONTENT_BROWSER_... Done. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:16: VibrationMessageFilter(VibrationService* vibration_service); On 2013/06/20 14:21:04, bulach wrote: > nit: explicit. also, since this is taking ownership, > scoped_ptr<VibrationService> vibration_service Done. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:25: void OnVibrate(unsigned milliseconds); On 2013/06/20 14:21:04, bulach wrote: > int? Like in the other comment... could you explain why? It is unsigned all the way through. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service.cc (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service.cc:16: #if defined(OS_ANDROID) On 2013/06/20 14:21:04, bulach wrote: > probably better to #if !defined(OS_ANDROID) the whole function, remove from > 8-10, and then just have this implementation in the android itself. Done. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service.h (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service.h:23: virtual void CancelVibration() = 0; On 2013/06/20 14:21:04, bulach wrote: > nit: > > private: > DISALLOW_COPY_AND_ASSIGN(VibrationService); How is this to be used on interfaces like this? I now get this compile error: vibration_service_impl_android.cc:23:58: error: no matching function for call to 'content::VibrationService::VibrationService()' https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service_impl_android.cc (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service_impl_android.cc:14: j_vibration_service_.Reset( On 2013/06/20 14:21:04, bulach wrote: > nit: unindent Done. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service_impl_android.h (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service_impl_android.h:24: base::android::ScopedJavaGlobalRef<jobject> j_vibration_service_; On 2013/06/20 14:21:04, bulach wrote: > nit: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service_impl_android.h:27: bool RegisterVibrationService(JNIEnv* env); On 2013/06/20 14:21:04, bulach wrote: > make it a static function in the class itself, and just call it "Register" Done. https://codereview.chromium.org/16781002/diff/21001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/VibrationService.java (right): https://codereview.chromium.org/16781002/diff/21001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:13: @JNINamespace("content") On 2013/06/20 14:21:04, bulach wrote: > nit: > /** > * This is the implementation of the C++ counterpart VibrationService. > */ Done. https://codereview.chromium.org/16781002/diff/21001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:25: mVibrator.vibrate(milliseconds); On 2013/06/20 14:21:04, bulach wrote: > should we have some kind of throttling at this level? > the 10s max is good, but how about some kind of exponential backoff to prevent > pages from constantly buzzing the device? It's a bit tricky, because there is a valid use case in games for frequent and / or long vibrations. Ultimately, if the page misbehaves the user can just close it. If we really want to further throttle usage of this service, we have a few options: * Minimum pauses * Maximum amount of vibration over time, e.g. cannot vibrate more than 50% of the time. I'm not convinced we need to throttle it any further, and would be comfortable with sending this out in the Beta release and see if we get any negative feedback on this. https://codereview.chromium.org/16781002/diff/21001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:29: public void cancelVibration() { On 2013/06/20 14:21:04, bulach wrote: > nit: make these whole lot private.. we tend to use the java qualifiers just for > java code.. Done.
clarifications inline: https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:14: const unsigned kMaximumVibrationDurationMs = 10000; On 2013/06/20 17:34:09, Michael van Ouwerkerk wrote: > On 2013/06/20 14:21:04, bulach wrote: > > nit: int > > Why int? Did you mean 'unsigned int'? The vibration duration is an 'unsigned' > from Blink up to Java. sorry, I should've been clearer: "unsigned int" rather than just "unsigned".. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service.h (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service.h:23: virtual void CancelVibration() = 0; On 2013/06/20 17:34:09, Michael van Ouwerkerk wrote: > On 2013/06/20 14:21:04, bulach wrote: > > nit: > > > > private: > > DISALLOW_COPY_AND_ASSIGN(VibrationService); > > How is this to be used on interfaces like this? I now get this compile error: > vibration_service_impl_android.cc:23:58: error: no matching function for call to > 'content::VibrationService::VibrationService()' I guess it requires a default constructor, right above line 12. :)
Thanks Marcus :-) Please take another look. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:14: const unsigned kMaximumVibrationDurationMs = 10000; On 2013/06/20 18:11:47, bulach wrote: > On 2013/06/20 17:34:09, Michael van Ouwerkerk wrote: > > On 2013/06/20 14:21:04, bulach wrote: > > > nit: int > > > > Why int? Did you mean 'unsigned int'? The vibration duration is an 'unsigned' > > from Blink up to Java. > > sorry, I should've been clearer: "unsigned int" rather than just "unsigned".. Done in the whole patch. https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... File content/browser/vibration/vibration_service.h (right): https://codereview.chromium.org/16781002/diff/21001/content/browser/vibration... content/browser/vibration/vibration_service.h:23: virtual void CancelVibration() = 0; On 2013/06/20 18:11:47, bulach wrote: > On 2013/06/20 17:34:09, Michael van Ouwerkerk wrote: > > On 2013/06/20 14:21:04, bulach wrote: > > > nit: > > > > > > private: > > > DISALLOW_COPY_AND_ASSIGN(VibrationService); > > > > How is this to be used on interfaces like this? I now get this compile error: > > vibration_service_impl_android.cc:23:58: error: no matching function for call > to > > 'content::VibrationService::VibrationService()' > > I guess it requires a default constructor, right above line 12. :) Done.
Chris: could you please take a look for view_messages.h ? Avi: could you please take a look for content/ ? Thanks!
lgtm, nice! just some nits, and would really like to see the lazy initialization done now, it doesn't seem too complicated.. https://codereview.chromium.org/16781002/diff/44001/content/browser/renderer_... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/16781002/diff/44001/content/browser/renderer_... content/browser/renderer_host/render_process_host_impl.cc:704: channel_->AddFilter(new VibrationMessageFilter(VibrationService::Create())); see below, I think it won't be too difficult to just address the TODO there to lazily instantiate this and avoid depending on java here.. https://codereview.chromium.org/16781002/diff/44001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/44001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:19: vibration_service_.reset(vibration_service.release()); sorry, I just realized.. this is _always_ going to be created, and it goes up to java, etc, for every renderer process host.. :( since the factory function is at the same level here and takes no arguments, how about just lazily create in this class? https://codereview.chromium.org/16781002/diff/44001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/VibrationService.java (right): https://codereview.chromium.org/16781002/diff/44001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:13: @JNINamespace("content") nit: move this right above the class, after the comments
Thanks Marcus! https://codereview.chromium.org/16781002/diff/44001/content/browser/renderer_... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/16781002/diff/44001/content/browser/renderer_... content/browser/renderer_host/render_process_host_impl.cc:704: channel_->AddFilter(new VibrationMessageFilter(VibrationService::Create())); On 2013/06/21 12:55:54, bulach wrote: > see below, I think it won't be too difficult to just address the TODO there to > lazily instantiate this and avoid depending on java here.. Done. https://codereview.chromium.org/16781002/diff/44001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/44001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:19: vibration_service_.reset(vibration_service.release()); On 2013/06/21 12:55:54, bulach wrote: > sorry, I just realized.. this is _always_ going to be created, and it goes up to > java, etc, for every renderer process host.. :( > > since the factory function is at the same level here and takes no arguments, how > about just lazily create in this class? Done. https://codereview.chromium.org/16781002/diff/44001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/VibrationService.java (right): https://codereview.chromium.org/16781002/diff/44001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:13: @JNINamespace("content") On 2013/06/21 12:55:54, bulach wrote: > nit: move this right above the class, after the comments Done.
https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:36: void VibrationMessageFilter::OnVibrate(unsigned int milliseconds) { int64_t https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:43: milliseconds = std::min(milliseconds, kMaximumVibrationDurationMs); You'll want to ensure milliseconds > 0 as well, since the type is signed. https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:23: void OnVibrate(unsigned int milliseconds); int64_t again https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... File content/browser/vibration/vibration_service.h (right): https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_service.h:24: virtual void Vibrate(unsigned int milliseconds) = 0; int64_t https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... File content/browser/vibration/vibration_service_impl_android.h (right): https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_service_impl_android.h:23: virtual void Vibrate(unsigned int milliseconds) OVERRIDE; int64_t https://codereview.chromium.org/16781002/diff/48002/content/common/view_messa... File content/common/view_messages.h (right): https://codereview.chromium.org/16781002/diff/48002/content/common/view_messa... content/common/view_messages.h:1698: // Sent by the renderer to the browser to start a vibration with the given Should these be guarded by #if defined(OS_ANDROID) ? https://codereview.chromium.org/16781002/diff/48002/content/common/view_messa... content/common/view_messages.h:1701: unsigned int /* milliseconds */) I'd say int64_t here. For best results, use an explicitly-sized integer across the IPC boundary. It's probably not going to be an issue on Android for a long time, but it could be possible that (e.g.) a plugin process runs as 32-bit while the browser process runs as 64-bit. Also, there's always the Java integers (well-defined absolute sizes) vs. C++ integers (well-defined relative sizes) issue. In this exact case, an explicitly-sized integer is mainly for readability. Note also that the style guide wants you to use unsigned integers when the defined behavior of integer overflow is necessary (signed overflow behavior is undefined); but not to use unsigned as a way of asserting that the value will never be negative. Since the Android API for the vibrator needs a Java long, let's consistently use the equivalent C++ type. https://codereview.chromium.org/16781002/diff/48002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/VibrationService.java (right): https://codereview.chromium.org/16781002/diff/48002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:27: private void vibrate(long milliseconds) { The IPC sends unsigned int, but Java long is defined to be signed 64-bit. It feels best to me to use the same type everywhere, i.e. int64_t in the IPC and long in Java. Alternately, take an int here, and send int32_t over IPC. But the Android vibrator wants a long, so let's just use that everywhere. https://codereview.chromium.org/16781002/diff/48002/content/renderer/renderer... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/16781002/diff/48002/content/renderer/renderer... content/renderer/renderer_webkitplatformsupport_impl.h:130: virtual void vibrate(unsigned int milliseconds) OVERRIDE; et c. :)
All done Chris. Please take another look! https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:36: void VibrationMessageFilter::OnVibrate(unsigned int milliseconds) { On 2013/06/21 18:17:40, Chromium Palmer wrote: > int64_t Done. https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:43: milliseconds = std::min(milliseconds, kMaximumVibrationDurationMs); On 2013/06/21 18:17:40, Chromium Palmer wrote: > You'll want to ensure milliseconds > 0 as well, since the type is signed. Done. https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:23: void OnVibrate(unsigned int milliseconds); On 2013/06/21 18:17:40, Chromium Palmer wrote: > int64_t again Done. https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... File content/browser/vibration/vibration_service.h (right): https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_service.h:24: virtual void Vibrate(unsigned int milliseconds) = 0; On 2013/06/21 18:17:40, Chromium Palmer wrote: > int64_t Done. https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... File content/browser/vibration/vibration_service_impl_android.h (right): https://codereview.chromium.org/16781002/diff/48002/content/browser/vibration... content/browser/vibration/vibration_service_impl_android.h:23: virtual void Vibrate(unsigned int milliseconds) OVERRIDE; On 2013/06/21 18:17:40, Chromium Palmer wrote: > int64_t Done. https://codereview.chromium.org/16781002/diff/48002/content/common/view_messa... File content/common/view_messages.h (right): https://codereview.chromium.org/16781002/diff/48002/content/common/view_messa... content/common/view_messages.h:1698: // Sent by the renderer to the browser to start a vibration with the given On 2013/06/21 18:17:40, Chromium Palmer wrote: > Should these be guarded by #if defined(OS_ANDROID) ? This feature is not necessarily restricted to Android, and also it's easier to automate tests this way. https://codereview.chromium.org/16781002/diff/48002/content/common/view_messa... content/common/view_messages.h:1701: unsigned int /* milliseconds */) Ok, so I changed it to int32_t. I think the use of a long in Java is a red herring. We don't want to support such long vibrations anyway, so it seems wasteful to use int64_t. Also, the spec uses 'unsigned long' in IDL (http://dev.w3.org/2009/dap/vibration/#vibration-interface) which seems to translate to 'unsigned' in Blink. I'm casting this to int32_t in renderer_webkitplatformsupport_impl.cc. On 2013/06/21 18:17:40, Chromium Palmer wrote: > I'd say int64_t here. > > For best results, use an explicitly-sized integer across the IPC boundary. It's > probably not going to be an issue on Android for a long time, but it could be > possible that (e.g.) a plugin process runs as 32-bit while the browser process > runs as 64-bit. Also, there's always the Java integers (well-defined absolute > sizes) vs. C++ integers (well-defined relative sizes) issue. In this exact case, > an explicitly-sized integer is mainly for readability. > > Note also that the style guide wants you to use unsigned integers when the > defined behavior of integer overflow is necessary (signed overflow behavior is > undefined); but not to use unsigned as a way of asserting that the value will > never be negative. > > Since the Android API for the vibrator needs a Java long, let's consistently use > the equivalent C++ type. https://codereview.chromium.org/16781002/diff/48002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/VibrationService.java (right): https://codereview.chromium.org/16781002/diff/48002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/VibrationService.java:27: private void vibrate(long milliseconds) { So, I chose an int :-) On 2013/06/21 18:17:40, Chromium Palmer wrote: > The IPC sends unsigned int, but Java long is defined to be signed 64-bit. It > feels best to me to use the same type everywhere, i.e. int64_t in the IPC and > long in Java. > > Alternately, take an int here, and send int32_t over IPC. But the Android > vibrator wants a long, so let's just use that everywhere. https://codereview.chromium.org/16781002/diff/48002/content/renderer/renderer... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/16781002/diff/48002/content/renderer/renderer... content/renderer/renderer_webkitplatformsupport_impl.h:130: virtual void vibrate(unsigned int milliseconds) OVERRIDE; On 2013/06/21 18:17:40, Chromium Palmer wrote: > et c. :) Receiving unsigned int, but casting to in32_t.
https://chromiumcodereview.appspot.com/16781002/diff/61002/content/common/vie... File content/common/view_messages.h (right): https://chromiumcodereview.appspot.com/16781002/diff/61002/content/common/vie... content/common/view_messages.h:1701: int32 /* milliseconds */) I really think you're better off using int64_t (== java's long) consistently everywhere. It makes the whole thing easier to read and reason about, and the "waste" is negligible. Integer types are just plain trouble in C++, and especially across languages. Consistent is simple is good.
Thanks Chris, please take another look! https://chromiumcodereview.appspot.com/16781002/diff/61002/content/common/vie... File content/common/view_messages.h (right): https://chromiumcodereview.appspot.com/16781002/diff/61002/content/common/vie... content/common/view_messages.h:1701: int32 /* milliseconds */) On 2013/06/24 19:07:08, Chromium Palmer wrote: > I really think you're better off using int64_t (== java's long) consistently > everywhere. It makes the whole thing easier to read and reason about, and the > "waste" is negligible. > > Integer types are just plain trouble in C++, and especially across languages. > Consistent is simple is good. The windows bots fail to compile because int64_t is not defined. Is that expected? I can't find a good header to include for defining it. Inside of content/ it seems mostly the pepper code that uses int64_t. Any ideas?
> The windows bots fail to compile because int64_t is not defined. Is that > expected? I can't find a good header to include for defining it. Inside of > content/ it seems mostly the pepper code that uses int64_t. Any ideas? This is my bad; I should have said "int64" (defined for all platforms in base/basictypes.h, which I don't think you need to explicitly include) instead of the ANSI-standard "int64_t". All platforms should build fine with int64. So that you don't have to wait: LGTM once you fix that. Thanks!
Thanks Chris! John, could you take a look please for content/ ?
few comments re VibrationService: -no point in having a creator function if the constructor is public -please read http://dev.chromium.org/developers/design-documents/conventions-and-patterns-... -in particular, this doc (and examples throughout the code) means that you shouldn't use a virtual interface to separate the different platform implementations. just have one header and different cc files for each platform. ok to have them all be empty initially. https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:48: // trust any values passed from the renderer. hmm, where is the sanitization happening there? seems that the constants need to be exported so that they're shared instead of duplicated https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:57: vibration_service_.reset(VibrationService::Create().release()); when would this condition occur? i.e. that a vibration is being cancelled but there's no vibration service? https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:59: if (vibration_service_.get()) what's the point of this line, since you're creating it above? https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:19: virtual bool OnMessageReceived(const IPC::Message& message, nit: just hide this in private https://codereview.chromium.org/16781002/diff/86001/content/renderer/renderer... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/16781002/diff/86001/content/renderer/renderer... content/renderer/renderer_webkitplatformsupport_impl.h:140: virtual void vibrate(unsigned int milliseconds) OVERRIDE; nit: follow the convention in this file and don't use OVERRIDE. we don't use that for methods that override interfaces in webkit because otherwise it would take two sided patches to change webkit apis.
one more point: not sure what's the point of having VibrationService which is separate from the message filter? i.e. usually we do that when the service is a singleton, but in this case there's one per filter which needs it.. so you can just fold that code in the message filter (by declaring the methods in the header and having vibration_message_filter_android.cc, vibration_message_filter_win.cc etc for the platform specific methods)
Thanks John, please take another look. Regarding having a separate service from the filter, the idea was to allow for lazy instantiation. https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:48: // trust any values passed from the renderer. On 2013/07/01 18:14:40, jam wrote: > hmm, where is the sanitization happening there? seems that the constants need to > be exported so that they're shared instead of duplicated It's in third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp . Is there an elegant way of sharing these constants without ending up with incorrect dependencies or layering violations? https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:57: vibration_service_.reset(VibrationService::Create().release()); On 2013/07/01 18:14:40, jam wrote: > when would this condition occur? i.e. that a vibration is being cancelled but > there's no vibration service? I've changed this. The implementation was such that non-Android builds always had an empty pointer. https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:59: if (vibration_service_.get()) On 2013/07/01 18:14:40, jam wrote: > what's the point of this line, since you're creating it above? I've changed this. The implementation was such that non-Android builds always had an empty pointer. https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:19: virtual bool OnMessageReceived(const IPC::Message& message, On 2013/07/01 18:14:40, jam wrote: > nit: just hide this in private I don't mind either way, but could you help me understand why? OnMessageReceived is public in BrowserMessageFilter and all subclasses I looked at. Are they all wrong? https://codereview.chromium.org/16781002/diff/86001/content/renderer/renderer... File content/renderer/renderer_webkitplatformsupport_impl.h (right): https://codereview.chromium.org/16781002/diff/86001/content/renderer/renderer... content/renderer/renderer_webkitplatformsupport_impl.h:140: virtual void vibrate(unsigned int milliseconds) OVERRIDE; On 2013/07/01 18:14:40, jam wrote: > nit: follow the convention in this file and don't use OVERRIDE. we don't use > that for methods that override interfaces in webkit because otherwise it would > take two sided patches to change webkit apis. Done.
On 2013/07/02 17:52:38, Michael van Ouwerkerk wrote: > Thanks John, please take another look. > > Regarding having a separate service from the filter, the idea was to allow for > lazy instantiation. I don't follow? You can lazy initialize j_vibration_service_ while still being in VibrationMessageFilter instead of another class. https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.cc:48: // trust any values passed from the renderer. On 2013/07/02 17:52:39, Michael van Ouwerkerk wrote: > On 2013/07/01 18:14:40, jam wrote: > > hmm, where is the sanitization happening there? seems that the constants need > to > > be exported so that they're shared instead of duplicated > > It's in third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp . Is > there an elegant way of sharing these constants without ending up with incorrect > dependencies or layering violations? you can put them in a public file in the WebKit API. jamesr can tell you exactly which one https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:19: virtual bool OnMessageReceived(const IPC::Message& message, On 2013/07/02 17:52:39, Michael van Ouwerkerk wrote: > On 2013/07/01 18:14:40, jam wrote: > > nit: just hide this in private > > I don't mind either way, but could you help me understand why? OnMessageReceived > is public in BrowserMessageFilter and all subclasses I looked at. Are they all > wrong? in general better to hide members if they don't need to be public
Thanks John. I'll ask James about sharing the constants. Please take another look. https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/86001/content/browser/vibration... content/browser/vibration/vibration_message_filter.h:19: virtual bool OnMessageReceived(const IPC::Message& message, On 2013/07/02 22:11:15, jam wrote: > On 2013/07/02 17:52:39, Michael van Ouwerkerk wrote: > > On 2013/07/01 18:14:40, jam wrote: > > > nit: just hide this in private > > > > I don't mind either way, but could you help me understand why? > OnMessageReceived > > is public in BrowserMessageFilter and all subclasses I looked at. Are they all > > wrong? > > in general better to hide members if they don't need to be public Done.
btw you can also ask other blink owners or devs for where to put this. there should be other examples Question: is this only going to be implemented on phones (i.e. cause they're the only devices with these motors)? If so, perhaps all this code should be ifdef'd for android, i.e. all the code in browesr, renderer, and the webkit embedder interfaces? https://codereview.chromium.org/16781002/diff/108001/content/browser/vibratio... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/108001/content/browser/vibratio... content/browser/vibration/vibration_message_filter.cc:77: void VibrationMessageFilter::initializeJava() { why do you need this method? i.e. won't OnVibrate always be called first? so we can do the lazy initialization there? https://codereview.chromium.org/16781002/diff/108001/content/browser/vibratio... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/108001/content/browser/vibratio... content/browser/vibration/vibration_message_filter.h:33: void initializeJava(); nit: chrome style is InitializeJava
Regarding sharing constants, I think it's kind of overkill to set that up. There is only one constant to be shared (maximum vibration duration). We'd need a new file, something like third_party/WebKit/public/platform/VibrationConstants.h just for that one line. Regarding using an ifdef. We try not to use any ifdefs anymore in Blink. The parts in Chromium that are not currently ifdef'd are minimal. I think it will also be easier to run tests this way using mocks / fakes. Finally, in addition to Android phones and tablets, we may support Windows tablets in the future. I hope this helps explain the approach I have taken in this CL. Thanks for your feedback John! https://codereview.chromium.org/16781002/diff/108001/content/browser/vibratio... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/108001/content/browser/vibratio... content/browser/vibration/vibration_message_filter.cc:77: void VibrationMessageFilter::initializeJava() { On 2013/07/03 16:51:55, jam wrote: > why do you need this method? i.e. won't OnVibrate always be called first? so we > can do the lazy initialization there? Done. It does look like it's safe to assume cancel is never called first, as NavigatorVibration.cpp checks whether it is currently vibrating before sending the cancel message. https://codereview.chromium.org/16781002/diff/108001/content/browser/vibratio... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/108001/content/browser/vibratio... content/browser/vibration/vibration_message_filter.h:33: void initializeJava(); On 2013/07/03 16:51:55, jam wrote: > nit: chrome style is InitializeJava Obsolete as this method was removed.
On 2013/07/10 15:24:26, Michael van Ouwerkerk wrote: > Regarding sharing constants, I think it's kind of overkill to set that up. There > is only one constant to be shared (maximum vibration duration). We'd need a new > file, something like third_party/WebKit/public/platform/VibrationConstants.h > just for that one line. > > Regarding using an ifdef. We try not to use any ifdefs anymore in Blink. ok, but I was referring to the parts in chromium and not blink (sorry if not clear) > The > parts in Chromium that are not currently ifdef'd are minimal. It seems that all the parts in chromium could be ifdef'd out completely, i.e. the new files that you're adding and the overrides in renderer_webkitplatformsupport_impl > I think it will > also be easier to run tests this way using mocks / fakes. What's the point of writing tests using mocks/fakes that run on all other platforms when those platforms don't support this feature? > Finally, in addition > to Android phones and tablets, we may support Windows tablets in the future. When we support other platforms in the future, then we can expand the ifdef's scopes
On 2013/07/10 15:24:26, Michael van Ouwerkerk wrote: > Regarding sharing constants, I think it's kind of overkill to set that up. There > is only one constant to be shared (maximum vibration duration). We'd need a new > file, something like third_party/WebKit/public/platform/VibrationConstants.h > just for that one line. I don't think it's overkill to create a new header. Duplicating a constant is wrong.
Hi John, I think I have addressed your concerns. Please take another look. This is the CL that defines the shared constant in WebVibration.h: https://codereview.chromium.org/19041004/
lgtm with changes https://codereview.chromium.org/16781002/diff/128001/content/browser/vibratio... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/128001/content/browser/vibratio... content/browser/vibration/vibration_message_filter.cc:22: const int64 kMinimumVibrationDurationMs = 1; should this go with the max value in the webkit header? https://codereview.chromium.org/16781002/diff/128001/content/browser/vibratio... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/128001/content/browser/vibratio... content/browser/vibration/vibration_message_filter.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. since this file is android only, put this in content/browser/android/ also remove all the OS_ANDROID checks https://codereview.chromium.org/16781002/diff/128001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/16781002/diff/128001/content/common/view_mess... content/common/view_messages.h:1702: #if defined(OS_ANDROID) there's one OS_ANDROID section below for android-specific ViewHostMsg IPCs. put these ones inside of it
Thanks John! https://codereview.chromium.org/16781002/diff/128001/content/browser/vibratio... File content/browser/vibration/vibration_message_filter.cc (right): https://codereview.chromium.org/16781002/diff/128001/content/browser/vibratio... content/browser/vibration/vibration_message_filter.cc:22: const int64 kMinimumVibrationDurationMs = 1; On 2013/07/12 00:12:04, jam wrote: > should this go with the max value in the webkit header? I think not for the moment, as we're using unsigned ints in that header and all the Blink code for Vibration. It's not necessary to check the minimum there. https://codereview.chromium.org/16781002/diff/128001/content/browser/vibratio... File content/browser/vibration/vibration_message_filter.h (right): https://codereview.chromium.org/16781002/diff/128001/content/browser/vibratio... content/browser/vibration/vibration_message_filter.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/07/12 00:12:04, jam wrote: > since this file is android only, put this in content/browser/android/ > > also remove all the OS_ANDROID checks Done. https://codereview.chromium.org/16781002/diff/128001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/16781002/diff/128001/content/common/view_mess... content/common/view_messages.h:1702: #if defined(OS_ANDROID) On 2013/07/12 00:12:04, jam wrote: > there's one OS_ANDROID section below for android-specific ViewHostMsg IPCs. put > these ones inside of it Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/16781002/13...
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Hi Adam, could you take a look at this one please? It looks like I'll need an
lgtm for the DEPS change:
Missing LGTM from OWNERS of directories added to DEPS:
'+third_party/WebKit/public/platform',
https://codereview.chromium.org/16781002/diff/137001/content/browser/DEPS File content/browser/DEPS (right): https://codereview.chromium.org/16781002/diff/137001/content/browser/DEPS#new... content/browser/DEPS:48: "+third_party/WebKit/public/platform/WebVibration.h", LGTM. You should CC jamesr if you want to be doubly sure.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/16781002/13...
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/16781002/13...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/16781002/18...
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/16781002/20...
Message was sent while issue was closed.
Change committed as 212670 |
