Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2878403002:
Support setting mouse cursor icon in Android N. (Closed)
|
Created:
3 years, 7 months ago by jaebaek Modified:
3 years, 4 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport setting mouse cursor icon in Android N.
Android N supports mouse input and setting mouse cursor icon
(https://developer.android.com/reference/android/view/View.html#setPointerIcon(android.view.PointerIcon)).
Current Chrome does not change the mouse cursor icon even
when CSS file specifies the cursor type. This CL supports
the mouse cursor setup (e.g., IBeam, Hand, custom cursor
from image).
BUG=584424
Review-Url: https://codereview.chromium.org/2878403002
Cr-Commit-Position: refs/heads/master@{#493995}
Committed: https://chromium.googlesource.com/chromium/src/+/d1b0d570f9bc9bf80d187170fbeac0d4a1f6eb50
Patch Set 1 #Patch Set 2 : [WIP] pointer icon in android #Patch Set 3 : Support setting pointer icon in Android N #Patch Set 4 : Support setting pointer icon in Android N #Patch Set 5 : Support setting pointer icon in Android N #
Total comments: 34
Patch Set 6 : Support setting pointer icon in Android N #Patch Set 7 : Support setting pointer icon in Android N #
Total comments: 4
Patch Set 8 : Support setting mouse cursor icon in Android N #
Total comments: 15
Patch Set 9 : Support setting mouse cursor icon in Android N #Patch Set 10 : Support setting mouse cursor icon in Android N #Patch Set 11 : Support setting mouse cursor icon in Android N #
Total comments: 4
Patch Set 12 : Support setting mouse cursor icon in Android N #
Total comments: 2
Patch Set 13 : Support setting mouse cursor icon in Android N #
Total comments: 16
Patch Set 14 : Support setting mouse cursor icon in Android N #Patch Set 15 : Support setting mouse cursor icon in Android N (Test) #Patch Set 16 : Support setting mouse cursor icon in Android N #Patch Set 17 : Support setting mouse cursor icon in Android N #Patch Set 18 : Support setting mouse cursor icon in Android N #
Total comments: 6
Patch Set 19 : Support setting mouse cursor icon in Android N #
Total comments: 10
Patch Set 20 : Support setting mouse cursor icon in Android N (Test) #Patch Set 21 : Support setting mouse cursor icon in Android N #
Total comments: 6
Patch Set 22 : Support setting mouse cursor icon in Android N #
Total comments: 2
Patch Set 23 : nit #
Total comments: 2
Patch Set 24 : CoordinatorLayout for PointerIcon #Patch Set 25 : Handle the case content view is null #Patch Set 26 : Handle the case content view is null #Patch Set 27 : Resize views to show pointer icon #Patch Set 28 : Resize views to show pointer icon (rebase) #Patch Set 29 : Support setting mouse cursor icon in Android N #
Total comments: 8
Patch Set 30 : Support setting mouse cursor icon in Android N #Patch Set 31 : Support setting mouse cursor icon in Android N #
Total comments: 12
Patch Set 32 : Override onResolvePointerIcon #Patch Set 33 : Override onResolvePointerIcon #
Total comments: 23
Patch Set 34 : Override onResolvePointerIcon #Patch Set 35 : Support setting mouse cursor icon in Android N #Patch Set 36 : rebase #Patch Set 37 : Support setting mouse cursor icon in Android N #Patch Set 38 : Apply patch 585579 #Patch Set 39 : rebase #Patch Set 40 : Apply patch 585579 #Patch Set 41 : Support setting mouse cursor icon in Android N #Patch Set 42 : Support setting mouse cursor icon in Android N #Patch Set 43 : Support setting mouse cursor icon in Android N #Patch Set 44 : Support setting mouse cursor icon in Android N #Patch Set 45 : Support setting mouse cursor icon in Android N #
Total comments: 8
Patch Set 46 : Support setting mouse cursor icon in Android N #Patch Set 47 : rebase #Patch Set 48 : Support setting mouse cursor icon in Android N #Messages
Total messages: 145 (74 generated)
Description was changed from ========== [WIP] pointer icon in android BUG=584424 ========== to ========== [WIP] Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon accordingly. Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). However, I added a JNI function to directly call ContentViewCore. I am working on using ViewAndroidDelegate instead. BUG=584424 ==========
Description was changed from ========== [WIP] Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon accordingly. Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). However, I added a JNI function to directly call ContentViewCore. I am working on using ViewAndroidDelegate instead. BUG=584424 ========== to ========== [WIP] Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon accordingly. Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). However, I added a JNI function to directly call ContentViewCore. I am working on adopting ViewAndroidDelegate instead. BUG=584424 ==========
Description was changed from ========== [WIP] Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon accordingly. Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). However, I added a JNI function to directly call ContentViewCore. I am working on adopting ViewAndroidDelegate instead. BUG=584424 ========== to ========== Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon accordingly (https://developer.android.com/reference/android/view/View.html#setPointerIcon...). Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). BUG=584424 ==========
jaebaek@chromium.org changed reviewers: + aelias@chromium.org, jinsukkim@chromium.org
Description was changed from ========== Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon accordingly (https://developer.android.com/reference/android/view/View.html#setPointerIcon...). Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). BUG=584424 ========== to ========== Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon accordingly (https://developer.android.com/reference/android/view/View.html#setPointerIcon...). Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). BUG=584424 ==========
Description was changed from ========== Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon accordingly (https://developer.android.com/reference/android/view/View.html#setPointerIcon...). Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). BUG=584424 ========== to ========== Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon (https://developer.android.com/reference/android/view/View.html#setPointerIcon...). Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). BUG=584424 ==========
PTAL
https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:710: if (!content_view_core_) No longer needed. https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:786: LOG(WARNING) << "Invalid cursor type: " This is likely to spam logs and the problem is not actionable. In general, logging is discouraged in Chromium unless there's a very good reason, so I'd suggest just deleting this line (and the GetName() implementation). https://codereview.chromium.org/2878403002/diff/80001/device/power_save_block... File device/power_save_blocker/BUILD.gn (right): https://codereview.chromium.org/2878403002/diff/80001/device/power_save_block... device/power_save_blocker/BUILD.gn:60: "//ui/gfx", This doesn't belong here, incorporated another patch by accident? https://codereview.chromium.org/2878403002/diff/80001/ui/android/cursor_type.h File ui/android/cursor_type.h (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/cursor_type.... ui/android/cursor_type.h:15: // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.ui.base Can you add this annotation to WebCursorInfo::type instead and avoid introducing a new enum? There is an analogue in https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIn... Then there's no need to have much code in RenderWidgetHostViewAndroid anymore, you can only have logic in Java. https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:113: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { "if ( < N ) return;" to reduce indentation https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:118: containerView.getContext(), PointerIcon.TYPE_NULL)); You can avoid some boilerplate duplication here by just assigning the PointerIcon type to an integer in the switch statement, and call the get/set at the end of the method. https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:206: default: Please exhaustively list all remaining WebCursorInfo types instead of using "default:". This will help keep this file synchronized with the C++ header.
Nice to see the icons working. https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:710: if (!content_view_core_) No need to do this check. https://codereview.chromium.org/2878403002/diff/80001/device/power_save_block... File device/power_save_blocker/BUILD.gn (right): https://codereview.chromium.org/2878403002/diff/80001/device/power_save_block... device/power_save_blocker/BUILD.gn:60: "//ui/gfx", This change looks irrelvant. https://codereview.chromium.org/2878403002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCursorInfo.h (right): https://codereview.chromium.org/2878403002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCursorInfo.h:118: static const char* GetName(WebCursorInfo::Type type) { This function is used only for warning message. I'm not sure if it's worth adding. https://codereview.chromium.org/2878403002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCursorInfo.h:120: case WebCursorInfo::k##t: \ Indentation looks wrong. https://codereview.chromium.org/2878403002/diff/80001/ui/android/cursor_type.h File ui/android/cursor_type.h (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/cursor_type.... ui/android/cursor_type.h:10: // This file contains a list of CursorType's usable by ViewAndroidDelegate, Remove the description ViewAndroidDelegate as it may not be the only user of the enum in java layer. How about just saying '.. a list of cursor types that provides.'? https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:113: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { Do early return to avoid too long if. if (INT < N) return; https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:114: ViewGroup containerView = getContainerView(); Define variables for the root view and the context to make the switch statement less verbose. https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android... ui/android/view_android.cc:14: #include "ui/android/cursor_type.h" It's already included in the header. No need to include again. https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android... ui/android/view_android.cc:259: env, delegate, java_bitmap, static_cast<jint>(hotspot.x()), I think you can do without static_cast. https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android... ui/android/view_android.h:15: #include "ui/gfx/android/java_bitmap.h" What is this included for? If for SkBitmap then directly include the one(s) that defines what you need instead.
jaebaek@chromium.org changed reviewers: + boliu@chromium.org
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL For device/power_save_blocker, device/power_save_blocker/power_save_blocker_android.cc includes view_android.h To enable the custom cursor type, I cannot help but adding SkBitmap type in view_android.h. It causes compile failure of device/power_save_blocker/power_save_blocker_android.cc, since it dose not have dependency with SkBitmap. It is the reason I added //ui/gfx in device/power_save_blocker/BUILD.gn. It might be better to make device/power_save_blocker/power_save_blocker_android.cc not include view_android.h. https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:710: if (!content_view_core_) On 2017/05/17 04:47:15, aelias wrote: > No longer needed. Done. https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:710: if (!content_view_core_) On 2017/05/17 04:49:50, Jinsuk Kim wrote: > No need to do this check. Done. https://codereview.chromium.org/2878403002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:786: LOG(WARNING) << "Invalid cursor type: " On 2017/05/17 04:47:15, aelias wrote: > This is likely to spam logs and the problem is not actionable. In general, > logging is discouraged in Chromium unless there's a very good reason, so I'd > suggest just deleting this line (and the GetName() implementation). Done. https://codereview.chromium.org/2878403002/diff/80001/device/power_save_block... File device/power_save_blocker/BUILD.gn (right): https://codereview.chromium.org/2878403002/diff/80001/device/power_save_block... device/power_save_blocker/BUILD.gn:60: "//ui/gfx", On 2017/05/17 04:47:15, aelias wrote: > This doesn't belong here, incorporated another patch by accident? This requires an independent patch. The problem is from device/power_save_blocker. Let me solve this in another patch. https://codereview.chromium.org/2878403002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCursorInfo.h (right): https://codereview.chromium.org/2878403002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCursorInfo.h:118: static const char* GetName(WebCursorInfo::Type type) { On 2017/05/17 04:49:50, Jinsuk Kim wrote: > This function is used only for warning message. I'm not sure if it's worth > adding. Done. https://codereview.chromium.org/2878403002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCursorInfo.h:120: case WebCursorInfo::k##t: \ On 2017/05/17 04:49:50, Jinsuk Kim wrote: > Indentation looks wrong. Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/cursor_type.h File ui/android/cursor_type.h (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/cursor_type.... ui/android/cursor_type.h:10: // This file contains a list of CursorType's usable by ViewAndroidDelegate, On 2017/05/17 04:49:50, Jinsuk Kim wrote: > Remove the description ViewAndroidDelegate as it may not be the only user of the > enum in java layer. How about just saying '.. a list of cursor types that > provides.'? Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/cursor_type.... ui/android/cursor_type.h:15: // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.ui.base On 2017/05/17 04:47:15, aelias wrote: > Can you add this annotation to WebCursorInfo::type instead and avoid introducing > a new enum? There is an analogue in > https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIn... > > Then there's no need to have much code in RenderWidgetHostViewAndroid anymore, > you can only have logic in Java. Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:113: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2017/05/17 04:47:15, aelias wrote: > "if ( < N ) return;" to reduce indentation Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:113: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2017/05/17 04:49:50, Jinsuk Kim wrote: > Do early return to avoid too long if. > > if (INT < N) return; Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:114: ViewGroup containerView = getContainerView(); On 2017/05/17 04:49:50, Jinsuk Kim wrote: > Define variables for the root view and the context to make the switch statement > less verbose. Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:118: containerView.getContext(), PointerIcon.TYPE_NULL)); On 2017/05/17 04:47:15, aelias wrote: > You can avoid some boilerplate duplication here by just assigning the > PointerIcon type to an integer in the switch statement, and call the get/set at > the end of the method. Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:206: default: On 2017/05/17 04:47:15, aelias wrote: > Please exhaustively list all remaining WebCursorInfo types instead of using > "default:". This will help keep this file synchronized with the C++ header. I listed all WebCursorInfo types and default for exceptions. https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android... ui/android/view_android.cc:14: #include "ui/android/cursor_type.h" On 2017/05/17 04:49:50, Jinsuk Kim wrote: > It's already included in the header. No need to include again. Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android... ui/android/view_android.cc:259: env, delegate, java_bitmap, static_cast<jint>(hotspot.x()), On 2017/05/17 04:49:50, Jinsuk Kim wrote: > I think you can do without static_cast. Done. https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2878403002/diff/80001/ui/android/view_android... ui/android/view_android.h:15: #include "ui/gfx/android/java_bitmap.h" On 2017/05/17 04:49:50, Jinsuk Kim wrote: > What is this included for? If for SkBitmap then directly include the one(s) that > defines what you need instead. Done.
https://codereview.chromium.org/2878403002/diff/80001/device/power_save_block... File device/power_save_blocker/BUILD.gn (right): https://codereview.chromium.org/2878403002/diff/80001/device/power_save_block... device/power_save_blocker/BUILD.gn:60: "//ui/gfx", On 2017/05/18 01:44:28, jaebaek wrote: > On 2017/05/17 04:47:15, aelias wrote: > > This doesn't belong here, incorporated another patch by accident? > > This requires an independent patch. > The problem is from device/power_save_blocker. > Let me solve this in another patch. So this is a side effect of view_android.h having a new dependency on skia. Looks like this is something that should be sorted out inside skia... Will wait for device/ owner to comment. https://codereview.chromium.org/2878403002/diff/120001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/2878403002/diff/120001/ui/android/BUILD.gn#ne... ui/android/BUILD.gn:103: "../../third_party/WebKit/public/platform/WebCursorInfo.h", Have the enum generated in blink where the header is defined, and add a dependency to the jar. https://codereview.chromium.org/2878403002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:183: case WebCursorInfoType.TYPE_EAST_RESIZE: default: will be enough. No need to have other cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2878403002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:183: case WebCursorInfoType.TYPE_EAST_RESIZE: On 2017/05/18 at 02:19:51, Jinsuk Kim wrote: > default: will be enough. No need to have other cases. I would prefer the reverse, please remove "default:" and keep the other cases. Then it's clearer which ones are missing, and if a new WebCursorType is added, it will force the author to look here and see if it maps to anything. https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:203: case WebCursorInfoType.TYPE_CUSTOM: This one in particular can have an assertion failure instead of falling back to ARROW, since it can never happen.
https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:190: case WebCursorInfoType.TYPE_WEST_RESIZE: These are web-specced to do something as CSS values: https://developer.mozilla.org/en-US/docs/Web/CSS/cursor?v=control so we should go ahead and support them, or it will be filed as a bug later. As Android does not provide a system resource for these, we can package a fallback PNG file within the Chromium APK and use the custom cursor Bitmap API to display them. There is precedent on other platforms for doing this: content/app/resources/content_resources.grd (Mac fallbacks) ui/resources/ui_resources.grd (Linux Aura fallbacks) I'd suggest expanding the if statement in ui_resources.grd to cover Android, only for the exact PNGs that we are missing (shouldn't be overbroad to avoid wasting disk space). Then load them in the C++ side and call onCursorChangedToCustom(). Also note for reference that the Android pngs are the pointer_ ones in https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master... (git history indicates they were copied from Chromium ones in the first place)
hmm... test? https://codereview.chromium.org/2878403002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2878403002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:725: view_.OnCursorChanged(cursor_info.type == WebCursorInfo::kTypeCustom, this parameter is redundant, type is already passed https://codereview.chromium.org/2878403002/diff/140001/device/power_save_bloc... File device/power_save_blocker/BUILD.gn (right): https://codereview.chromium.org/2878403002/diff/140001/device/power_save_bloc... device/power_save_blocker/BUILD.gn:60: "//ui/gfx", hmm, why is this needed? if everything is set up correctly, then no code change should mean no BUILD.gn change needed either https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:209: containerView.getRootView().setPointerIcon( why root view? https://codereview.chromium.org/2878403002/diff/140001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2878403002/diff/140001/ui/android/view_androi... ui/android/view_android.h:13: #include "third_party/skia/include/core/SkBitmap.h" this can just be a forward declaration, then include header in cc file, also forward declare and include in cc gfx::Point. the rule is include what you use
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
jaebaek@chromium.org changed reviewers: + thakis@chromium.org
PTAL https://codereview.chromium.org/2878403002/diff/120001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/120001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:183: case WebCursorInfoType.TYPE_EAST_RESIZE: On 2017/05/18 02:19:51, Jinsuk Kim wrote: > default: will be enough. No need to have other cases. This is for letting others know all enums specified in the header file. This was a comment from aelias@ https://codereview.chromium.org/2878403002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2878403002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:725: view_.OnCursorChanged(cursor_info.type == WebCursorInfo::kTypeCustom, On 2017/05/19 02:27:57, boliu wrote: > this parameter is redundant, type is already passed Done. https://codereview.chromium.org/2878403002/diff/140001/device/power_save_bloc... File device/power_save_blocker/BUILD.gn (right): https://codereview.chromium.org/2878403002/diff/140001/device/power_save_bloc... device/power_save_blocker/BUILD.gn:60: "//ui/gfx", On 2017/05/19 02:27:57, boliu wrote: > hmm, why is this needed? if everything is set up correctly, then no code change > should mean no BUILD.gn change needed either Done. https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:190: case WebCursorInfoType.TYPE_WEST_RESIZE: On 2017/05/18 19:59:11, aelias wrote: > These are web-specced to do something as CSS values: > https://developer.mozilla.org/en-US/docs/Web/CSS/cursor?v=control so we should > go ahead and support them, or it will be filed as a bug later. As Android does > not provide a system resource for these, we can package a fallback PNG file > within the Chromium APK and use the custom cursor Bitmap API to display them. > There is precedent on other platforms for doing this: > > content/app/resources/content_resources.grd (Mac fallbacks) > ui/resources/ui_resources.grd (Linux Aura fallbacks) > > I'd suggest expanding the if statement in ui_resources.grd to cover Android, > only for the exact PNGs that we are missing (shouldn't be overbroad to avoid > wasting disk space). Then load them in the C++ side and call > onCursorChangedToCustom(). > > Also note for reference that the Android pngs are the pointer_ ones in > https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master... > (git history indicates they were copied from Chromium ones in the first place) When I checked images in ui/resources/ui_resources.grd, it does not have these images. For example, the mouse cursor in Linux Chrome for the "n-resize" shape (check it here https://developer.mozilla.org/en-US/docs/Web/CSS/cursor?v=control) looks different from ui/resources/default_100_percent/common/pointers/sb_v_double_arrow.png which is matched to "IDR_AURA_CURSOR_NORTH_RESIZE". In fact, it is the same with PointerIcon.TYPE_HORIZONTAL_DOUBLE_ARROW. Now I will modify the code structure to adopt these images, but we need some helps from UI designer. I will report this issue in the bug report. https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:203: case WebCursorInfoType.TYPE_CUSTOM: On 2017/05/18 18:57:34, aelias wrote: > This one in particular can have an assertion failure instead of falling back to > ARROW, since it can never happen. Done. https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:205: pointerIconType = PointerIcon.TYPE_ARROW; When the type is exceptional (i.e., none of the above), should it be default (arrow)? or must it be just returned here? or assertion failure? https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:209: containerView.getRootView().setPointerIcon( On 2017/05/19 02:27:57, boliu wrote: > why root view? If I call "setPointerIcon" to "containerView", the mouse icon is not changed. "containerView" stays behind of its RootView. https://codereview.chromium.org/2878403002/diff/140001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2878403002/diff/140001/ui/android/view_androi... ui/android/view_android.h:13: #include "third_party/skia/include/core/SkBitmap.h" On 2017/05/19 02:27:57, boliu wrote: > this can just be a forward declaration, then include header in cc file, also > forward declare and include in cc gfx::Point. the rule is include what you use Done.
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:209: containerView.getRootView().setPointerIcon( On 2017/05/22 12:08:35, jaebaek wrote: > On 2017/05/19 02:27:57, boliu wrote: > > why root view? > > If I call "setPointerIcon" to "containerView", the mouse icon is not changed. > "containerView" stays behind of its RootView. It's generally not ok to modify global state like style on the root window. Also looking at TextView, the more flexible way to implement pointer icon is to override onResolvePointerIcon, which is passed an event, so you get the location as well: https://github.com/android/platform_frameworks_base/blob/master/core/java/and... Then looking at onResolvePointerIcon in ViewGroup.java, I think if the pointer event is delivered to the view, then onResolvePointerIcon on that view should be called. So I think setPointerIcon on the container view should just work as well. So I'd suggest investigating why containerview.onResolvePointerIcon doesn't work
https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:209: containerView.getRootView().setPointerIcon( On 2017/05/22 at 16:33:09, boliu wrote: > It's generally not ok to modify global state like style on the root window. It looks like the algorithm to hit test the view hierarchy for this is at http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/V... . We may have to change other View state to make the child Views appear on top of this for the purpose of mouse cursor hit testing. Any suggestion, Bo? > > Also looking at TextView, the more flexible way to implement pointer icon is to override onResolvePointerIcon, which is passed an event, so you get the location as well: The model for Blink is that it receives the usual MouseMove stream, and then Blink asynchronously sets the cursor whenever it wants. The location passed in onResolvePointerIcon is 100% useless as we are certainly not about to do a synchronous hit test. If I look at the base class implementation of setPointerIcon and onResolvePointerIcon, setPointerIcon just caches a value which onResolvePointerIcon returns, which is exactly what we want: http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/V... so I think we should just keep using setPointerIcon.
On 2017/05/22 22:31:14, aelias wrote: > https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... > File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): > > https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... > ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:209: > containerView.getRootView().setPointerIcon( > On 2017/05/22 at 16:33:09, boliu wrote: > > It's generally not ok to modify global state like style on the root window. > > It looks like the algorithm to hit test the view hierarchy for this is at > http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/V... > . We may have to change other View state to make the child Views appear on top > of this for the purpose of mouse cursor hit testing. Any suggestion, Bo? Shouldn't the view that receive input event be the container view. That would be ContentView in chrome? Or maybe that's not how things work, then I guess if chrome needs to do something special, then this logic needs to be moved up to chrome/android_webview layer. It's definitely wrong to call setPointerIcon in the root view for webview. But if chrome doesn't care, then chrome can call it. > > > > > Also looking at TextView, the more flexible way to implement pointer icon is > to override onResolvePointerIcon, which is passed an event, so you get the > location as well: > > The model for Blink is that it receives the usual MouseMove stream, and then > Blink asynchronously sets the cursor whenever it wants. The location passed in > onResolvePointerIcon is 100% useless as we are certainly not about to do a > synchronous hit test. If I look at the base class implementation of > setPointerIcon and onResolvePointerIcon, setPointerIcon just caches a value > which onResolvePointerIcon returns, which is exactly what we want: > http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/V... > so I think we should just keep using setPointerIcon.
On 2017/05/22 22:48:26, boliu wrote: > On 2017/05/22 22:31:14, aelias wrote: > > > https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... > > File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java > (right): > > > > > https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... > > ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:209: > > containerView.getRootView().setPointerIcon( > > On 2017/05/22 at 16:33:09, boliu wrote: > > > It's generally not ok to modify global state like style on the root window. > > > > It looks like the algorithm to hit test the view hierarchy for this is at > > > http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/V... > > . We may have to change other View state to make the child Views appear on > top > > of this for the purpose of mouse cursor hit testing. Any suggestion, Bo? > > Shouldn't the view that receive input event be the container view. That would be > ContentView in chrome? Or maybe that's not how things work, then I guess if > chrome needs to do something special, then this logic needs to be moved up to > chrome/android_webview layer. It's definitely wrong to call setPointerIcon in > the root view for webview. But if chrome doesn't care, then chrome can call it. > > > > > > > > > Also looking at TextView, the more flexible way to implement pointer icon is > > to override onResolvePointerIcon, which is passed an event, so you get the > > location as well: > > > > The model for Blink is that it receives the usual MouseMove stream, and then > > Blink asynchronously sets the cursor whenever it wants. The location passed > in > > onResolvePointerIcon is 100% useless as we are certainly not about to do a > > synchronous hit test. If I look at the base class implementation of > > setPointerIcon and onResolvePointerIcon, setPointerIcon just caches a value > > which onResolvePointerIcon returns, which is exactly what we want: > > > http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/V... > > so I think we should just keep using setPointerIcon. I just confirm it works fine to call setPointerIcon in the root view for webview.
PTAL
I was added to this mid-review; what do you want me to look at? On Tue, May 23, 2017 at 5:08 AM, <jaebaek@chromium.org> wrote: > PTAL > > https://codereview.chromium.org/2878403002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/23 09:08:29, jaebaek wrote: > On 2017/05/22 22:48:26, boliu wrote: > > On 2017/05/22 22:31:14, aelias wrote: > > > > > > https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... > > > File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java > > (right): > > > > > > > > > https://codereview.chromium.org/2878403002/diff/140001/ui/android/java/src/or... > > > ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:209: > > > containerView.getRootView().setPointerIcon( > > > On 2017/05/22 at 16:33:09, boliu wrote: > > > > It's generally not ok to modify global state like style on the root > window. > > > > > > It looks like the algorithm to hit test the view hierarchy for this is at > > > > > > http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/V... > > > . We may have to change other View state to make the child Views appear on > > top > > > of this for the purpose of mouse cursor hit testing. Any suggestion, Bo? > > > > Shouldn't the view that receive input event be the container view. That would > be > > ContentView in chrome? Or maybe that's not how things work, then I guess if > > chrome needs to do something special, then this logic needs to be moved up to > > chrome/android_webview layer. It's definitely wrong to call setPointerIcon in > > the root view for webview. But if chrome doesn't care, then chrome can call > it. > > > > > > > > > > > > > Also looking at TextView, the more flexible way to implement pointer icon > is > > > to override onResolvePointerIcon, which is passed an event, so you get the > > > location as well: > > > > > > The model for Blink is that it receives the usual MouseMove stream, and then > > > Blink asynchronously sets the cursor whenever it wants. The location passed > > in > > > onResolvePointerIcon is 100% useless as we are certainly not about to do a > > > synchronous hit test. If I look at the base class implementation of > > > setPointerIcon and onResolvePointerIcon, setPointerIcon just caches a value > > > which onResolvePointerIcon returns, which is exactly what we want: > > > > > > http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/V... > > > so I think we should just keep using setPointerIcon. > > I just confirm it works fine to call setPointerIcon in the root view for > webview. The issue is not that it won't (happen) to work on webview. The issue is webview should not modify any global state, period. I mean just imagine having two webviews in the view tree, then the two webviews will be basically fighting each other about setting the correct pointer, and since everything is asynchronous in webview, there is no guarantee it works at all. For webview, should call setPointerIcon directly on the container view, not the root view. Just plumb this up to ViewAndroidDelegate so that chrome and webview can do different things?
https://codereview.chromium.org/2878403002/diff/200001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/200001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:157: case WebCursorInfoType.TYPE_EAST_WEST_RESIZE: These are supposed to look different so let's not collapse them. https://codereview.chromium.org/2878403002/diff/200001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:206: default: Please remove the "default:". It's better to have a compile-time error than a runtime assert. https://codereview.chromium.org/2878403002/diff/220001/ui/base/cursor/cursors... File ui/base/cursor/cursors_android.cc (right): https://codereview.chromium.org/2878403002/diff/220001/ui/base/cursor/cursors... ui/base/cursor/cursors_android.cc:58: .GetImageSkiaNamed(resource_id) Hmm, sorry to go back on previous code review suggestion, but I changed my mind about how best to add our own custom cursors. This also will mdpi, xhdpi and accessibility-large cursors, as well as anchor points, based on what the Android cursors have. This will be hard to do using the C++ custom-cursor path -- it should probably be done as Android resources instead. But as we don't have proper PNGs for all of these formats yet, we should just leave them all to the default cursor in this patch.
boliu@, Ah! That is a good exception case. Thank you for letting me know it. I updated that part. Nico@, I added you because "git cl pre" told me I need to add you to get LGTM in term of CL for DEPS. PTAL https://codereview.chromium.org/2878403002/diff/200001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/200001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:157: case WebCursorInfoType.TYPE_EAST_WEST_RESIZE: On 2017/05/23 22:43:25, aelias wrote: > These are supposed to look different so let's not collapse them. Done. https://codereview.chromium.org/2878403002/diff/200001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:206: default: On 2017/05/23 22:43:25, aelias wrote: > Please remove the "default:". It's better to have a compile-time error than a > runtime assert. Done. https://codereview.chromium.org/2878403002/diff/220001/ui/base/cursor/cursors... File ui/base/cursor/cursors_android.cc (right): https://codereview.chromium.org/2878403002/diff/220001/ui/base/cursor/cursors... ui/base/cursor/cursors_android.cc:58: .GetImageSkiaNamed(resource_id) On 2017/05/23 22:43:26, aelias wrote: > Hmm, sorry to go back on previous code review suggestion, but I changed my mind > about how best to add our own custom cursors. This also will mdpi, xhdpi and > accessibility-large cursors, as well as anchor points, based on what the Android > cursors have. This will be hard to do using the C++ custom-cursor path -- it > should probably be done as Android resources instead. But as we don't have > proper PNGs for all of these formats yet, we should just leave them all to the > default cursor in this patch. Can I just return false for all the cases here and comment TODO? Or do you mean that I remove all this code (i.e., this file)?
What exactly in the DEPS file do you need me for? It's mostly deps on tp/WebKit/public, which I don't own. dcheng, can you look at the WebKit/public DEPS entries? https://codereview.chromium.org/2878403002/diff/240001/ui/android/DEPS File ui/android/DEPS (right): https://codereview.chromium.org/2878403002/diff/240001/ui/android/DEPS#newcode12 ui/android/DEPS:12: "+third_party/WebKit/public/platform/WebCursorInfo.h", dcheng: ^
what's with new changes in ui/base? Does that need to be part of this CL? In general, don't add more code to a CL once review has started, unless specifically requested by reviewers. And mention that in comments. and for the second time, tests? https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:107: getContainerView().getRootView().setPointerIcon( same issue here https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:113: public void onCursorChanged(int cursorType) { private https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:238: public void updatePointerIcon(int type) { methods that embedders override should be no-op document what int type is https://codereview.chromium.org/2878403002/diff/240001/ui/base/cursor/cursors... File ui/base/cursor/cursors_android.cc (right): https://codereview.chromium.org/2878403002/diff/240001/ui/base/cursor/cursors... ui/base/cursor/cursors_android.cc:41: default: avoid default https://codereview.chromium.org/2878403002/diff/240001/ui/base/cursor/cursors... File ui/base/cursor/cursors_android.h (right): https://codereview.chromium.org/2878403002/diff/240001/ui/base/cursor/cursors... ui/base/cursor/cursors_android.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c)
https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:227: assert false : "These pointer icon types are not supported"; As Java asserts are not stripped even in release builds, asserting is a unnecessarily heavyweight for these in case Blink ever sends them (probably in the context of setting --enable-blink-features=MiddleClickAutoscroll ). As we're going with the approach of mapping to close-enough cursors, I suggest mapping them to TYPE_ALL_SCROLL as well. https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:229: case WebCursorInfoType.TYPE_MOVE: Please map MOVE and MIDDLE_PANNING to "TYPE_ALL_SCROLL" and delete the ui/base code. According to https://developer.android.com/reference/android/R.attr.html#pointerIcon , "ALL_SCROLL" is the four-way-arrow PNG which I've observed is used for these two types on ChromeOS.
https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:227: assert false : "These pointer icon types are not supported"; On 2017/05/24 19:03:56, aelias wrote: > As Java asserts are not stripped even in release builds, asserting is a > unnecessarily heavyweight for these in case Blink ever sends them (probably in > the context of setting --enable-blink-features=MiddleClickAutoscroll ). As > we're going with the approach of mapping to close-enough cursors, I suggest > mapping them to TYPE_ALL_SCROLL as well. Actually, agrieve made java asserts work just like DCHECK now. Enabled on debug builds everywhere, and stripped out of release builds. It was some black magic that involved modifying byte code after compilation, but it works..
The last commit is for testing mouse cursor. I tried DOMUtils.clickNode(), MotionEvent + OnGenericMotionListener(), and instrumentation.sendPointerSync(), but failed in input mouse cursor (I checked other types of inputs such as touch work fine). I guess SOURCE_MOUSE_RELATIVE is needed to input mouse cursor motion event (https://developer.android.com/reference/android/view/InputDevice.html). It is available from Android O. Do you have any suggestions to input mouse cursor? https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:107: getContainerView().getRootView().setPointerIcon( On 2017/05/24 18:24:07, boliu wrote: > same issue here Done. https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:113: public void onCursorChanged(int cursorType) { On 2017/05/24 18:24:07, boliu wrote: > private Done. https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:227: assert false : "These pointer icon types are not supported"; On 2017/05/24 19:14:02, boliu wrote: > On 2017/05/24 19:03:56, aelias wrote: > > As Java asserts are not stripped even in release builds, asserting is a > > unnecessarily heavyweight for these in case Blink ever sends them (probably in > > the context of setting --enable-blink-features=MiddleClickAutoscroll ). As > > we're going with the approach of mapping to close-enough cursors, I suggest > > mapping them to TYPE_ALL_SCROLL as well. > > Actually, agrieve made java asserts work just like DCHECK now. Enabled on debug > builds everywhere, and stripped out of release builds. It was some black magic > that involved modifying byte code after compilation, but it works.. Ok, then I will keep this assert. Thank you for the good information. https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:229: case WebCursorInfoType.TYPE_MOVE: On 2017/05/24 19:03:56, aelias wrote: > Please map MOVE and MIDDLE_PANNING to "TYPE_ALL_SCROLL" and delete the ui/base > code. According to > https://developer.android.com/reference/android/R.attr.html#pointerIcon , > "ALL_SCROLL" is the four-way-arrow PNG which I've observed is used for these two > types on ChromeOS. Done. https://codereview.chromium.org/2878403002/diff/240001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:238: public void updatePointerIcon(int type) { On 2017/05/24 18:24:07, boliu wrote: > methods that embedders override should be no-op > > document what int type is Done. https://codereview.chromium.org/2878403002/diff/240001/ui/base/cursor/cursors... File ui/base/cursor/cursors_android.cc (right): https://codereview.chromium.org/2878403002/diff/240001/ui/base/cursor/cursors... ui/base/cursor/cursors_android.cc:41: default: On 2017/05/24 18:24:07, boliu wrote: > avoid default I also drop this file. https://codereview.chromium.org/2878403002/diff/240001/ui/base/cursor/cursors... File ui/base/cursor/cursors_android.h (right): https://codereview.chromium.org/2878403002/diff/240001/ui/base/cursor/cursors... ui/base/cursor/cursors_android.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/24 18:24:07, boliu wrote: > no (c) Since the current CL does not need this, I just drop this file.
Added an instrumentation test. PTAL.
https://codereview.chromium.org/2878403002/diff/340001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java (right): https://codereview.chromium.org/2878403002/diff/340001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:87: moveCursor(11.0f, 20.0f); how are you getting these coordinates? in general nothing should be hard coded, because these tests need to work on screens of different sizes. Maybe can use DOMUtils.getClickTargetForNode to get the right nodes here? https://codereview.chromium.org/2878403002/diff/340001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:89: Thread.sleep(20); no sleeps, you are polling already. do you really need to sleep? also prefer CallbackHelper (ie signaling when a callback happens) over polling. Should be able to get the ShellViewAndroidDelegate and add a CallbackHelper for pointer change?
lgtm modulo Bo's test comments and a nit https://codereview.chromium.org/2878403002/diff/340001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/340001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:246: public void updatePointerIcon(PointerIcon icon) {} Please use "abstract" instead of default implementation.
boliu@ Could you please check CallbackHelper part? I added CallbackHelper rather than poll, but not sure my implementation follows convention. In particular, I used Runnable just as a simple callback interface (not multi-thread). It is fine? Must I define a new interface? https://codereview.chromium.org/2878403002/diff/340001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java (right): https://codereview.chromium.org/2878403002/diff/340001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:87: moveCursor(11.0f, 20.0f); On 2017/05/30 21:30:02, boliu wrote: > how are you getting these coordinates? in general nothing should be hard coded, > because these tests need to work on screens of different sizes. Maybe can use > DOMUtils.getClickTargetForNode to get the right nodes here? Done. https://codereview.chromium.org/2878403002/diff/340001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:89: Thread.sleep(20); On 2017/05/30 21:30:02, boliu wrote: > no sleeps, you are polling already. do you really need to sleep? > > also prefer CallbackHelper (ie signaling when a callback happens) over polling. > Should be able to get the ShellViewAndroidDelegate and add a CallbackHelper for > pointer change? Done. https://codereview.chromium.org/2878403002/diff/340001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/340001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:246: public void updatePointerIcon(PointerIcon icon) {} On 2017/05/30 23:26:35, aelias wrote: > Please use "abstract" instead of default implementation. Done.
https://codereview.chromium.org/2878403002/diff/360001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:29: @MinAndroidSdkLevel(Build.VERSION_CODES.N) which part of this test requires N specifically? we don't have N bots on cq, which means N-only tests are kind of useless. Maybe there's some way around it.. https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... File content/shell/android/java/src/org/chromium/content_shell/ShellViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... content/shell/android/java/src/org/chromium/content_shell/ShellViewAndroidDelegate.java:39: if (mOnCursorUpdate != null) mOnCursorUpdate.run(); generally, the pattern is this class holds a CallbackHelper subclass where notifyCalled takes a parameter that's being checked, in this case PointerIcon. then test can get it directly from CallbackHelper after waitForCallback. Example: https://cs.chromium.org/chromium/src/content/public/test/android/javatests/sr... https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:121: .getActiveShell() activity accessors should happen on ui thread. use mCallback.runOnUiThreadForTestCommon
https://codereview.chromium.org/2878403002/diff/360001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:31: public class ContentViewPointerIconTest extends ContentShellTestBase { also, most of content test have already been converted to junit4 tests, ie ones that do not inherit from ContentShellTestBase Example: https://cs.chromium.org/chromium/src/content/public/android/javatests/src/org... So use @RunWith(ContentJUnit4ClassRunner.class) @Test @Before/@After ContentShellActivityTestRule etc
boliu@, still unclear in terms of Android N issue .. If ShellViewAndroidDelegate overrides onCursorChange(), the overridden onCursorChange() method does nothing and just return? and must I make the test always passed? onCursorChange() will not change the cursor if Android version is not N. Even though I remove Android N target annotation from the test, the test is still meaningless. Is your suggestion that just making the test pass even though it does not actually test the cursor change? https://codereview.chromium.org/2878403002/diff/360001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:29: @MinAndroidSdkLevel(Build.VERSION_CODES.N) On 2017/05/31 16:30:03, boliu wrote: > which part of this test requires N specifically? > > we don't have N bots on cq, which means N-only tests are kind of useless. Maybe > there's some way around it.. I am sorry but this is still unclear .. Even though we avoid N-only tests here, the mouse cursor setting is still available only for Android N (or upper maybe). Running this test in other versions is meaningless because the mouse cursor will not be updated nor tested. https://codereview.chromium.org/2878403002/diff/360001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:31: public class ContentViewPointerIconTest extends ContentShellTestBase { On 2017/05/31 16:32:47, boliu wrote: > also, most of content test have already been converted to junit4 tests, ie ones > that do not inherit from ContentShellTestBase > > Example: > https://cs.chromium.org/chromium/src/content/public/android/javatests/src/org... > > So use > @RunWith(ContentJUnit4ClassRunner.class) > @Test > @Before/@After > ContentShellActivityTestRule > > etc Done. https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... File content/shell/android/java/src/org/chromium/content_shell/ShellViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... content/shell/android/java/src/org/chromium/content_shell/ShellViewAndroidDelegate.java:39: if (mOnCursorUpdate != null) mOnCursorUpdate.run(); On 2017/05/31 16:30:03, boliu wrote: > generally, the pattern is this class holds a CallbackHelper subclass where > notifyCalled takes a parameter that's being checked, in this case PointerIcon. > then test can get it directly from CallbackHelper after waitForCallback. > Example: > https://cs.chromium.org/chromium/src/content/public/test/android/javatests/sr... Done. https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:121: .getActiveShell() On 2017/05/31 16:30:03, boliu wrote: > activity accessors should happen on ui thread. use > mCallback.runOnUiThreadForTestCommon Ok, I will update it in that way, but have a question. Why do other methods call "getActiveShell()" directly?
On 2017/06/01 06:20:56, jaebaek wrote: > boliu@, still unclear in terms of Android N issue .. > If ShellViewAndroidDelegate overrides onCursorChange(), the overridden > onCursorChange() method does nothing and just return? and must I make the test > always passed? The fact that onCursorChange is called is still a meaningful test. Because it's possible the hook up before onCursorChanged is broken. You can also check it got the correct WebCursorInfoType, ie OnCursorUpdateHelper saves an int rather than PointerIcon. > > onCursorChange() will not change the cursor if Android version is not N. Even > though I remove Android N target annotation from the test, the test is still > meaningless. > > Is your suggestion that just making the test pass even though it does not > actually test the cursor change? > https://codereview.chromium.org/2878403002/diff/360001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerIconTest.java:29: @MinAndroidSdkLevel(Build.VERSION_CODES.N) On 2017/06/01 06:20:56, jaebaek wrote: > On 2017/05/31 16:30:03, boliu wrote: > > which part of this test requires N specifically? > > > > we don't have N bots on cq, which means N-only tests are kind of useless. > Maybe > > there's some way around it.. > > I am sorry but this is still unclear .. > Even though we avoid N-only tests here, the mouse cursor setting is still > available only for Android N (or upper maybe). > Running this test in other versions is meaningless because the mouse cursor will > not be updated nor tested. Just check WebCursorInfoType in ViewAndroidDelegate.onCursorChanged? Don't bother testing setPointerIcon https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java (right): https://codereview.chromium.org/2878403002/diff/360001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:121: .getActiveShell() On 2017/06/01 06:20:56, jaebaek wrote: > On 2017/05/31 16:30:03, boliu wrote: > > activity accessors should happen on ui thread. use > > mCallback.runOnUiThreadForTestCommon > > Ok, I will update it in that way, but have a question. > Why do other methods call "getActiveShell()" directly? they are probably also unsafe, in theory
PTAL
Ah .. I am sorry .. I rebased the code and it resulted in mixed CL. I will separate my CL and rebase later. In the very last CL (i.e., 20), the following file is not modified by me (it is from rebase): content/browser/renderer_host/render_widget_host_view_android.cc
https://codereview.chromium.org/2878403002/diff/400001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java (right): https://codereview.chromium.org/2878403002/diff/400001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:34: @MinAndroidSdkLevel(Build.VERSION_CODES.N) drop these two and make sure test runs fine on cq? https://codereview.chromium.org/2878403002/diff/400001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:90: Assert.assertTrue(mActivityTestRule.getOnCursorUpdateHelper().getPointerType() == type); nit: there's assertEquals https://codereview.chromium.org/2878403002/diff/400001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java (right): https://codereview.chromium.org/2878403002/diff/400001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:128: .getViewAndroidDelegate() these two should be on UI thread as well
It works fine. https://codereview.chromium.org/2878403002/diff/400001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java (right): https://codereview.chromium.org/2878403002/diff/400001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:34: @MinAndroidSdkLevel(Build.VERSION_CODES.N) On 2017/06/02 15:54:41, boliu wrote: > drop these two and make sure test runs fine on cq? Done. https://codereview.chromium.org/2878403002/diff/400001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:90: Assert.assertTrue(mActivityTestRule.getOnCursorUpdateHelper().getPointerType() == type); On 2017/06/02 15:54:41, boliu wrote: > nit: there's assertEquals Done. https://codereview.chromium.org/2878403002/diff/400001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java (right): https://codereview.chromium.org/2878403002/diff/400001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java:128: .getViewAndroidDelegate() On 2017/06/02 15:54:41, boliu wrote: > these two should be on UI thread as well Done.
lgtm % nit https://codereview.chromium.org/2878403002/diff/420001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java (right): https://codereview.chromium.org/2878403002/diff/420001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:85: Assert.assertEquals(mActivityTestRule.getOnCursorUpdateHelper().getPointerType(), type); nit: expected first, actual second
jaebaek@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@, Could you please review the CL of the following file? chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java chrome/android/java/src/org/chromium/chrome/browser/tab/TabViewAndroidDelegate.java
https://codereview.chromium.org/2878403002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabViewAndroidDelegate.java:56: getContainerView().getRootView().setPointerIcon(icon); since this mutates the root view, what happens in cases where the tab crashes, the tab is switched to another tab via a keyboard shortcut, the mouse is disconnected, move focus outside of the view (i.e. it is full page textarea in the web contents but you move to the omnibox, does it get reset)? is this called often enough to reset to the initial value? I just want to make sure that we don't leave the cursor in a bad state ever.
I will dig into the container view issue more. https://codereview.chromium.org/2878403002/diff/420001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java (right): https://codereview.chromium.org/2878403002/diff/420001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:85: Assert.assertEquals(mActivityTestRule.getOnCursorUpdateHelper().getPointerType(), type); On 2017/06/07 02:38:09, boliu_OOO_june_5-6 wrote: > nit: expected first, actual second Done. https://codereview.chromium.org/2878403002/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabViewAndroidDelegate.java (right): https://codereview.chromium.org/2878403002/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabViewAndroidDelegate.java:56: getContainerView().getRootView().setPointerIcon(icon); On 2017/06/07 19:27:18, Ted C wrote: > since this mutates the root view, what happens in cases where the tab crashes, > the tab is switched to another tab via a keyboard shortcut, the mouse is > disconnected, move focus outside of the view (i.e. it is full page textarea in > the web contents but you move to the omnibox, does it get reset)? > > is this called often enough to reset to the initial value? I just want to make > sure that we don't leave the cursor in a bad state ever. It seems we cannot make the pointer icon visible by setting it in the container view. (as far as I tested, we must set it to parent of parent of container view) When testing cases you mentioned: tab crashes --> the pointer icon disappears. (it becomes visible when I move it) switch to another tab --> the same as tab crashes. mouse disconnected --> the pointer icon remains around 10 seconds and disappears. move focus outside of the view --> it gets reset. When observing its behavior: move event --> it is updated quickly (almost realtime). keyboard or touch event --> disappears after 1 second. (during the short time until it disappears, it is updated if needed) no event --> the pointer icon remains around 10 seconds and disappears. (the same as mouse disconnected) It looks fine, but I also dislike this hacky solution .. let me check why we cannot change the pointer icon only by setting it to the container view.
I tried to identify the exact View to correctly set the pointer icon. The ancestors of the content view are (I get it by calling getParent() recursively): org.chromium.chrome.browser.compositor.CompositorViewHolder android.support.design.widget.CoordinatorLayout android.widget.LinearLayout android.support.v7.widget.ContentFrameLayout android.support.v7.widget.FitWindowsFrameLayout android.widget.FrameLayout android.widget.LinearLayout com.android.internal.policy.DecorView android.view.ViewRootImpl android.support.design.widget.CoordinatorLayout is the parent of the parent of the content view. It has 11 children (see chrome/android/java/res/layout/main.xml) and FrameLayout among them is the one that we can set the pointer icon. It has bottom_sheet_snackbar_container as its id (i.e., SnackBar). Must we set pointer icon again SnackBar rather than the root view?
On 2017/06/08 01:00:11, jaebaek wrote: > I will dig into the container view issue more. > > https://codereview.chromium.org/2878403002/diff/420001/content/public/android... > File > content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java > (right): > > https://codereview.chromium.org/2878403002/diff/420001/content/public/android... > content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:85: > Assert.assertEquals(mActivityTestRule.getOnCursorUpdateHelper().getPointerType(), > type); > On 2017/06/07 02:38:09, boliu_OOO_june_5-6 wrote: > > nit: expected first, actual second > > Done. > > https://codereview.chromium.org/2878403002/diff/440001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/tab/TabViewAndroidDelegate.java > (right): > > https://codereview.chromium.org/2878403002/diff/440001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/tab/TabViewAndroidDelegate.java:56: > getContainerView().getRootView().setPointerIcon(icon); > On 2017/06/07 19:27:18, Ted C wrote: > > since this mutates the root view, what happens in cases where the tab crashes, > > the tab is switched to another tab via a keyboard shortcut, the mouse is > > disconnected, move focus outside of the view (i.e. it is full page textarea in > > the web contents but you move to the omnibox, does it get reset)? > > > > is this called often enough to reset to the initial value? I just want to > make > > sure that we don't leave the cursor in a bad state ever. > > It seems we cannot make the pointer icon visible by setting it in the container > view. > (as far as I tested, we must set it to parent of parent of container view) Hmm...that is unfortunate. Ideally, we should just be setting the pointer icon on the container view itself. I would look at the ViewGroup code that decides what view should receive the pointer: https://android.googlesource.com/platform/frameworks/base.git/+/master/core/j... It should be dispatching to the children that are consuming the pointer events, but I know that both the CompositorViewHolder and CoordinatorLayout do special things around events. We might need to update those classes to properly resolve the pointer. I would just override the methods in the CompositorViewHolder to see if it is being called at all. If not, we could extend the CoordinatorLayout to see what it is doing. Having to set it at anything but the leaf view itself seems like a bad thing and we should figure out why it is required (and it might be outside of our control, but I think we need to dig a bit more to figure out why). > > When testing cases you mentioned: > tab crashes --> the pointer icon disappears. > (it becomes visible when I move it) > switch to another tab --> the same as tab crashes. > mouse disconnected --> the pointer icon remains around 10 seconds and > disappears. > move focus outside of the view --> it gets reset. > > When observing its behavior: > move event --> it is updated quickly (almost realtime). > keyboard or touch event --> disappears after 1 second. > (during the short time until it disappears, it is updated if needed) > no event --> the pointer icon remains around 10 seconds and disappears. > (the same as mouse disconnected) > > It looks fine, but I also dislike this hacky solution .. let me check why we > cannot change the pointer icon only by setting it to the container view.
2017. 6. 8. 오후 11:24에 <tedchoc@chromium.org>님이 작성: On 2017/06/08 01:00:11, jaebaek wrote: > I will dig into the container view issue more. > > https://codereview.chromium.org/2878403002/diff/420001/ content/public/android/javatests/src/org/chromium/content/browser/ ContentViewPointerTypeTest.java > File > content/public/android/javatests/src/org/chromium/content/browser/ ContentViewPointerTypeTest.java > (right): > > https://codereview.chromium.org/2878403002/diff/420001/ content/public/android/javatests/src/org/chromium/content/browser/ ContentViewPointerTypeTest.java#newcode85 > content/public/android/javatests/src/org/chromium/content/browser/ ContentViewPointerTypeTest.java:85: > Assert.assertEquals(mActivityTestRule.getOnCursorUpdateHelper(). getPointerType(), > type); > On 2017/06/07 02:38:09, boliu_OOO_june_5-6 wrote: > > nit: expected first, actual second > > Done. > > https://codereview.chromium.org/2878403002/diff/440001/ chrome/android/java/src/org/chromium/chrome/browser/tab/ TabViewAndroidDelegate.java > File > chrome/android/java/src/org/chromium/chrome/browser/tab/ TabViewAndroidDelegate.java > (right): > > https://codereview.chromium.org/2878403002/diff/440001/ chrome/android/java/src/org/chromium/chrome/browser/tab/ TabViewAndroidDelegate.java#newcode56 > chrome/android/java/src/org/chromium/chrome/browser/tab/ TabViewAndroidDelegate.java:56: > getContainerView().getRootView().setPointerIcon(icon); > On 2017/06/07 19:27:18, Ted C wrote: > > since this mutates the root view, what happens in cases where the tab crashes, > > the tab is switched to another tab via a keyboard shortcut, the mouse is > > disconnected, move focus outside of the view (i.e. it is full page textarea in > > the web contents but you move to the omnibox, does it get reset)? > > > > is this called often enough to reset to the initial value? I just want to > make > > sure that we don't leave the cursor in a bad state ever. > > It seems we cannot make the pointer icon visible by setting it in the container > view. > (as far as I tested, we must set it to parent of parent of container view) Hmm...that is unfortunate. Ideally, we should just be setting the pointer icon on the container view itself. I would look at the ViewGroup code that decides what view should receive the pointer: https://android.googlesource.com/platform/frameworks/base. git/+/master/core/java/android/view/ViewGroup.java#1678 Interesting. Actually I did similar things to check which view (child) has effects on the point icon. It should be dispatching to the children that are consuming the pointer events, but I know that both the CompositorViewHolder and CoordinatorLayout do special things around events. We might need to update those classes to properly resolve the pointer. I would just override the methods in the CompositorViewHolder to see if it is being called at all. If not, we could extend the CoordinatorLayout to see what it is doing. Make sense. I will try this. Having to set it at anything but the leaf view itself seems like a bad thing and we should figure out why it is required (and it might be outside of our control, but I think we need to dig a bit more to figure out why). > > When testing cases you mentioned: > tab crashes --> the pointer icon disappears. > (it becomes visible when I move it) > switch to another tab --> the same as tab crashes. > mouse disconnected --> the pointer icon remains around 10 seconds and > disappears. > move focus outside of the view --> it gets reset. > > When observing its behavior: > move event --> it is updated quickly (almost realtime). > keyboard or touch event --> disappears after 1 second. > (during the short time until it disappears, it is updated if needed) > no event --> the pointer icon remains around 10 seconds and disappears. > (the same as mouse disconnected) > > It looks fine, but I also dislike this hacky solution .. let me check why we > cannot change the pointer icon only by setting it to the container view. https://codereview.chromium.org/2878403002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
When extending ContentView and CompositorViewHolder to override onResolvePointerIcon(), I checked both are not invoked. I guess it is mainly because the CoordinatorLayout.onResolvePointerIcon() sorts its children views and determine SnackBar is the correct child which we must show its pointer icon. To force ContentView to be chosen instead, I added a class derived from CoordinatorLayout and updated onResolvePointerIcon(). Do you think this is a right direction?
tedchoc@chromium.org changed reviewers: + mdjones@chromium.org
+mdjones Looking at CoordinatorLayout, it doesn't actually override onResolvePointerIcon, so it is relying on the underlying ViewGroup implementation. Based on the fact that you are seeing the snackbar get accessed, I wonder if the snackbar view is just too large and obscuring the rest of the contentview. @Matt, I know there was a bug where we weren't shrinking the infobar container, but does that affect snackbars as well? Are those in the same viewgroup and thus compounding this? With your fix as proposed, it would prevent the pointer from being correct if a snackbar was present (and in fact it would show the underlying web contents pointer if you were to hover over a snackbar). I do think this is on the right path, but I would look at hierarchyviewer to see the view stacking order to see what is being hit. To me, it looks like the snackbar view isn't hidden as it should be and/or it is too large and thus is returning true when View#pointInView is called (https://android.googlesource.com/platform/frameworks/base.git/+/master/core/j...)
To exactly understand what happens in onResolvePointerIcon() of the initial CoordinatorLayout, I used java reflection to replay the implementation of onResolvePointerIcon() as shown in https://android.googlesource.com/platform/frameworks/base.git/+/master/core/j... I tested it in onResolvePointerIcon() of CoordinatorLayoutForPointer (i.e., the class extends the initial CoordinatorLayout). buildOrderedChildList() returns null. isChildrenDrawingOrderEnabled() is false. Thus, the control flow makes the last child chosen among those listed in https://cs.chromium.org/chromium/src/chrome/android/java/res/layout/main.xml?... and the last child is the snackbar. isTransformedTouchPointInView(x, y, child, point) is true when "child" is the snackbar. When I printed (width, height, z) of all children, it shows: 06-13 14:43:03.570 18904 18904 E onResolvePointerIcon: child #0: z = 0.0, w = 1080, h = 1731 06-13 14:43:03.570 18904 18904 E onResolvePointerIcon: child #1: z = 0.0, w = 1080, h = 1731 06-13 14:43:03.570 18904 18904 E onResolvePointerIcon: child #2: z = 0.0, w = 1080, h = 1731 06-13 14:43:03.570 18904 18904 E onResolvePointerIcon: child #3: z = 0.0, w = 0, h = 0 06-13 14:43:03.570 18904 18904 E onResolvePointerIcon: child #4: z = 0.0, w = 1080, h = 1731 06-13 14:43:03.570 18904 18904 E onResolvePointerIcon: child #5: z = 0.0, w = 0, h = 0 06-13 14:43:03.570 18904 18904 E onResolvePointerIcon: child #6: z = 0.0, w = 1080, h = 147 06-13 14:43:03.570 18904 18904 E onResolvePointerIcon: child #7: z = 0.0, w = 1080, h = 173 06-13 14:43:03.571 18904 18904 E onResolvePointerIcon: child #8: z = 0.0, w = 0, h = 0 06-13 14:43:03.571 18904 18904 E onResolvePointerIcon: child #9: z = 0.0, w = 0, h = 0 06-13 14:43:03.571 18904 18904 E onResolvePointerIcon: child #10: z = 0.0, w = 1080, h = 1877 06-13 14:43:03.571 18904 18904 E onResolvePointerIcon: preorderedList = null 06-13 14:43:03.571 18904 18904 E onResolvePointerIcon: isTransformedTouchPointInView: true 06-13 14:43:03.571 18904 18904 E onResolvePointerIcon: isChildrenDrawingOrderEnabled: false "child #10" is the snackbar. It is covers the boundary of the compositor view holder. Since it is wide enough, isTransformedTouchPointInView(x, y, child, point) returns true. An alternative is to set the drawing order https://developer.android.com/reference/android/view/ViewGroup.html#setChildr... https://developer.android.com/reference/android/view/ViewGroup.html#getChildD..., int) but it will cause side effects on event handling. There are some event handler that are dependent on the drawing order. Thus, we have two choices: 1. override onResolvePointerIcon as the current one. 2. change the size of snackbar. The current one sets the pointer as default (i.e., arrow) when the pointer hovers the snackbar (maybe blink recognizes it and choose the arrow pointer).
I'd suggest changing the size of the snackbar view to reflect its true visible size at all times. (This probably means changing it to make it dynamically sized instead of fixed in advance.) That sounds like the principled way to solve this problem.
The CQ bit was checked by jaebaek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2878403002/#ps540001 (title: "Resize views to show pointer icon (rebase)")
The CQ bit was unchecked by jaebaek@chromium.org
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I resized BottomContainer (snackbar) and FadingBackgroundView to make the pointer icon of CompositorViewHolder selected when onResolvePointerIcon() is called. And CompositorViewHolder overrides onResolvePointerIcon() to choose ContentViewCore's pointer icon as its pointer icon. PTAL
https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java (right): https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:249: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { Version check is repeated more often than necessary. I think the checking in |setPointerIcon| is enough. https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java (left): https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java:146: int keyboardHeight = mParent.getHeight() - mCurrentVisibleRect.bottom Why is it okay not to take the keyboard height into account any more? https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java (right): https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java:297: private void setPointerIcon(View view, int type) { static? https://codereview.chromium.org/2878403002/diff/560001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java (right): https://codereview.chromium.org/2878403002/diff/560001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:85: Assert.assertEquals(type, mActivityTestRule.getOnCursorUpdateHelper().getPointerType()); s/mActivityTestRule.getOnCursorUpdateHelper()/onCursorUpdateHelper/
tedchoc@, could you please review the code? I resized view components listed in main.xml to make the pointer icon of CompositorViewHolder selected when onResolvePointerIcon() is called. And CompositorViewHolder overrides onResolvePointerIcon() to choose ContentViewCore's pointer icon as its pointer icon. https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java (right): https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:249: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2017/07/17 04:45:18, Jinsuk Kim wrote: > Version check is repeated more often than necessary. I think the checking in > |setPointerIcon| is enough. This is for PointerIcon.TYPE_HAND, but I will move PointerIcon.TYPE_HAND to setPointerIcon() method and change the name of method as setPointerIconAsHand() https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java (left): https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java:146: int keyboardHeight = mParent.getHeight() - mCurrentVisibleRect.bottom On 2017/07/17 04:45:19, Jinsuk Kim wrote: > Why is it okay not to take the keyboard height into account any more? Initially, layout_height of mParent was "match_parent". In that setting, if we do not give bottomMargin when keyboard or android home button is shown, it overlaps with mView (i.e., snackbar) and mView is partially not shown. Since I changed layout_height of mParent as "wrap_content", by default, mParent is placed on the top of keyboard or android home button without overlaps. Moreover, this margin calculation becomes a huge minus number because mParent.getHeight() is small and mView disappears. Thus, this bottomMargin must be removed. https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java (right): https://codereview.chromium.org/2878403002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java:297: private void setPointerIcon(View view, int type) { On 2017/07/17 04:45:19, Jinsuk Kim wrote: > static? Done. https://codereview.chromium.org/2878403002/diff/560001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java (right): https://codereview.chromium.org/2878403002/diff/560001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewPointerTypeTest.java:85: Assert.assertEquals(type, mActivityTestRule.getOnCursorUpdateHelper().getPointerType()); On 2017/07/17 04:45:19, Jinsuk Kim wrote: > s/mActivityTestRule.getOnCursorUpdateHelper()/onCursorUpdateHelper/ Done.
mdjones@ will be the best reviewer for the sizing changes here. In general, I think we'd be better off splitting off the sizing changes from the pointer fixes as they're going to be muddled. https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:26: android:layout_height="wrap_content" /> I'm pretty sure this needs to be match_parent. This is what shows the accessibility tab switcher and should take up the whole screen. That being said, I'm not sure why it isn't marked as gone. https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:59: android:layout_height="0dp" as mentioned in the other file, this doesn't seem right. we should be using visibility to trigger the layout. https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/BottomContainer.java (right): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/BottomContainer.java:42: int height = 0; Why do we need this? Is this view set as MATCH_PARENT? The logic you are adding seems like WRAP_CONTENT. https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java (left): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java:148: FrameLayout.LayoutParams lp = getLayoutParams(); this change will prevent snackbars from appearing about the keyboard. is that intentional? https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java (right): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java:103: changeHeight(0); changing the layout height in this case seems wrong to me. This seems like where we should be setting it to gone. I'll defer to mdjones@ though as I'm not familiar with these changes: Source: https://codereview.chromium.org/2709883002
tedchoc@, mdjones@, aelias@, As tedchoc pointed, it worries me side effects caused by this resizing. In particular, it seems that heights of some sibling views of CompositorViewHolder must be set as match_parent. How about just rolling back to the initial implementation with a new class derived from CoordinatorLayout that overrides onResolvePointerIcon()? https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:26: android:layout_height="wrap_content" /> On 2017/07/18 16:41:18, Ted C wrote: > I'm pretty sure this needs to be match_parent. This is what shows the > accessibility tab switcher and should take up the whole screen. That being > said, I'm not sure why it isn't marked as gone. If we must keep it as match_parent, there is no chance to set mouse cursor based on this resizing. It is mainly because onResolvePointerIcon() of ViewGroup chooses the later child first regardless of whether its visibility is gone or not. https://github.com/android/platform_frameworks_base/blob/master/core/java/and... I think we should roll it back to the way of creating a class derived from android.support.design.widget.CoordinatorLayout and overriding onResolvePointerIcon(). tedchoc@, mdjones@, aelias@, what is your opinion? https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:59: android:layout_height="0dp" On 2017/07/18 16:41:18, Ted C wrote: > as mentioned in the other file, this doesn't seem right. we should be using > visibility to trigger the layout. The reason behind this is because onResolvePointerIcon() of ViewGroup chooses this instead of CompositorViewHolder when the layout height is match_parent. https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java (left): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarView.java:148: FrameLayout.LayoutParams lp = getLayoutParams(); On 2017/07/18 16:41:18, Ted C wrote: > this change will prevent snackbars from appearing about the keyboard. is that > intentional? Yes, it is intentional. When we set the height of BottomContainer as wrap_content, snackbar is shown properly without this code. Rather, this bottomMargin hides snackbar because it becomes a large minus number.
https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:26: android:layout_height="wrap_content" /> On 2017/07/19 01:18:53, jaebaek wrote: > On 2017/07/18 16:41:18, Ted C wrote: > > I'm pretty sure this needs to be match_parent. This is what shows the > > accessibility tab switcher and should take up the whole screen. That being > > said, I'm not sure why it isn't marked as gone. > > If we must keep it as match_parent, there is no chance to > set mouse cursor based on this resizing. It is mainly because > onResolvePointerIcon() of ViewGroup chooses the later > child first regardless of whether its visibility is gone or not. > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > I think we should roll it back to the way of creating a class > derived from android.support.design.widget.CoordinatorLayout > and overriding onResolvePointerIcon(). > > tedchoc@, mdjones@, aelias@, what is your opinion? This will need to remain match parent. That being said, I agree with ted that this view should be 'gone' by default. Unfortunately, at quick glance it looks like visibility of the view isn't considered when resolving the pointer which makes this more difficult. I think making a coordinator that overrides onResolvePointerIcon is probably one of the least fragile ways to handle this. It can ignore views that are set to 'gone' (assuming I didn't miss something) and wouldn't require special logic for each new child in this view. https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:59: android:layout_height="0dp" On 2017/07/19 01:18:53, jaebaek wrote: > On 2017/07/18 16:41:18, Ted C wrote: > > as mentioned in the other file, this doesn't seem right. we should be using > > visibility to trigger the layout. > > The reason behind this is because onResolvePointerIcon() of > ViewGroup chooses this instead of CompositorViewHolder > when the layout height is match_parent. Does this view not return null for onResolvePointerIcon? Even if FadingBackground view consumes the whole screen, returning null should cause the coordinator layout to check subsequent views. https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java (right): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java:103: changeHeight(0); On 2017/07/18 16:41:18, Ted C wrote: > changing the layout height in this case seems wrong to me. This seems like > where we should be setting it to gone. I'll defer to mdjones@ though as I'm not > familiar with these changes: > > Source: > https://codereview.chromium.org/2709883002 For this view, alpha is tied to visibility (see setAlpha above). This check is to block anyone trying to set the view to visible when the alpha is 0, i.e. the view should already be gone at this point.
Roll back to overriding CoordinatorLayout. tedchoc@, mdjones@, could you please review? https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2878403002/diff/600001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:26: android:layout_height="wrap_content" /> On 2017/07/19 18:40:50, mdjones wrote: > On 2017/07/19 01:18:53, jaebaek wrote: > > On 2017/07/18 16:41:18, Ted C wrote: > > > I'm pretty sure this needs to be match_parent. This is what shows the > > > accessibility tab switcher and should take up the whole screen. That being > > > said, I'm not sure why it isn't marked as gone. > > > > If we must keep it as match_parent, there is no chance to > > set mouse cursor based on this resizing. It is mainly because > > onResolvePointerIcon() of ViewGroup chooses the later > > child first regardless of whether its visibility is gone or not. > > > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > > I think we should roll it back to the way of creating a class > > derived from android.support.design.widget.CoordinatorLayout > > and overriding onResolvePointerIcon(). > > > > tedchoc@, mdjones@, aelias@, what is your opinion? > > This will need to remain match parent. That being said, I agree with ted that > this view should be 'gone' by default. Unfortunately, at quick glance it looks > like visibility of the view isn't considered when resolving the pointer which > makes this more difficult. > > I think making a coordinator that overrides onResolvePointerIcon is probably one > of the least fragile ways to handle this. It can ignore views that are set to > 'gone' (assuming I didn't miss something) and wouldn't require special logic for > each new child in this view. Simply checking whether a view's visibility is 'gone' is not enough. There are some views that are partially visible although their visibility is 'visible' (e.g., BottomContainer). snackbar can be shown while other parts of BottomContainer must be transparent. For those transparent parts, we must choose CompositorViewHolder's mouse cursor instead of the one of BottomContainer. A correct way to choose mouse cursor is to find 'not transparent' child on a given location and use its cursor. However, I think this is redundant with the implementation of the existing onResolvePointerIcon() method. A better way to reuse the existing onResolvePointerIcon() is to choose the cursor of a child whose onResolvePointerIcon() returns non null value. One issue is the case a view becomes visible but does not set a pointer icon, so the point icon of CompositorViewHolder is chosen. To prevent this case, I will set pointer icon for all views when they becomes actually visible. Please review the detail in my next code submission.
Looks good, just a few comments: Regarding the partly transparent views: If a view group is taking up the whole screen but a child view only takes up part, null should be returned for the empty area: https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an... The child view is skipped if the event is not in its bounds. As long as the group is returning null (and it should be by default), this seems like it should work. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:18: * must be applied according to a mouse motion event. nit: Mention this is needed because the default android impl does not consider view visibility. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java (left): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:246: mButtonRowLayout.addView(primaryButton); Is this view added somewhere else? If not, removing this will break infobars.
overall this seems an ok, but sad (because of the framework issue) approach to the problem https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:190: ContentViewCore content = getActiveContent(); do we also need to call isTabInteractive() here? if you're in the tab switcher, would this show the pointer icons as if you were still showing the tab? https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:15: * This class extends a {@link CoordinatorLayout} which is the parent of wrap comments at 100 chars in java land https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:21: public CoordinatorLayoutForPointer(Context context) { we should only need the second constructor for inflating from xml, so let's remove this one https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:30: public PointerIcon onResolvePointerIcon(MotionEvent event, int pointerIndex) { I believe this got fixed in N-MR1 (b/34114031). Should we only do this on N and otherwise call super.onResolvePointerIcon? https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:529: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { why is this needed? this is while the top controls are scrolling off right? The view is made to be gone in that case, so do we still need this? https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2302: mOmniboxResultsContainer.setPointerIcon(icon); what is the default icon? why do we need to set anything? I guess this question applies all places. If we don't set anything, is the default icon null? In which case, does it walk up the children to find a non-null icon? That seems like quite the crazy api to me. Does that mean every child in our code needs to set an arrow to prevent blending up the hierarchy?
PTAL https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:190: ContentViewCore content = getActiveContent(); On 2017/07/21 00:15:25, Ted C wrote: > do we also need to call isTabInteractive() here? if you're in the tab switcher, > would this show the pointer icons as if you were still showing the tab? I made sure that the default pointer icon (pointer/arrow) will be used when the isTabInteractive returns false. So the default one will be chose when other view covers CompositorViewHolder as in the tab switcher. That makes me think calling isTabInteractive() is not necessary here. It would be still okay to call it though. Feel free to let me know if you believe it would be safer to do that. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:15: * This class extends a {@link CoordinatorLayout} which is the parent of On 2017/07/21 00:15:25, Ted C wrote: > wrap comments at 100 chars in java land Done. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:18: * must be applied according to a mouse motion event. On 2017/07/20 21:49:06, mdjones wrote: > nit: Mention this is needed because the default android impl does not consider > view visibility. Done. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:21: public CoordinatorLayoutForPointer(Context context) { On 2017/07/21 00:15:25, Ted C wrote: > we should only need the second constructor for inflating from xml, so let's > remove this one Done. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:30: public PointerIcon onResolvePointerIcon(MotionEvent event, int pointerIndex) { On 2017/07/21 00:15:25, Ted C wrote: > I believe this got fixed in N-MR1 (b/34114031). > > Should we only do this on N and otherwise call super.onResolvePointerIcon? I want to check the updated code, but I cannot access it. Based on the description, I am not sure they handle the case the view is partially visible. Why don't we keep this? I guess calling onResolvePointerIcon for each child does not cause that big performance overhead. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:529: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2017/07/21 00:15:25, Ted C wrote: > why is this needed? this is while the top controls are scrolling off right? > The view is made to be gone in that case, so do we still need this? The view is still present at its location (i.e., the visibility is not gone) though it is not visible. So the wrong icons (such as text I-beam) will be shown without this code. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java (left): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:246: mButtonRowLayout.addView(primaryButton); On 2017/07/20 21:49:06, mdjones wrote: > Is this view added somewhere else? If not, removing this will break infobars. Ah .. it's my mistake. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2302: mOmniboxResultsContainer.setPointerIcon(icon); On 2017/07/21 00:15:25, Ted C wrote: > what is the default icon? why do we need to set anything? > > I guess this question applies all places. If we don't set anything, is the > default icon null? In which case, does it walk up the children to find a > non-null icon? > > That seems like quite the crazy api to me. Does that mean every child in our > code needs to set an arrow to prevent blending up the hierarchy? In my implementation, onResolvePointerIcon method of CoordinatorLayoutForPointer skips children that return null in onResolvePointerIcon. The reason behind this is because of partially transparent views. When SnackBar is shown, BottomContainer is not invisible but transparent except the part for SnackBar. In the case, the pointer icon for the transparent part must be the same with the one of CompositorViewHolder, while the one of SnackBar is not. If onResolvePointerIcon of CoordinatorLayoutForPointer only checks visibility (without null check), it chooses pointer icon of BottomContainer but not the one for CompositorViewHolder. Thus, point icon is always default (i.e., null). To solve this, I add null check in onResolvePointerIcon of CoordinatorLayoutForPointer. Because of the null check, All its children must return non-null in onResolvePointerIcon, when they are not transparent.
https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:36: PointerIcon icon = getChildAt(i).onResolvePointerIcon(event, pointerIndex); this doesn't actually check that the mouse cursor is within the bounds of the view. Doesn't that mean that the first visible child will always use their pointer? In general, I think this logic should mimic as much if not all of the existing platform code, it should just add the missing visibility check. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:529: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2017/07/21 10:03:04, jaebaek wrote: > On 2017/07/21 00:15:25, Ted C wrote: > > why is this needed? this is while the top controls are scrolling off right? > > The view is made to be gone in that case, so do we still need this? > > The view is still present at its location (i.e., the visibility is not gone) > though it is not visible. So the wrong icons (such as text I-beam) will be shown > without this code. Since the controlcontainer is a child of the coordinatorlayout, why doesn't your visibility check handle this? It is INVISIBLE which should have it skipped. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2302: mOmniboxResultsContainer.setPointerIcon(icon); On 2017/07/21 10:03:05, jaebaek wrote: > On 2017/07/21 00:15:25, Ted C wrote: > > what is the default icon? why do we need to set anything? > > > > I guess this question applies all places. If we don't set anything, is the > > default icon null? In which case, does it walk up the children to find a > > non-null icon? > > > > That seems like quite the crazy api to me. Does that mean every child in our > > code needs to set an arrow to prevent blending up the hierarchy? > > In my implementation, onResolvePointerIcon method of CoordinatorLayoutForPointer > skips children that return null in onResolvePointerIcon. The reason behind this > is because of partially transparent views. > > When SnackBar is shown, BottomContainer is not invisible but transparent except > the part for SnackBar. In the case, the pointer icon for the transparent part > must be the same with the one of CompositorViewHolder, while the one of SnackBar > is not. If onResolvePointerIcon of CoordinatorLayoutForPointer only checks > visibility (without null check), it chooses pointer icon of BottomContainer but > not the one for CompositorViewHolder. Thus, point icon is always default (i.e., > null). > > To solve this, I add null check in onResolvePointerIcon of > CoordinatorLayoutForPointer. Because of the null check, All its children must > return non-null in onResolvePointerIcon, when they are not transparent. Is BottomContainer the only issue? Can that be made to wrap_contents? Actually, I'm still confused why partially transparent views give us grief. Looking at the android code link you sent: https://github.com/android/platform_frameworks_base/blob/master/core/java/and... For each child, it checks whether the point is within the view (isTransformedTouchPointInView). When a snackbar is showing, the bottomcontainer has the one view. If you're hovering over the blank area. The code above looks like it should just return null as it should fall into "return super.onResolvePointerIcon" since no view should have returned true for Touch point calculation: https://github.com/android/platform_frameworks_base/blob/master/core/java/and... https://github.com/android/platform_frameworks_base/blob/master/core/java/and... What about the default call in BottomContainer#onResolvePointerIcon doesn't work?
https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:36: PointerIcon icon = getChildAt(i).onResolvePointerIcon(event, pointerIndex); On 2017/07/21 18:45:31, Ted C wrote: > this doesn't actually check that the mouse cursor is within the bounds > of the view. Doesn't that mean that the first visible child will always > use their pointer? > > In general, I think this logic should mimic as much if not all of the > existing platform code, it should just add the missing visibility check. > With the assumption that this layout is always match_parent, this actually ends up working (the bounds checks are only needed in the children). Though, I do agree we should probably just take the ViewGroup implementation and expand on it.
Two things are updated: 1. Adding bound check in onResolvePointerIcon() of CoordinatorLayoutForPointer. 2. Changing layout_height of bottom container as wrap_content. This makes snackbar hidden when snackbar must be shown. To solve this problem, I implemented onMeasure() in BottomContainer class and removed bottom margin for keyboard's height from SnackbarView. PTAL https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:36: PointerIcon icon = getChildAt(i).onResolvePointerIcon(event, pointerIndex); On 2017/07/21 20:42:28, mdjones wrote: > On 2017/07/21 18:45:31, Ted C wrote: > > this doesn't actually check that the mouse cursor is within the bounds > > of the view. Doesn't that mean that the first visible child will always > > use their pointer? > > > > In general, I think this logic should mimic as much if not all of the > > existing platform code, it should just add the missing visibility check. > > > > With the assumption that this layout is always match_parent, this actually ends > up working (the bounds checks are only needed in the children). Though, I do > agree we should probably just take the ViewGroup implementation and expand on > it. Ah .. it's my mistake. I added bound check here. I also dropped this null check. This method only checks the visibility and I adjusted heights of bottom containers (i.e., @+id/bottom_sheet_snackbar_container and @+id/bottom_container in main.xml) as wrap_content. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:529: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { On 2017/07/21 18:45:31, Ted C wrote: > On 2017/07/21 10:03:04, jaebaek wrote: > > On 2017/07/21 00:15:25, Ted C wrote: > > > why is this needed? this is while the top controls are scrolling off right? > > > > The view is made to be gone in that case, so do we still need this? > > > > The view is still present at its location (i.e., the visibility is not gone) > > though it is not visible. So the wrong icons (such as text I-beam) will be > shown > > without this code. > > Since the controlcontainer is a child of the coordinatorlayout, why doesn't your > visibility check handle this? > > It is INVISIBLE which should have it skipped. You are right. I dropped this code. https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2302: mOmniboxResultsContainer.setPointerIcon(icon); On 2017/07/21 18:45:31, Ted C wrote: > On 2017/07/21 10:03:05, jaebaek wrote: > > On 2017/07/21 00:15:25, Ted C wrote: > > > what is the default icon? why do we need to set anything? > > > > > > I guess this question applies all places. If we don't set anything, is the > > > default icon null? In which case, does it walk up the children to find a > > > non-null icon? > > > > > > That seems like quite the crazy api to me. Does that mean every child in > our > > > code needs to set an arrow to prevent blending up the hierarchy? > > > > In my implementation, onResolvePointerIcon method of > CoordinatorLayoutForPointer > > skips children that return null in onResolvePointerIcon. The reason behind > this > > is because of partially transparent views. > > > > When SnackBar is shown, BottomContainer is not invisible but transparent > except > > the part for SnackBar. In the case, the pointer icon for the transparent part > > must be the same with the one of CompositorViewHolder, while the one of > SnackBar > > is not. If onResolvePointerIcon of CoordinatorLayoutForPointer only checks > > visibility (without null check), it chooses pointer icon of BottomContainer > but > > not the one for CompositorViewHolder. Thus, point icon is always default > (i.e., > > null). > > > > To solve this, I add null check in onResolvePointerIcon of > > CoordinatorLayoutForPointer. Because of the null check, All its children must > > return non-null in onResolvePointerIcon, when they are not transparent. > > Is BottomContainer the only issue? Can that be made to wrap_contents? > > Actually, I'm still confused why partially transparent views give us grief. > > Looking at the android code link you sent: > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > For each child, it checks whether the point is within the view > (isTransformedTouchPointInView). When a snackbar is showing, > the bottomcontainer has the one view. If you're hovering over > the blank area. The code above looks like it should just return > null as it should fall into "return super.onResolvePointerIcon" > since no view should have returned true for > > Touch point calculation: > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > What about the default call in BottomContainer#onResolvePointerIcon > doesn't work? I changed the layout_height of BottomContainer to wrap_content.
On 2017/07/24 07:34:11, jaebaek wrote: > Two things are updated: > > 1. Adding bound check in onResolvePointerIcon() of CoordinatorLayoutForPointer. > 2. Changing layout_height of bottom container as wrap_content. This makes > snackbar hidden when snackbar must be shown. To solve this problem, I > implemented onMeasure() in BottomContainer class and removed bottom margin for > keyboard's height from SnackbarView. > > PTAL > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java > (right): > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:36: > PointerIcon icon = getChildAt(i).onResolvePointerIcon(event, pointerIndex); > On 2017/07/21 20:42:28, mdjones wrote: > > On 2017/07/21 18:45:31, Ted C wrote: > > > this doesn't actually check that the mouse cursor is within the bounds > > > of the view. Doesn't that mean that the first visible child will always > > > use their pointer? > > > > > > In general, I think this logic should mimic as much if not all of the > > > existing platform code, it should just add the missing visibility check. > > > > > > > With the assumption that this layout is always match_parent, this actually > ends > > up working (the bounds checks are only needed in the children). Though, I do > > agree we should probably just take the ViewGroup implementation and expand on > > it. > > Ah .. it's my mistake. I added bound check here. I also dropped this null check. > This method only checks the visibility and I adjusted heights of bottom > containers (i.e., @+id/bottom_sheet_snackbar_container and @+id/bottom_container > in main.xml) as wrap_content. > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java > (right): > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:529: > if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { > On 2017/07/21 18:45:31, Ted C wrote: > > On 2017/07/21 10:03:04, jaebaek wrote: > > > On 2017/07/21 00:15:25, Ted C wrote: > > > > why is this needed? this is while the top controls are scrolling off > right? > > > > > > The view is made to be gone in that case, so do we still need this? > > > > > > The view is still present at its location (i.e., the visibility is not gone) > > > though it is not visible. So the wrong icons (such as text I-beam) will be > > shown > > > without this code. > > > > Since the controlcontainer is a child of the coordinatorlayout, why doesn't > your > > visibility check handle this? > > > > It is INVISIBLE which should have it skipped. > > You are right. I dropped this code. > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java > (right): > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2302: > mOmniboxResultsContainer.setPointerIcon(icon); > On 2017/07/21 18:45:31, Ted C wrote: > > On 2017/07/21 10:03:05, jaebaek wrote: > > > On 2017/07/21 00:15:25, Ted C wrote: > > > > what is the default icon? why do we need to set anything? > > > > > > > > I guess this question applies all places. If we don't set anything, is > the > > > > default icon null? In which case, does it walk up the children to find a > > > > non-null icon? > > > > > > > > That seems like quite the crazy api to me. Does that mean every child in > > our > > > > code needs to set an arrow to prevent blending up the hierarchy? > > > > > > In my implementation, onResolvePointerIcon method of > > CoordinatorLayoutForPointer > > > skips children that return null in onResolvePointerIcon. The reason behind > > this > > > is because of partially transparent views. > > > > > > When SnackBar is shown, BottomContainer is not invisible but transparent > > except > > > the part for SnackBar. In the case, the pointer icon for the transparent > part > > > must be the same with the one of CompositorViewHolder, while the one of > > SnackBar > > > is not. If onResolvePointerIcon of CoordinatorLayoutForPointer only checks > > > visibility (without null check), it chooses pointer icon of BottomContainer > > but > > > not the one for CompositorViewHolder. Thus, point icon is always default > > (i.e., > > > null). > > > > > > To solve this, I add null check in onResolvePointerIcon of > > > CoordinatorLayoutForPointer. Because of the null check, All its children > must > > > return non-null in onResolvePointerIcon, when they are not transparent. > > > > Is BottomContainer the only issue? Can that be made to wrap_contents? > > > > Actually, I'm still confused why partially transparent views give us grief. > > > > Looking at the android code link you sent: > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > > For each child, it checks whether the point is within the view > > (isTransformedTouchPointInView). When a snackbar is showing, > > the bottomcontainer has the one view. If you're hovering over > > the blank area. The code above looks like it should just return > > null as it should fall into "return super.onResolvePointerIcon" > > since no view should have returned true for > > > > Touch point calculation: > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > > What about the default call in BottomContainer#onResolvePointerIcon > > doesn't work? > > I changed the layout_height of BottomContainer to wrap_content. I looked into the changes to BottomContainer and I believe it can be simplified much more. I've uploaded a separate CL here: https://chromium-review.googlesource.com/c/585579/ I believe that should remove the need to override onMeasure. If you take a look at patch set one, it shows a snackbar repeatedly and it works with the keyboard showing and not.
On 2017/07/24 07:34:11, jaebaek wrote: > Two things are updated: > > 1. Adding bound check in onResolvePointerIcon() of CoordinatorLayoutForPointer. > 2. Changing layout_height of bottom container as wrap_content. This makes > snackbar hidden when snackbar must be shown. To solve this problem, I > implemented onMeasure() in BottomContainer class and removed bottom margin for > keyboard's height from SnackbarView. > > PTAL > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java > (right): > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/coordinator/CoordinatorLayoutForPointer.java:36: > PointerIcon icon = getChildAt(i).onResolvePointerIcon(event, pointerIndex); > On 2017/07/21 20:42:28, mdjones wrote: > > On 2017/07/21 18:45:31, Ted C wrote: > > > this doesn't actually check that the mouse cursor is within the bounds > > > of the view. Doesn't that mean that the first visible child will always > > > use their pointer? > > > > > > In general, I think this logic should mimic as much if not all of the > > > existing platform code, it should just add the missing visibility check. > > > > > > > With the assumption that this layout is always match_parent, this actually > ends > > up working (the bounds checks are only needed in the children). Though, I do > > agree we should probably just take the ViewGroup implementation and expand on > > it. > > Ah .. it's my mistake. I added bound check here. I also dropped this null check. > This method only checks the visibility and I adjusted heights of bottom > containers (i.e., @+id/bottom_sheet_snackbar_container and @+id/bottom_container > in main.xml) as wrap_content. > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java > (right): > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:529: > if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { > On 2017/07/21 18:45:31, Ted C wrote: > > On 2017/07/21 10:03:04, jaebaek wrote: > > > On 2017/07/21 00:15:25, Ted C wrote: > > > > why is this needed? this is while the top controls are scrolling off > right? > > > > > > The view is made to be gone in that case, so do we still need this? > > > > > > The view is still present at its location (i.e., the visibility is not gone) > > > though it is not visible. So the wrong icons (such as text I-beam) will be > > shown > > > without this code. > > > > Since the controlcontainer is a child of the coordinatorlayout, why doesn't > your > > visibility check handle this? > > > > It is INVISIBLE which should have it skipped. > > You are right. I dropped this code. > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java > (right): > > https://codereview.chromium.org/2878403002/diff/640001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2302: > mOmniboxResultsContainer.setPointerIcon(icon); > On 2017/07/21 18:45:31, Ted C wrote: > > On 2017/07/21 10:03:05, jaebaek wrote: > > > On 2017/07/21 00:15:25, Ted C wrote: > > > > what is the default icon? why do we need to set anything? > > > > > > > > I guess this question applies all places. If we don't set anything, is > the > > > > default icon null? In which case, does it walk up the children to find a > > > > non-null icon? > > > > > > > > That seems like quite the crazy api to me. Does that mean every child in > > our > > > > code needs to set an arrow to prevent blending up the hierarchy? > > > > > > In my implementation, onResolvePointerIcon method of > > CoordinatorLayoutForPointer > > > skips children that return null in onResolvePointerIcon. The reason behind > > this > > > is because of partially transparent views. > > > > > > When SnackBar is shown, BottomContainer is not invisible but transparent > > except > > > the part for SnackBar. In the case, the pointer icon for the transparent > part > > > must be the same with the one of CompositorViewHolder, while the one of > > SnackBar > > > is not. If onResolvePointerIcon of CoordinatorLayoutForPointer only checks > > > visibility (without null check), it chooses pointer icon of BottomContainer > > but > > > not the one for CompositorViewHolder. Thus, point icon is always default > > (i.e., > > > null). > > > > > > To solve this, I add null check in onResolvePointerIcon of > > > CoordinatorLayoutForPointer. Because of the null check, All its children > must > > > return non-null in onResolvePointerIcon, when they are not transparent. > > > > Is BottomContainer the only issue? Can that be made to wrap_contents? > > > > Actually, I'm still confused why partially transparent views give us grief. > > > > Looking at the android code link you sent: > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > > For each child, it checks whether the point is within the view > > (isTransformedTouchPointInView). When a snackbar is showing, > > the bottomcontainer has the one view. If you're hovering over > > the blank area. The code above looks like it should just return > > null as it should fall into "return super.onResolvePointerIcon" > > since no view should have returned true for > > > > Touch point calculation: > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > https://github.com/android/platform_frameworks_base/blob/master/core/java/and... > > > > What about the default call in BottomContainer#onResolvePointerIcon > > doesn't work? > > I changed the layout_height of BottomContainer to wrap_content. From my above comment, I was thinking that ViewGroup itself would handle these semi-transparent views, but it breaks down quickly. ViewGroup's handling of onResolvePointerIcon gets the pointer icon of the first view that contains the coords of the pointer event. Since isTransformedTouchPointInView only checks the layout bounds of the child, it doesn't understand the internals of the child layout. Since BottomContainer is match parent, the coordinatorlayout always would think the point is within that view. There is no way for a ViewGroup to notify it's parent that it shouldn't consume the pointer icon call and to continue up the view hierarchy. It just checks the first child that contains the point and no others. This break in particular is what keeps it from walking up the chain: https://github.com/android/platform_frameworks_base/blob/master/core/java/and... If we wanted to support this, we'd need the default implementation of onResolvePointerIcon to return non-null https://github.com/android/platform_frameworks_base/blob/master/core/java/and... Then we'd need to return null to indicate that this view doesn't consume the pointer icon and the next view should take it. Since we can't make that change, I think changing to wrap_contents is our only option.
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Adopted the patch https://chromium-review.googlesource.com/c/585579 that tedchoc@ suggested. PTAL
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Rebased (on the patch https://chromium-review.googlesource.com/c/585579) PTAL
lgtm w/ the one xml change and one continuing question https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/re... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:26: android:layout_height="wrap_content" /> With your fixes for pointers in the outer container, I think this should be match parent now. If we need to mark it gone, that seems more correct as well. https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java (right): https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:189: ContentViewCore content = getActiveContent(); I still don't see how this would work if you're in the tab switcher. What makes this return null? Is it because the container view is not attached and therefore something in the android framework returns null? Or are we sending a blur to the webcontents when the tab is detached and thus ViewAndroidDelegate gets updated?
https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/re... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:26: android:layout_height="wrap_content" /> On 2017/08/09 04:30:54, Ted C wrote: > With your fixes for pointers in the outer container, I think this should be > match parent now. If we need to mark it gone, that seems more correct as well. Marking it gone causes several test failures such as OverviewListLayoutTest https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu.... When I test it on a real mobile with ENABLE_ACCESSIBILITY_TAB_SWITCHER, the tab switch does not show the overview list of tabs. I think this must be visible. Do you think setting the height as wrap_content will cause some side effects? If it will, please let me know potential side effects. https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java (right): https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:189: ContentViewCore content = getActiveContent(); On 2017/08/09 04:30:54, Ted C wrote: > I still don't see how this would work if you're in the tab switcher. What makes > this return null? Is it because the container view is not attached and > therefore something in the android framework returns null? > > Or are we sending a blur to the webcontents when the tab is detached and thus > ViewAndroidDelegate gets updated? In the tab switcher, the ContentViewCore is null, so the null checking must be here. I am not sure about other cases.
https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/re... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:26: android:layout_height="wrap_content" /> On 2017/08/09 06:38:17, jaebaek wrote: > On 2017/08/09 04:30:54, Ted C wrote: > > With your fixes for pointers in the outer container, I think this should be > > match parent now. If we need to mark it gone, that seems more correct as > well. > > Marking it gone causes several test failures such as OverviewListLayoutTest > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu.... > > When I test it on a real mobile with ENABLE_ACCESSIBILITY_TAB_SWITCHER, the tab > switch does not show the overview list of tabs. I think this must be visible. Do > you think setting the height as wrap_content will cause some side effects? If it > will, please let me know potential side effects. I think we should fix the code to make this work then. At a quick glance, I think we could look at changing OverviewListLayout#attachViews and detachViews to set the visibility of the parent. https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java (right): https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:189: ContentViewCore content = getActiveContent(); On 2017/08/09 06:38:17, jaebaek wrote: > On 2017/08/09 04:30:54, Ted C wrote: > > I still don't see how this would work if you're in the tab switcher. What > makes > > this return null? Is it because the container view is not attached and > > therefore something in the android framework returns null? > > > > Or are we sending a blur to the webcontents when the tab is detached and thus > > ViewAndroidDelegate gets updated? > > In the tab switcher, the ContentViewCore is null, so the null checking must be > here. I am not sure about other cases. Sorry to keep belaboring the point, but that is definitely not true. When entering the tab switcher on phones (at least the high end ones), the tab remains selected in the model and thus has a valid CVC. I just added logging in CompositorViewHolder#didSwapFrame and the CVC is never null in the tab switcher unless I close all the tabs. So, I still very much believe this is not working as you expect.
The CQ bit was checked by jaebaek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tedchoc@, updated it as discussed in offline. PTAL! https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/re... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/re... chrome/android/java/res/layout/main.xml:26: android:layout_height="wrap_content" /> On 2017/08/09 16:30:55, Ted C wrote: > On 2017/08/09 06:38:17, jaebaek wrote: > > On 2017/08/09 04:30:54, Ted C wrote: > > > With your fixes for pointers in the outer container, I think this should be > > > match parent now. If we need to mark it gone, that seems more correct as > > well. > > > > Marking it gone causes several test failures such as OverviewListLayoutTest > > > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu.... > > > > When I test it on a real mobile with ENABLE_ACCESSIBILITY_TAB_SWITCHER, the > tab > > switch does not show the overview list of tabs. I think this must be visible. > Do > > you think setting the height as wrap_content will cause some side effects? If > it > > will, please let me know potential side effects. > > I think we should fix the code to make this work then. > > At a quick glance, I think we could look at changing > OverviewListLayout#attachViews and detachViews to set the visibility of the > parent. Done. https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java (right): https://codereview.chromium.org/2878403002/diff/880001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:189: ContentViewCore content = getActiveContent(); On 2017/08/09 16:30:55, Ted C wrote: > On 2017/08/09 06:38:17, jaebaek wrote: > > On 2017/08/09 04:30:54, Ted C wrote: > > > I still don't see how this would work if you're in the tab switcher. What > > makes > > > this return null? Is it because the container view is not attached and > > > therefore something in the android framework returns null? > > > > > > Or are we sending a blur to the webcontents when the tab is detached and > thus > > > ViewAndroidDelegate gets updated? > > > > In the tab switcher, the ContentViewCore is null, so the null checking must be > > here. I am not sure about other cases. > > Sorry to keep belaboring the point, but that is definitely not true. When > entering the tab switcher on phones (at least the high end ones), the tab > remains selected in the model and thus has a valid CVC. > > I just added logging in CompositorViewHolder#didSwapFrame and the CVC is never > null in the tab switcher unless I close all the tabs. > > So, I still very much believe this is not working as you expect. As discussed this in offline, I replaced this with the activeView.
lgtm
The CQ bit was checked by jaebaek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2878403002/#ps920001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jaebaek@chromium.org
The CQ bit was checked by jaebaek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, boliu@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2878403002/#ps940001 (title: "Support setting mouse cursor icon in Android N")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 940001, "attempt_start_ts": 1502611817316820,
"parent_rev": "12a54998f9e37b024fd47c60aef781ebde24bf7e", "commit_rev":
"d1b0d570f9bc9bf80d187170fbeac0d4a1f6eb50"}
Message was sent while issue was closed.
Description was changed from ========== Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon (https://developer.android.com/reference/android/view/View.html#setPointerIcon...). Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). BUG=584424 ========== to ========== Support setting mouse cursor icon in Android N. Android N supports mouse input and setting mouse cursor icon (https://developer.android.com/reference/android/view/View.html#setPointerIcon...). Current Chrome does not change the mouse cursor icon even when CSS file specifies the cursor type. This CL supports the mouse cursor setup (e.g., IBeam, Hand, custom cursor from image). BUG=584424 Review-Url: https://codereview.chromium.org/2878403002 Cr-Commit-Position: refs/heads/master@{#493995} Committed: https://chromium.googlesource.com/chromium/src/+/d1b0d570f9bc9bf80d187170fbea... ==========
Message was sent while issue was closed.
Committed patchset #48 (id:940001) as https://chromium.googlesource.com/chromium/src/+/d1b0d570f9bc9bf80d187170fbea... |
