Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
696713002:
Pointer/hover media query support: platform-dependent changes (Closed)
|
Created:
6 years, 1 month ago by mustaq Modified:
6 years ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPointer/hover media query support: platform-dependent changes
This CL includes complete android-specific changes, and stubs for non-Android platforms.
BUG=136119
Committed: https://crrev.com/43306bb1b3f9af4cde375bb837d9e9790ae7fd39
Cr-Commit-Position: refs/heads/master@{#306832}
Patch Set 1 #Patch Set 2 : Incremental, android-only changes #
Total comments: 10
Patch Set 3 : Added a gyp rule to generate java enums #Patch Set 4 : Fixed build dependencies #
Total comments: 6
Patch Set 5 : Rebased, cleaned up TouchDevice.java #
Total comments: 8
Patch Set 6 : Rebased #Patch Set 7 : Completed the Android API, added stubs for other platforms. #
Total comments: 8
Patch Set 8 : #
Total comments: 7
Patch Set 9 : Fixed BUILD.gn, rebased #Patch Set 10 : Added android_webview/java_library_common.mk for generated files #
Total comments: 7
Patch Set 11 : Fixed Java styles. #
Total comments: 4
Patch Set 12 : #
Messages
Total messages: 46 (12 generated)
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:33: PointerTypeNone = 1, Why start with 1 for None? Also, the typical way of styling bit flags would be: PointerTypeNone = 0, PointerTypeCoarse = 1 << 0, PointerTypeHover = 1 << 1, etc... https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:41: HoverTypeNone = 1, Why can't we just use the ui::PointerType/ui::HoverType enums in content?
mustaq@chromium.org changed reviewers: + bokan@chromium.org
Currently trying build rule mods for generated_java_enums. https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:33: PointerTypeNone = 1, On 2014/10/31 15:51:44, jdduke wrote: > Why start with 1 for None? Also, the typical way of styling bit flags would be: > > PointerTypeNone = 0, > PointerTypeCoarse = 1 << 0, > PointerTypeHover = 1 << 1, > etc... Short answer: I followed the blink-side (WebSettings.h) as is. Re none=1: bokan@ and I had a discussion about this few days ago. He mentioned that we wanted to differentiate between unknown vs known-none types. The MQ spec doesn't distinguish these two possibilities and treats "none" as zero, which IMHO is not the right thing to do. I would prefer a PointerTypeUnknown==0 on the spec. One possibility is to add PointerTypeUnknown=0 enum value in both chromium & blink, and limit the MQ weirdness in blink::MediaQueryEvauator. wdyt? Re shifting 1: Yes, this is clearer. Will fix. https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:41: HoverTypeNone = 1, On 2014/10/31 15:51:44, jdduke wrote: > Why can't we just use the ui::PointerType/ui::HoverType enums in content? Hmm, didn't think this way. I instead considered using content::HoverTypes here but it would cause build-rule troubles with generated java enums. Let me get back to you after I am successful in the java enum front.
https://codereview.chromium.org/696713002/diff/20001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:18: public class TouchDevice { Can we change the name of this class? Perhaps InputDevice? We're doing more than just touchscreen here now. (Not necessarily in this CL) https://codereview.chromium.org/696713002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:79: if (hasSource(sources, InputDevice.SOURCE_JOYSTICK) Is there a cursor associated with a joystick? I've never seen a joystick interface use a cursor, to me this would be a pointer: none device. https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:33: PointerTypeNone = 1, On 2014/10/31 16:58:20, mustaq wrote: > On 2014/10/31 15:51:44, jdduke wrote: > > Why start with 1 for None? Also, the typical way of styling bit flags would > be: > > > > PointerTypeNone = 0, > > PointerTypeCoarse = 1 << 0, > > PointerTypeHover = 1 << 1, > > etc... > > Short answer: I followed the blink-side (WebSettings.h) as is. > > Re none=1: bokan@ and I had a discussion about this few days ago. He mentioned > that we wanted to differentiate between unknown vs known-none types. The MQ spec > doesn't distinguish these two possibilities and treats "none" as zero, which > IMHO is not the right thing to do. I would prefer a PointerTypeUnknown==0 on the > spec. > > One possibility is to add PointerTypeUnknown=0 enum value in both chromium & > blink, and limit the MQ weirdness in blink::MediaQueryEvauator. wdyt? > > Re shifting 1: Yes, this is clearer. Will fix. Actually, this isn't to do with an "unknown" value (the spec doesn't have an "unknown" option). It's to correctly handle the any-pointer/hover union [1]; none is a property of the pointer, not an absence of pointers. i.e. we can have any-pointer: none *and* any-pointer: fine both return true if we have two "pointing" devices, one with a fine precision and one without the ability to point (maybe a speech based input?) It's easier to see with hover. If we have a mouse and a touch screen, any-hover will eval to true for either 'none' or 'hover'. Therefore 'none' can't be 0. Also, in the spec "none" isn't treated as zero technically, it evaluates to false in a boolean context [2]. This is to match the CSS style where a media feature evaluated in a boolean context returns true for non-none, non-0 values. This makes statements like @media (pointer) intuitive, it will evaluate to true if there is a pointing capability, regardless of its precision. WRT an unknown value, I don't think it adds any value. We'd be better off just making a reasoned guess if we really can't figure out anything about our input devices. e.g. on Desktop platforms, fall back to pointer: fine hover: hover. [1] http://dev.w3.org/csswg/mediaqueries-4/#any-input [2] http://dev.w3.org/csswg/mediaqueries-4/#mq-boolean-context
https://codereview.chromium.org/696713002/diff/20001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:18: public class TouchDevice { On 2014/11/03 14:28:11, bokan wrote: > Can we change the name of this class? Perhaps InputDevice? We're doing more than > just touchscreen here now. (Not necessarily in this CL) I agree InputDevice is a better name here, will add a TODO. Note that it could be overkill because this class is directly associated with ui/base/touch/touch_device*, and we should then rename the whole tree on the c++ side! https://codereview.chromium.org/696713002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:79: if (hasSource(sources, InputDevice.SOURCE_JOYSTICK) On 2014/11/03 14:28:10, bokan wrote: > Is there a cursor associated with a joystick? I've never seen a joystick > interface use a cursor, to me this would be a pointer: none device. I was considering an old PC-based game I played long ago. Will fix. https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/696713002/diff/20001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:33: PointerTypeNone = 1, On 2014/11/03 14:28:11, bokan wrote: > On 2014/10/31 16:58:20, mustaq wrote: > > On 2014/10/31 15:51:44, jdduke wrote: > > > Why start with 1 for None? Also, the typical way of styling bit flags would > > be: > > > > > > PointerTypeNone = 0, > > > PointerTypeCoarse = 1 << 0, > > > PointerTypeHover = 1 << 1, > > > etc... > > > > Short answer: I followed the blink-side (WebSettings.h) as is. > > > > Re none=1: bokan@ and I had a discussion about this few days ago. He mentioned > > that we wanted to differentiate between unknown vs known-none types. The MQ > spec > > doesn't distinguish these two possibilities and treats "none" as zero, which > > IMHO is not the right thing to do. I would prefer a PointerTypeUnknown==0 on > the > > spec. > > > > One possibility is to add PointerTypeUnknown=0 enum value in both chromium & > > blink, and limit the MQ weirdness in blink::MediaQueryEvauator. wdyt? > > > > Re shifting 1: Yes, this is clearer. Will fix. > > Actually, this isn't to do with an "unknown" value (the spec doesn't have an > "unknown" option). It's to correctly handle the any-pointer/hover union [1]; > none is a property of the pointer, not an absence of pointers. i.e. we can have > any-pointer: none *and* any-pointer: fine both return true if we have two > "pointing" devices, one with a fine precision and one without the ability to > point (maybe a speech based input?) > > It's easier to see with hover. If we have a mouse and a touch screen, any-hover > will eval to true for either 'none' or 'hover'. Therefore 'none' can't be 0. > > Also, in the spec "none" isn't treated as zero technically, it evaluates to > false in a boolean context [2]. This is to match the CSS style where a media > feature evaluated in a boolean context returns true for non-none, non-0 values. > This makes statements like @media (pointer) intuitive, it will evaluate to true > if there is a pointing capability, regardless of its precision. > > WRT an unknown value, I don't think it adds any value. We'd be better off just > making a reasoned guess if we really can't figure out anything about our input > devices. e.g. on Desktop platforms, fall back to pointer: fine hover: hover. > > [1] http://dev.w3.org/csswg/mediaqueries-4/#any-input > [2] http://dev.w3.org/csswg/mediaqueries-4/#mq-boolean-context Thanks for the clarification. Will keep the 1-based enums unchanged.
https://codereview.chromium.org/696713002/diff/60001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:13: import org.chromium.ui.base.PointerType; // to test compilation only You shouldn't need to import this, as we're already in the |package org.chromium.ui.base|. https://codereview.chromium.org/696713002/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:56: public enum PointerType { You shouldn't need this? https://codereview.chromium.org/696713002/diff/60001/ui/base/ui_base.gyp File ui/base/ui_base.gyp (right): https://codereview.chromium.org/696713002/diff/60001/ui/base/ui_base.gyp#newc... ui/base/ui_base.gyp:761: 'dependencies': [ Shouldn't need this, see below. https://codereview.chromium.org/696713002/diff/60001/ui/base/ui_base.gyp#newc... ui/base/ui_base.gyp:770: # TODO(mustaq): GN version: //ui/base:ui_base_??? This should be put in ui/android/ui_android.gyp, just follow the pattern for the other types there.
https://codereview.chromium.org/696713002/diff/60001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:13: import org.chromium.ui.base.PointerType; // to test compilation only On 2014/11/07 22:28:56, jdduke wrote: > You shouldn't need to import this, as we're already in the |package > org.chromium.ui.base|. I had added this as a (short-cut) test for the visibility of the CPP enums. This import should have caused a build warning only, and the actual build error should have been caused by the (duplicate) PointerType below. Because the CPP enums were not visible here, this import failed with "cannot find symbol". I have (cleaned-up the Java code and) updated BUILD.gn rules, but the compiler fails with the same problem: "error: cannot find symbol...PointerType.fine". https://codereview.chromium.org/696713002/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:56: public enum PointerType { On 2014/11/07 22:28:56, jdduke wrote: > You shouldn't need this? It was a leftover from a scratch version, see above.
https://codereview.chromium.org/696713002/diff/80001/ui/base/touch/touch_devi... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/696713002/diff/80001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:34: PointerTypeNone = 1, nit: Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. Continue to use this style for consistency. http://www.chromium.org/developers/coding-style
mkosiba@chromium.org changed reviewers: + mkosiba@chromium.org
https://codereview.chromium.org/696713002/diff/80001/ui/base/BUILD.gn File ui/base/BUILD.gn (right): https://codereview.chromium.org/696713002/diff/80001/ui/base/BUILD.gn#newcode705 ui/base/BUILD.gn:705: java_cpp_enum("ui_base_touch_device_java_enums") { in gn you can have all of the enums in one target so the name should be ui_base_java_enums https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp File ui/base/ui_base.gyp (right): https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp#newc... ui/base/ui_base.gyp:762: 'ui_base_touch_device_java_enums', in gyp it's one target per file so the target name should be ui_base_touch_device_java
https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp File ui/base/ui_base.gyp (right): https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp#newc... ui/base/ui_base.gyp:767: 'includes': [ '../../build/jni_generator.gypi' ], This is your problem. You're trying to make the rule that generates the JNI bindings depend on the auto-generated .java code, that doesn't work right now. The problem is that the generated .java file has to be included in one .jar/.apk, so we have to make it a direct_dependent_settings (and not an all_dependent_settings). Right now only the rules to generate .jar/.apk files properly 'receive' that dependency. You have 2 options: - change build/jni_generator.gypi to forward 'generated_src_dirs' to its direct deps, - have ui_java depend on ui_base_touch_device_java_enums directly.
On 2014/11/10 22:16:54, mkosiba wrote: > https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp > File ui/base/ui_base.gyp (right): > > https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp#newc... > ui/base/ui_base.gyp:767: 'includes': [ '../../build/jni_generator.gypi' ], > This is your problem. You're trying to make the rule that generates the JNI > bindings depend on the auto-generated .java code, that doesn't work right now. > The problem is that the generated .java file has to be included in one > .jar/.apk, so we have to make it a direct_dependent_settings (and not an > all_dependent_settings). Right now only the rules to generate .jar/.apk files > properly 'receive' that dependency. > You have 2 options: > - change build/jni_generator.gypi to forward 'generated_src_dirs' to its direct > deps, > - have ui_java depend on ui_base_touch_device_java_enums directly. mustaq@: Making the original change I suggested (see https://codereview.chromium.org/694533010), it compiles just fine.
ptal, the Android side should be complete now.
For now I have added stubs for touch_device_{aurax11,ozone,win}.cc, I'll replace
them in later CLs.
https://codereview.chromium.org/696713002/diff/80001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):
https://codereview.chromium.org/696713002/diff/80001/ui/base/BUILD.gn#newcode705
ui/base/BUILD.gn:705: java_cpp_enum("ui_base_touch_device_java_enums") {
On 2014/11/10 22:06:04, mkosiba wrote:
> in gn you can have all of the enums in one target so the name should be
> ui_base_java_enums
Done.
https://codereview.chromium.org/696713002/diff/80001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device.h (right):
https://codereview.chromium.org/696713002/diff/80001/ui/base/touch/touch_devi...
ui/base/touch/touch_device.h:34: PointerTypeNone = 1,
On 2014/11/10 22:00:13, mkosiba wrote:
> nit: Though the Google C++ Style Guide now says to use kConstantNaming for
> enums, Chromium was written using MACRO_STYLE naming. Continue to use this
> style for consistency.
> http://www.chromium.org/developers/coding-style
Done.
https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):
https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp#newc...
ui/base/ui_base.gyp:762: 'ui_base_touch_device_java_enums',
On 2014/11/10 22:06:04, mkosiba wrote:
> in gyp it's one target per file so the target name should be
> ui_base_touch_device_java
Done.
https://codereview.chromium.org/696713002/diff/80001/ui/base/ui_base.gyp#newc...
ui/base/ui_base.gyp:767: 'includes': [ '../../build/jni_generator.gypi' ],
On 2014/11/10 22:16:54, mkosiba wrote:
> This is your problem. You're trying to make the rule that generates the JNI
> bindings depend on the auto-generated .java code, that doesn't work right now.
> The problem is that the generated .java file has to be included in one
> .jar/.apk, so we have to make it a direct_dependent_settings (and not an
> all_dependent_settings). Right now only the rules to generate .jar/.apk files
> properly 'receive' that dependency.
> You have 2 options:
> - change build/jni_generator.gypi to forward 'generated_src_dirs' to its
direct
> deps,
> - have ui_java depend on ui_base_touch_device_java_enums directly.
Acknowledged.
https://codereview.chromium.org/696713002/diff/120001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:17: * FIXME: This does more than just touches. Rename to InputDevice? Should do the Nit: Let's move this TODO to the touch_device.h header, as this is just a reflection of that. https://codereview.chromium.org/696713002/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:71: Hmm, while I don't mind the line break here before the closing brace, I don't think it's common style (same below). https://codereview.chromium.org/696713002/diff/120001/ui/base/touch/touch_dev... File ui/base/touch/touch_device_android.cc (right): https://codereview.chromium.org/696713002/diff/120001/ui/base/touch/touch_dev... ui/base/touch/touch_device_android.cc:33: if (available_pointer_types & POINTER_TYPE_COARSE) Will this cascading logic apply to all platforms? That is, if a given platform supports all pointer types, will we always use this logic to select the primary pointer type? (same question for hover). If so, why ship the primary pointer at all to the renderer?
https://codereview.chromium.org/696713002/diff/120001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:17: * FIXME: This does more than just touches. Rename to InputDevice? Should do the On 2014/11/26 16:57:04, jdduke wrote: > Nit: Let's move this TODO to the touch_device.h header, as this is just a > reflection of that. Done. https://codereview.chromium.org/696713002/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:71: On 2014/11/26 16:57:04, jdduke wrote: > Hmm, while I don't mind the line break here before the closing brace, I don't > think it's common style (same below). Done. https://codereview.chromium.org/696713002/diff/120001/ui/base/touch/touch_dev... File ui/base/touch/touch_device_android.cc (right): https://codereview.chromium.org/696713002/diff/120001/ui/base/touch/touch_dev... ui/base/touch/touch_device_android.cc:12: bool IsTouchDevicePresent() { I think it's time to fine-tune this, right? I mean using AvailablePointerTypes()? https://codereview.chromium.org/696713002/diff/120001/ui/base/touch/touch_dev... ui/base/touch/touch_device_android.cc:33: if (available_pointer_types & POINTER_TYPE_COARSE) On 2014/11/26 16:57:04, jdduke wrote: > Will this cascading logic apply to all platforms? That is, if a given platform > supports all pointer types, will we always use this logic to select the primary > pointer type? (same question for hover). If so, why ship the primary pointer at > all to the renderer? I think it will be different for other platforms. E.g. unlike in Android, in linux desktop it is reasonable assume that the fine-pointer (mouse/trackball) is primary when both fine- and coarse-pointers are there.
https://codereview.chromium.org/696713002/diff/120001/ui/base/touch/touch_dev... File ui/base/touch/touch_device_android.cc (right): https://codereview.chromium.org/696713002/diff/120001/ui/base/touch/touch_dev... ui/base/touch/touch_device_android.cc:33: if (available_pointer_types & POINTER_TYPE_COARSE) On 2014/11/26 18:41:44, mustaq wrote: > On 2014/11/26 16:57:04, jdduke wrote: > > Will this cascading logic apply to all platforms? That is, if a given platform > > supports all pointer types, will we always use this logic to select the > primary > > pointer type? (same question for hover). If so, why ship the primary pointer > at > > all to the renderer? > > I think it will be different for other platforms. E.g. unlike in Android, in > linux desktop it is reasonable assume that the fine-pointer (mouse/trackball) is > primary when both fine- and coarse-pointers are there. This is just the initial default implementation. I think eventually we may experiment with some behaviours here to make this more sophisticated. i.e. Maybe if you used your mouse to interact with the previous page we'll use PointerFine as the primary pointer, even there's a touchscreen. We could also present an override to the user. Whatever the case, we'll probably want the same logic to apply on all platforms, though as Mustaq said, we'll have different fallback defaults.
https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1351: void RenderViewHostImpl::UpdateWebkitPreferences(const WebPreferences& prefs) { Any idea how frequently this gets called?
mustaq@chromium.org changed reviewers: + rbyers@chromium.org
https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1351: void RenderViewHostImpl::UpdateWebkitPreferences(const WebPreferences& prefs) { On 2014/11/26 19:29:28, jdduke wrote: > Any idea how frequently this gets called? It seems RenderViewHostImpl::CreateRenderView is the only "end"-user for this. So it gets called only during browser startup I think. Or may be new tab. Rick, you mentioned about supporting hot-plugging of input devices though this method, right?
https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1351: void RenderViewHostImpl::UpdateWebkitPreferences(const WebPreferences& prefs) { On 2014/11/26 19:51:50, mustaq wrote: > On 2014/11/26 19:29:28, jdduke wrote: > > Any idea how frequently this gets called? > > It seems RenderViewHostImpl::CreateRenderView is the only "end"-user for this. > So it gets called only during browser startup I think. Or may be new tab. > > Rick, you mentioned about supporting hot-plugging of input devices though this > method, right? I ask because each of the queries into Java are likely generating garbage, so as long as they're called relatively infrequently, I'm happy. Note that there's also an |InputDeviceListener| interface (http://developer.android.com/reference/android/hardware/input/InputManager.In...) if we wanted to push these values when we detect a change in hover/pointer types.
On 2014/11/26 20:05:38, jdduke wrote: > https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... > File content/browser/renderer_host/render_view_host_impl.cc (right): > > https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... > content/browser/renderer_host/render_view_host_impl.cc:1351: void > RenderViewHostImpl::UpdateWebkitPreferences(const WebPreferences& prefs) { > On 2014/11/26 19:51:50, mustaq wrote: > > On 2014/11/26 19:29:28, jdduke wrote: > > > Any idea how frequently this gets called? > > > > It seems RenderViewHostImpl::CreateRenderView is the only "end"-user for this. > > So it gets called only during browser startup I think. Or may be new tab. > > > > Rick, you mentioned about supporting hot-plugging of input devices though this > > method, right? > > I ask because each of the queries into Java are likely generating garbage, so as > long as they're called relatively infrequently, I'm happy. Note that there's > also an |InputDeviceListener| interface > (http://developer.android.com/reference/android/hardware/input/InputManager.In...) > if we wanted to push these values when we detect a change in hover/pointer > types. Yeah I think eventually we'll want to use the InputDeviceListener to watch for changes. I told Mustaq I thought it was OK to worry about dynamic changes in a follow-up patch if that was more convenient. We'll get 95% of the value from the static case (since it's rare for input devices to change).
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
https://codereview.chromium.org/696713002/diff/140001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/696713002/diff/140001/ui/android/BUILD.gn#new... ui/android/BUILD.gn:15: "org/chromium/ui/base/PageTransition.java", You need to add an appropriate |TouchDevice.java| output entry here.
Patchset #9 (id:200001) has been deleted
PTAL: - All Android tests are passing now. Wondering if we should commit the Android part (with stubs for other platforms, as in Patch 10) separately. I am fearing that the CPP-enums-in-Java could have remote dependency issues not caught by git-cl try. Committing early would give us more time to fix before M41 feature cutoff date. - Done with touch_device_win.cc in Patch 11. https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1351: void RenderViewHostImpl::UpdateWebkitPreferences(const WebPreferences& prefs) { On 2014/11/26 20:05:38, jdduke wrote: > On 2014/11/26 19:51:50, mustaq wrote: > > On 2014/11/26 19:29:28, jdduke wrote: > > > Any idea how frequently this gets called? > > > > It seems RenderViewHostImpl::CreateRenderView is the only "end"-user for this. > > So it gets called only during browser startup I think. Or may be new tab. > > > > Rick, you mentioned about supporting hot-plugging of input devices though this > > method, right? > > I ask because each of the queries into Java are likely generating garbage, so as > long as they're called relatively infrequently, I'm happy. Note that there's > also an |InputDeviceListener| interface > (http://developer.android.com/reference/android/hardware/input/InputManager.In...) > if we wanted to push these values when we detect a change in hover/pointer > types. Acknowledged. https://codereview.chromium.org/696713002/diff/140001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/696713002/diff/140001/ui/android/BUILD.gn#new... ui/android/BUILD.gn:15: "org/chromium/ui/base/PageTransition.java", On 2014/12/01 18:53:06, jdduke wrote: > You need to add an appropriate |TouchDevice.java| output entry here. Done.
Patchset #11 (id:260001) has been deleted
Hmm, the latest patchset looks a bit bare?
On 2014/12/02 16:37:12, jdduke wrote: > Hmm, the latest patchset looks a bit bare? Sorry for not being clear: I deliberately removed non-windows diffs (because I was wondering if windows, linux, aurax11 and ozone could be in another cl). Changed the patch label.
jdduke@chromium.org changed reviewers: + tedchoc@chromium.org
On 2014/12/02 16:42:44, mustaq wrote: > On 2014/12/02 16:37:12, jdduke wrote: > > Hmm, the latest patchset looks a bit bare? > > Sorry for not being clear: I deliberately removed non-windows diffs (because I > was wondering if windows, linux, aurax11 and ozone could be in another cl). > Changed the patch label. Do you have working prototypes for those platforms? In general this patch lgtm. Adding tedchoc@ to do a sanity check on TouchDevice.java. https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (left): https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:25: * Returns the number of supported touch points. Why get rid of this comment? It may seem redundant but, well, Javadoc... https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:15: * Simple proxy to let us query input devices from C++. Nit: "query input device properties"
> > Sorry for not being clear: I deliberately removed non-windows diffs (because I > > was wondering if windows, linux, aurax11 and ozone could be in another cl). > > Changed the patch label. > > Do you have working prototypes for those platforms? > Windows is done, ozone is trivial (and done). I'll work on linux and aurax11 from tomorrow.
https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/696713002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1351: void RenderViewHostImpl::UpdateWebkitPreferences(const WebPreferences& prefs) { On 2014/12/02 16:28:19, mustaq wrote: > On 2014/11/26 20:05:38, jdduke wrote: > > On 2014/11/26 19:51:50, mustaq wrote: > > > On 2014/11/26 19:29:28, jdduke wrote: > > > > Any idea how frequently this gets called? > > > > > > It seems RenderViewHostImpl::CreateRenderView is the only "end"-user for > this. > > > So it gets called only during browser startup I think. Or may be new tab. > > > > > > Rick, you mentioned about supporting hot-plugging of input devices though > this > > > method, right? > > > > I ask because each of the queries into Java are likely generating garbage, so > as > > long as they're called relatively infrequently, I'm happy. Note that there's > > also an |InputDeviceListener| interface > > > (http://developer.android.com/reference/android/hardware/input/InputManager.In...) > > if we wanted to push these values when we detect a change in hover/pointer > > types. > > Acknowledged. Here is a better answer to your original question: it is called once per page load. Just double-checked by running on linux and Android. https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (left): https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:25: * Returns the number of supported touch points. On 2014/12/02 20:00:19, jdduke wrote: > Why get rid of this comment? It may seem redundant but, well, Javadoc... /** * Let's get rid of redundancy in all Javadocs! * @param Javadoc with redundancy * @return Javadoc without redundancy * @throws RedundantUseOfJavadocRedundancyEliminatorException */ https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:15: * Simple proxy to let us query input devices from C++. On 2014/12/02 20:00:19, jdduke wrote: > Nit: "query input device properties" Done.
Patchset #11 (id:280001) has been deleted
mustaq@chromium.org changed reviewers: + sadrul@chromium.org
I have removed the incremental patch#11 (touch_device_win.cc) to avoid confusion. Will create a separate CL for non-Android platforms (win, linux, aurax11 and ozone). sadrul@chromium.org: Please review changes in touch_device*.
androidy bits lgtm with some little nits https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:53: * @return Returns the pointer-types supported by the device, as the union drop [Returns], it's fine to start with The since the @return makes it read that way. The line length in java is 100, so more should be able to fit on this line. Also, the secondary lines should align with the first word of the javadoc after the @return. https://codereview.chromium.org/696713002/diff/240001/ui/base/touch/touch_dev... File ui/base/touch/touch_device_android.cc (right): https://codereview.chromium.org/696713002/diff/240001/ui/base/touch/touch_dev... ui/base/touch/touch_device_android.cc:27: Java_TouchDevice_availablePointerTypes(env, context); I think you should be able to just return this instead of static casting to int (at least I think you can).
https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/TouchDevice.java (right): https://codereview.chromium.org/696713002/diff/240001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/TouchDevice.java:53: * @return Returns the pointer-types supported by the device, as the union On 2014/12/03 20:31:31, Ted C wrote: > drop [Returns], it's fine to start with The since the @return makes it read that > way. > > The line length in java is 100, so more should be able to fit on this line. > > Also, the secondary lines should align with the first word of the javadoc after > the @return. Done.
A couple of nits. Otherwise, LGTM https://codereview.chromium.org/696713002/diff/300001/ui/base/touch/touch_dev... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/696713002/diff/300001/ui/base/touch/touch_dev... ui/base/touch/touch_device.h:15: // the same for most of the files in this folder. Can you please file a bug and reference it from here? https://codereview.chromium.org/696713002/diff/300001/ui/base/touch/touch_dev... ui/base/touch/touch_device.h:60: UI_BASE_EXPORT HoverType PrimaryHoverType(); Can all these have Get prefix? Document that these return _NONE if there's no real implementation.
https://codereview.chromium.org/696713002/diff/300001/ui/base/touch/touch_dev... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/696713002/diff/300001/ui/base/touch/touch_dev... ui/base/touch/touch_device.h:15: // the same for most of the files in this folder. On 2014/12/03 21:45:02, sadrul wrote: > Can you please file a bug and reference it from here? Done. https://codereview.chromium.org/696713002/diff/300001/ui/base/touch/touch_dev... ui/base/touch/touch_device.h:60: UI_BASE_EXPORT HoverType PrimaryHoverType(); On 2014/12/03 21:45:02, sadrul wrote: > Can all these have Get prefix? > > Document that these return _NONE if there's no real implementation. Done with the Get prefix. I will skip adding comments that reflect my stub implementation details, for two reasons: (i) I'll replace my stubs soon, and (ii) I don't want to create a convention for non-real implementations.
lgtm
The CQ bit was checked by mustaq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/696713002/320001
Message was sent while issue was closed.
Committed patchset #12 (id:320001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/43306bb1b3f9af4cde375bb837d9e9790ae7fd39 Cr-Commit-Position: refs/heads/master@{#306832} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
