ui/base/touch/touch_device_android.cc - Issue 2673963002: Drop hover:on-demand support & let touch-screens report "none" instead.

Side by Side Diff: ui/base/touch/touch_device_android.cc

Issue 2673963002: Drop hover:on-demand support & let touch-screens report "none" instead. (Closed)

Patch Set: Fixed tests. Created 3 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

OLD	NEW
1 // Copyright (c) 2013 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2013 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "ui/base/touch/touch_device.h"	5 #include "ui/base/touch/touch_device.h"

6	6

7 #include "base/android/context_utils.h"	7 #include "base/android/context_utils.h"

8 #include "base/android/jni_array.h"	8 #include "base/android/jni_array.h"

9 #include "base/logging.h"	9 #include "base/logging.h"

10 #include "jni/TouchDevice_jni.h"	10 #include "jni/TouchDevice_jni.h"

(...skipping 27 matching lines...) Expand all Loading...
38 PointerType GetPrimaryPointerType(int available_pointer_types) {	38 PointerType GetPrimaryPointerType(int available_pointer_types) {

39 if (available_pointer_types & POINTER_TYPE_COARSE)	39 if (available_pointer_types & POINTER_TYPE_COARSE)

40 return POINTER_TYPE_COARSE;	40 return POINTER_TYPE_COARSE;

41 if (available_pointer_types & POINTER_TYPE_FINE)	41 if (available_pointer_types & POINTER_TYPE_FINE)

42 return POINTER_TYPE_FINE;	42 return POINTER_TYPE_FINE;

43 DCHECK_EQ(available_pointer_types, POINTER_TYPE_NONE);	43 DCHECK_EQ(available_pointer_types, POINTER_TYPE_NONE);

44 return POINTER_TYPE_NONE;	44 return POINTER_TYPE_NONE;

45 }	45 }

46	46

47 HoverType GetPrimaryHoverType(int available_hover_types) {	47 HoverType GetPrimaryHoverType(int available_hover_types) {

48 if (available_hover_types & HOVER_TYPE_ON_DEMAND)

49 return HOVER_TYPE_ON_DEMAND;
Rick Byers 2017/03/21 20:47:17 There's a subtle change in semantics here. Note t There's a subtle change in semantics here. Note that unlike on Windows, on Android we were always considering touch "primary" over "mouse". Previously when both touch and mouse were present we'd have pointer:coarse and hover:on-demand. Now in that case it seems we'd have pointer:coarse and hover:hover, which seems inconsistent (is the primary pointer a hovering coarse pointer?). Assuming we want to stick with always considering touch "primary" on android, then I think you want this here: if (available_hover_types & HOVER_TYPE_NONE) return HOVER_TYPE_NONE; Then the only way you ever get hover:hover on Android is when there's no touchscreen at all and only a mouse. mustaq 2017/03/22 14:31:29 Yes, I noted this subtle change in #msg25 above bu Show quoted text On 2017/03/21 20:47:17, Rick Byers wrote: > There's a subtle change in semantics here. Note that unlike on Windows, on > Android we were always considering touch "primary" over "mouse". Previously > when both touch and mouse were present we'd have pointer:coarse and > hover:on-demand. Now in that case it seems we'd have pointer:coarse and > hover:hover, which seems inconsistent (is the primary pointer a hovering coarse > pointer?). > > Assuming we want to stick with always considering touch "primary" on android, > then I think you want this here: > if (available_hover_types & HOVER_TYPE_NONE) > return HOVER_TYPE_NONE; > > Then the only way you ever get hover:hover on Android is when there's no > touchscreen at all and only a mouse. Yes, I noted this subtle change in #msg25 above but I had ruled it out as uncommon. Your suggestion seems better. Done.
50 if (available_hover_types & HOVER_TYPE_HOVER)	48 if (available_hover_types & HOVER_TYPE_HOVER)

51 return HOVER_TYPE_HOVER;	49 return HOVER_TYPE_HOVER;

52 DCHECK_EQ(available_hover_types, HOVER_TYPE_NONE);	50 DCHECK_EQ(available_hover_types, HOVER_TYPE_NONE);
Rick Byers 2017/03/21 20:47:17 This isn't a new problem, but I believe this DCHEC This isn't a new problem, but I believe this DCHECK isn't necessarily true. Looking at the Java code, an Android device with neither a mouse or touchscreen will have available_hover_times==0 (which is not HOVER_TYPE_NONE). Perhaps just remove the DCHECK? I think returning HOVER_TYPE_NONE in both the no device and touch-only cases is what we want here anyway. mustaq 2017/03/22 14:31:29 Did you mean some Java code other than TouchDevice Show quoted text On 2017/03/21 20:47:17, Rick Byers wrote: > This isn't a new problem, but I believe this DCHECK isn't necessarily true. > Looking at the Java code, an Android device with neither a mouse or touchscreen > will have available_hover_times==0 (which is not HOVER_TYPE_NONE). Perhaps just > remove the DCHECK? I think returning HOVER_TYPE_NONE in both the no device and > touch-only cases is what we want here anyway. Did you mean some Java code other than TouchDevice.java:89? I have changed the DCHECK to match your comment above but this code shouldn't see any unset hover-types I believe. Rick Byers 2017/03/23 19:27:32 Ah, sorry - I missed the code that was overwriting Show quoted text On 2017/03/22 14:31:29, mustaq wrote: > On 2017/03/21 20:47:17, Rick Byers wrote: > > This isn't a new problem, but I believe this DCHECK isn't necessarily true. > > Looking at the Java code, an Android device with neither a mouse or > touchscreen > > will have available_hover_times==0 (which is not HOVER_TYPE_NONE). Perhaps > just > > remove the DCHECK? I think returning HOVER_TYPE_NONE in both the no device > and > > touch-only cases is what we want here anyway. > > Did you mean some Java code other than TouchDevice.java:89? I have changed the > DCHECK to match your comment above but this code shouldn't see any unset > hover-types I believe. Ah, sorry - I missed the code that was overwriting the '0' value with the NONE one. Seems a little weird, but OK that means I was wrong and the DHECK is guaranteed to be true. Thanks!
53 return HOVER_TYPE_NONE;	51 return HOVER_TYPE_NONE;

54 }	52 }

55	53

56 } // namespace ui	54 } // namespace ui

OLD	NEW

« third_party/WebKit/public/platform/PointerProperties.h ('K') | « ui/base/touch/touch_device.h ('k') | ui/base/touch/touch_device_linux.cc » ('j') | no next file with comments »