Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708613002:
Add EventForwarder for routing touch events (Closed)
|
Created:
3 years, 10 months ago by Jinsuk Kim Modified:
3 years, 9 months ago CC:
agrieve+watch_chromium.org, android-webview-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, jam Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd EventForwarder for routing touch events
EventForwarder replaces ContentViewCore for forwarding Java input/view
events to native content. The instance that embedders uses can be obtained
via |WebContents.getEventForwarder()|.
BUG=671401
Review-Url: https://codereview.chromium.org/2708613002
Cr-Commit-Position: refs/heads/master@{#455913}
Committed: https://chromium.googlesource.com/chromium/src/+/5c3a120fa6ff650943131fd0a288adcaf6ccd895
Patch Set 1 #Patch Set 2 : - #
Total comments: 2
Patch Set 3 : EventHandler #
Total comments: 34
Patch Set 4 : rebased #
Total comments: 20
Patch Set 5 : comments #
Total comments: 14
Patch Set 6 : unittests #
Total comments: 64
Patch Set 7 : removed MotionEventData #
Total comments: 39
Patch Set 8 : base::Bind (doesn't compile yet) #
Total comments: 20
Patch Set 9 : comments #
Total comments: 2
Patch Set 10 : comment #
Total comments: 4
Patch Set 11 : nits #
Total comments: 2
Patch Set 12 : OffsetCachedPointer #Patch Set 13 : rebased #Patch Set 14 : fix build #Patch Set 15 : EventForwarder... #Messages
Total messages: 93 (35 generated)
The CQ bit was checked by jinsukkim@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_...)
The CQ bit was checked by jinsukkim@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.
Description was changed from ========== Add EventForwarder for routing touch events EventForwarder replaces ContentView for forwarding Java input/view events to native content. BUG=671401 ========== to ========== Add EventForwarder for routing touch events EventForwarder replaces ContentViewCore for forwarding Java input/view events to native content. The instance that embedders uses can be obtained via |WebContents.getEventForwarder()|. BUG=671401 ==========
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org, dtrainor@chromium.org
dtrainor@chromium.org: Please review changes in chrome/ boliu@chromium.org: Please review changes in content/, ui/ Notes: - The name (EventForwarder) might not be the best choice. Any suggestion would be welcome. - I still kept ContentView (or its equivalent view like native page view in Chrome, WebViewChromium in webview, etc) as the class used to dispatch events in Android view hierarchy, and only replaced ContentViewCore with EventForwarder in this CL. I wanted to separate how to refactor ContentView into its own CL, as that needs broader impact on various embedders. - I'm thinking of reverting ViewRoot rather than repurposing it for the new approach taken here. It will be necessary only when Chrome/Content-side refactoring make further progress and we have a better idea.
+1 to deleting ViewRoot, probably before this CL. We should to back to EventHandler that's hangs off of viewandroid, ie earlier patch sets on this CL: https://codereview.chromium.org/2502763003/#ps180001 https://codereview.chromium.org/2708613002/diff/20001/content/browser/android... File content/browser/android/event_forwarder_impl.cc (right): https://codereview.chromium.org/2708613002/diff/20001/content/browser/android... content/browser/android/event_forwarder_impl.cc:87: return is_touch_handle_event ? rwhv->OnTouchHandleEvent(event) this isn't actually using the viewandroid tree to forward events?
On 2017/02/21 16:23:30, boliu wrote: > +1 to deleting ViewRoot, probably before this CL. > > We should to back to EventHandler that's hangs off of viewandroid, ie earlier > patch sets on this CL: > https://codereview.chromium.org/2502763003/#ps180001 > > https://codereview.chromium.org/2708613002/diff/20001/content/browser/android... > File content/browser/android/event_forwarder_impl.cc (right): > > https://codereview.chromium.org/2708613002/diff/20001/content/browser/android... > content/browser/android/event_forwarder_impl.cc:87: return is_touch_handle_event > ? rwhv->OnTouchHandleEvent(event) > this isn't actually using the viewandroid tree to forward events? With this change, the content that will consume the event is already determined - so I thought it wouldn't be necessary to go through ViewAndroid at all. Do you suggest it because it is better in terms of the structure?
On 2017/02/21 19:48:16, Jinsuk Kim wrote: > On 2017/02/21 16:23:30, boliu wrote: > > +1 to deleting ViewRoot, probably before this CL. > > > > We should to back to EventHandler that's hangs off of viewandroid, ie earlier > > patch sets on this CL: > > https://codereview.chromium.org/2502763003/#ps180001 > > > > > https://codereview.chromium.org/2708613002/diff/20001/content/browser/android... > > File content/browser/android/event_forwarder_impl.cc (right): > > > > > https://codereview.chromium.org/2708613002/diff/20001/content/browser/android... > > content/browser/android/event_forwarder_impl.cc:87: return > is_touch_handle_event > > ? rwhv->OnTouchHandleEvent(event) > > this isn't actually using the viewandroid tree to forward events? > > With this change, the content that will consume the event is already determined > - so I thought it wouldn't be necessary to go through ViewAndroid at all. Do you > suggest it because it is better in terms of the structure? Yeah, but we still want to use ViewAndroid tree for the webcontents->rwhva part. So the EventHandler would hang off of the ViewAndroid of the WebContents for now. Then events can be injected from there. Then EventHandler will be moved higher up in the tree as chrome layer is refactored, until eventually it's just the root, at which point we can just rename it back to ViewRoot.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
- Renamed EventForwarder to EventHandler, and moved to ui/android - Migrated |onTouchEvent| for an example, while only half-done |onMouseEvent| along the way. Entire input events will be migrated in coming CLs to make the CL size manageable. This caused some dup'ed code in EventHandler/CVC. https://codereview.chromium.org/2708613002/diff/20001/content/browser/android... File content/browser/android/event_forwarder_impl.cc (right): https://codereview.chromium.org/2708613002/diff/20001/content/browser/android... content/browser/android/event_forwarder_impl.cc:87: return is_touch_handle_event ? rwhv->OnTouchHandleEvent(event) On 2017/02/21 16:23:30, boliu wrote: > this isn't actually using the viewandroid tree to forward events? Done.
only looked at ui https://codereview.chromium.org/2708613002/diff/80001/ui/android/event_handle... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/event_handle... ui/android/event_handler.cc:38: : view_(view), dip_scale_(view->GetDipScale()) {} don't cache this scale, since it can change at run time, just ask the view every time? https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/EventHandler.java:80: if (mNativeEventHandler == 0) return false; this can never happen, right? https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/EventHandler.java:128: mFocusPreOSKViewportRect.setEmpty(); this should stay in CVC. https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:15: public final class SPenSupport { I think we don't need to expose this at all, just detect lazily? https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:49: assert sIsSPenSupported != null; how is client supposed to know if detect succeeded or failed? https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:78: dip_scale_(ui::GetScaleFactorForNativeView(this)) {} again, don't cache this https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:104: DCHECK(!parent_ || parent_ == GetWindowAndroid()) this is gonna break after WCVA is not immediate child of root use the original condition, that a root can have at most one source of events https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:116: DCHECK(!ViewTreeHasEventHandler(child)) << "Child cannot have event handler"; why not..? that only holds if we don't already have a event handler https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:258: if (client_ && client_->OnTouchEvent(event, is_touch_handle_event)) what happened to hit testing? Did that part get reverted too? I mean we still should do hit testing here for things like touch events, even though it won't actually have any effects until more refactors.. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_client.cc File ui/android/view_client.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_client.... ui/android/view_client.cc:13: bool ViewClient::OnMouseEvent(const MotionEventAndroid& event, nit: empty line above https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_client.... ui/android/view_client.h:11: #include "ui/events/android/motion_event_android.h" forward declare is enough
https://codereview.chromium.org/2708613002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2708613002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1776: bool is_touch_handle_event) { We should look at how we could remove this distinction at some point. https://codereview.chromium.org/2708613002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2708613002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:804: base::android::ScopedJavaLocalRef<jobject> CreateJavaEventHandlerFromNative(); Should this live on WebContentsAndroid? https://codereview.chromium.org/2708613002/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2708613002/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:92: * WARNING: ContentViewCore is in the process of being broken up. Please do not add new stuff. Nice :). Should we add you specifically to the OWNERS file for ContentViewCore.java? That way you can make sure nobody adds code here? https://codereview.chromium.org/2708613002/diff/100001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/100001/ui/android/event_handl... ui/android/event_handler.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. Is this file copy and paste from ContentViewCore? Same for Java class? https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:125: public void cancelRequestToScrollFocusedEditableNodeIntoView() { javadoc https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:131: public Rect getFocusPreOSKViewportRect() { javadoc here too https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:23: public static void detect(Context context) { javadoc? https://codereview.chromium.org/2708613002/diff/100001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/100001/ui/android/view_androi... ui/android/view_android.cc:93: // A ViewAndroid may have its own delegate or otherwise will comment looks wrapped too early. https://codereview.chromium.org/2708613002/diff/100001/ui/android/view_androi... ui/android/view_android.cc:99: float ViewAndroid::GetDipScale() { dip_scale() in header? https://codereview.chromium.org/2708613002/diff/100001/ui/android/view_androi... ui/android/view_android.cc:125: bool ViewAndroid::ViewTreeHasEventHandler(ViewAndroid* view) { "// static" above this.
https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:258: if (client_ && client_->OnTouchEvent(event, is_touch_handle_event)) On 2017/02/27 19:21:57, boliu wrote: > what happened to hit testing? > > Did that part get reverted too? I mean we still should do hit testing here for > things like touch events, even though it won't actually have any effects until > more refactors.. actually, never mind, hit testing can be added later, just pretend everything is "match parent" for now
https://codereview.chromium.org/2708613002/diff/80001/ui/android/event_handle... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/event_handle... ui/android/event_handler.cc:38: : view_(view), dip_scale_(view->GetDipScale()) {} On 2017/02/27 19:21:56, boliu wrote: > don't cache this scale, since it can change at run time, just ask the view every > time? Done. https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/EventHandler.java:80: if (mNativeEventHandler == 0) return false; On 2017/02/27 19:21:56, boliu wrote: > this can never happen, right? Removed. https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/EventHandler.java:128: mFocusPreOSKViewportRect.setEmpty(); On 2017/02/27 19:21:56, boliu wrote: > this should stay in CVC. Put it back to CVC and defined an interface (EventNotifier) to handle it in CVC. https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:15: public final class SPenSupport { On 2017/02/27 19:21:57, boliu wrote: > I think we don't need to expose this at all, just detect lazily? This is used by Chrome too. See CompositorViewHolder. The reason I split detect / convert is for the callsites of convert not to have to pass |context| which is used only for detection. https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:49: assert sIsSPenSupported != null; On 2017/02/27 19:21:57, boliu wrote: > how is client supposed to know if detect succeeded or failed? ? |sIsSPenSupported| will either be true or false. FYI SPenSupport.detect() needs to be invoked in advance. That's where sIsPenSupported is initialized. assert is here to guard against use without init. Added a comment to clarify it. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:78: dip_scale_(ui::GetScaleFactorForNativeView(this)) {} On 2017/02/27 19:21:57, boliu wrote: > again, don't cache this Done. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:104: DCHECK(!parent_ || parent_ == GetWindowAndroid()) On 2017/02/27 19:21:57, boliu wrote: > this is gonna break after WCVA is not immediate child of root > > use the original condition, that a root can have at most one source of events Done. Restored |GetViewRoot()| that returns the top of the view tree where the event forwarding gets started for this content. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:116: DCHECK(!ViewTreeHasEventHandler(child)) << "Child cannot have event handler"; On 2017/02/27 19:21:57, boliu wrote: > why not..? that only holds if we don't already have a event handler The restriction was to allow for the event handler only to WCVA's VA. I guess you want it more general... Done. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:258: if (client_ && client_->OnTouchEvent(event, is_touch_handle_event)) On 2017/02/27 19:21:57, boliu wrote: > what happened to hit testing? > > Did that part get reverted too? I mean we still should do hit testing here for > things like touch events, even though it won't actually have any effects until > more refactors.. Done. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:258: if (client_ && client_->OnTouchEvent(event, is_touch_handle_event)) On 2017/02/28 03:18:15, boliu wrote: > On 2017/02/27 19:21:57, boliu wrote: > > what happened to hit testing? > > > > Did that part get reverted too? I mean we still should do hit testing here for > > things like touch events, even though it won't actually have any effects until > > more refactors.. > > actually, never mind, hit testing can be added later, just pretend everything is > "match parent" for now Restored and saw your comment too late :( Set |match_parent_ | to true. Take another look and let me know how much you want to keep or gut out again. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_client.cc File ui/android/view_client.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_client.... ui/android/view_client.cc:13: bool ViewClient::OnMouseEvent(const MotionEventAndroid& event, On 2017/02/27 19:21:57, boliu wrote: > nit: empty line above Done. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_client.... ui/android/view_client.h:11: #include "ui/events/android/motion_event_android.h" On 2017/02/27 19:21:57, boliu wrote: > forward declare is enough Done. https://codereview.chromium.org/2708613002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2708613002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1776: bool is_touch_handle_event) { On 2017/02/28 01:31:23, David Trainor wrote: > We should look at how we could remove this distinction at some point. Left a TODO for a reminder for future refactoring. https://codereview.chromium.org/2708613002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2708613002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:804: base::android::ScopedJavaLocalRef<jobject> CreateJavaEventHandlerFromNative(); On 2017/02/28 01:31:23, David Trainor wrote: > Should this live on WebContentsAndroid? That's better. Done. https://codereview.chromium.org/2708613002/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2708613002/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:92: * WARNING: ContentViewCore is in the process of being broken up. Please do not add new stuff. On 2017/02/28 01:31:23, David Trainor wrote: > Nice :). Should we add you specifically to the OWNERS file for > ContentViewCore.java? That way you can make sure nobody adds code here? That would be great if that's the way to get notified when somebody touches this class. Is it okay to have such granularity for OWNERS? https://codereview.chromium.org/2708613002/diff/100001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/100001/ui/android/event_handl... ui/android/event_handler.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/02/28 01:31:23, David Trainor wrote: > Is this file copy and paste from ContentViewCore? Same for Java class? Yes |OnTouchEvent| and |OnMouseEvent| are being moved here. Same for Java class. https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:125: public void cancelRequestToScrollFocusedEditableNodeIntoView() { On 2017/02/28 01:31:23, David Trainor wrote: > javadoc Removed. https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:131: public Rect getFocusPreOSKViewportRect() { On 2017/02/28 01:31:23, David Trainor wrote: > javadoc here too Removed. https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:23: public static void detect(Context context) { On 2017/02/28 01:31:23, David Trainor wrote: > javadoc? Done. https://codereview.chromium.org/2708613002/diff/100001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/100001/ui/android/view_androi... ui/android/view_android.cc:93: // A ViewAndroid may have its own delegate or otherwise will On 2017/02/28 01:31:23, David Trainor wrote: > comment looks wrapped too early. Done. https://codereview.chromium.org/2708613002/diff/100001/ui/android/view_androi... ui/android/view_android.cc:99: float ViewAndroid::GetDipScale() { On 2017/02/28 01:31:23, David Trainor wrote: > dip_scale() in header? Turned to not caching the scale. https://codereview.chromium.org/2708613002/diff/100001/ui/android/view_androi... ui/android/view_android.cc:125: bool ViewAndroid::ViewTreeHasEventHandler(ViewAndroid* view) { On 2017/02/28 01:31:23, David Trainor wrote: > "// static" above this. Done.
Patchset #5 (id:120001) has been deleted
only looked at ui diff https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/EventHandler.java:128: mFocusPreOSKViewportRect.setEmpty(); On 2017/02/28 06:56:04, Jinsuk Kim wrote: > On 2017/02/27 19:21:56, boliu wrote: > > this should stay in CVC. > > Put it back to CVC and defined an interface (EventNotifier) to handle it in CVC. Is that really necessary? Can CVC just catch all the cases where ACTION_DOWN happens. Or there a corner case I'm missing? https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:15: public final class SPenSupport { On 2017/02/28 06:56:04, Jinsuk Kim wrote: > On 2017/02/27 19:21:57, boliu wrote: > > I think we don't need to expose this at all, just detect lazily? > > This is used by Chrome too. See CompositorViewHolder. The reason I split detect > / convert is for the callsites of convert not to have to pass |context| which is > used only for detection. ok, I guess hiding this needs to come after chrome refactoring is done but I think lazy detect is still a good idea https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:49: assert sIsSPenSupported != null; On 2017/02/28 06:56:04, Jinsuk Kim wrote: > On 2017/02/27 19:21:57, boliu wrote: > > how is client supposed to know if detect succeeded or failed? > > ? |sIsSPenSupported| will either be true or false. > > FYI SPenSupport.detect() needs to be invoked in advance. That's where > sIsPenSupported is initialized. assert is here to guard against use without > init. Added a comment to clarify it. oh oops, misread, I thought it's "assert sIsSPenSupported" check.. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:258: if (client_ && client_->OnTouchEvent(event, is_touch_handle_event)) On 2017/02/28 06:56:04, Jinsuk Kim wrote: > On 2017/02/28 03:18:15, boliu wrote: > > On 2017/02/27 19:21:57, boliu wrote: > > > what happened to hit testing? > > > > > > Did that part get reverted too? I mean we still should do hit testing here > for > > > things like touch events, even though it won't actually have any effects > until > > > more refactors.. > > > > actually, never mind, hit testing can be added later, just pretend everything > is > > "match parent" for now > > Restored and saw your comment too late :( > Set |match_parent_ | to true. Take another look and let me know how much you > want to keep or gut out again. Sure :p but bring back the unit tests too :) https://codereview.chromium.org/2708613002/diff/140001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/event_handl... ui/android/event_handler.cc:58: MotionEventData event = MotionEventData::ForTouch( nit: you could avoid declaring this local variable here and below, maybe that can enable some compiler optimizations.. https://codereview.chromium.org/2708613002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:166: if (mNativeEventHandler == 0) return false; ditto not required https://codereview.chromium.org/2708613002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:181: return true; this is not totally clear to me, why is the native method not returning anything now, and this is just always returning true? https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_androi... ui/android/view_android.cc:218: DCHECK(!match_parent || (x == 0 && y == 0 && width == 0 && height == 0)); can you write this in a way such that when it fails, it prints out an useful error message of what exactly was wrong? like maybe print out the actual values, or split into multiple DCHECKs https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client... ui/android/view_client.h:18: struct MotionEventData { so why do we need a new struct if MotionEventAndroid is 99% there already? can we just add the missing field to the existing thing instead?
dtrainor@chromium.org changed reviewers: + khushalsagar@chromium.org
https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/EventHandler.java:128: mFocusPreOSKViewportRect.setEmpty(); On 2017/02/28 22:44:50, boliu wrote: > On 2017/02/28 06:56:04, Jinsuk Kim wrote: > > On 2017/02/27 19:21:56, boliu wrote: > > > this should stay in CVC. > > > > Put it back to CVC and defined an interface (EventNotifier) to handle it in > CVC. > > Is that really necessary? Can CVC just catch all the cases where ACTION_DOWN > happens. Or there a corner case I'm missing? Touch event is not going through CVC any more.. the only way I could think of doing it in CVC is to call CVC.onTouchEvent() as well from ContentView. See if that's aligned with what you want. https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:15: public final class SPenSupport { On 2017/02/28 22:44:54, boliu wrote: > On 2017/02/28 06:56:04, Jinsuk Kim wrote: > > On 2017/02/27 19:21:57, boliu wrote: > > > I think we don't need to expose this at all, just detect lazily? > > > > This is used by Chrome too. See CompositorViewHolder. The reason I split > detect > > / convert is for the callsites of convert not to have to pass |context| which > is > > used only for detection. > > ok, I guess hiding this needs to come after chrome refactoring is done > > but I think lazy detect is still a good idea I agree. Left a TODO for that. I think it gets easier to do that once EventHandler (or ViewRoot) later has an access to WindowAndroid which can provide |Context|. https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:49: assert sIsSPenSupported != null; On 2017/02/28 22:44:54, boliu wrote: > On 2017/02/28 06:56:04, Jinsuk Kim wrote: > > On 2017/02/27 19:21:57, boliu wrote: > > > how is client supposed to know if detect succeeded or failed? > > > > ? |sIsSPenSupported| will either be true or false. > > > > FYI SPenSupport.detect() needs to be invoked in advance. That's where > > sIsPenSupported is initialized. assert is here to guard against use without > > init. Added a comment to clarify it. > > oh oops, misread, I thought it's "assert sIsSPenSupported" check.. Acknowledged. https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/view_android... ui/android/view_android.cc:258: if (client_ && client_->OnTouchEvent(event, is_touch_handle_event)) On 2017/02/28 22:44:54, boliu wrote: > On 2017/02/28 06:56:04, Jinsuk Kim wrote: > > On 2017/02/28 03:18:15, boliu wrote: > > > On 2017/02/27 19:21:57, boliu wrote: > > > > what happened to hit testing? > > > > > > > > Did that part get reverted too? I mean we still should do hit testing here > > for > > > > things like touch events, even though it won't actually have any effects > > until > > > > more refactors.. > > > > > > actually, never mind, hit testing can be added later, just pretend > everything > > is > > > "match parent" for now > > > > Restored and saw your comment too late :( > > Set |match_parent_ | to true. Take another look and let me know how much you > > want to keep or gut out again. > > Sure :p but bring back the unit tests too :) Done. https://codereview.chromium.org/2708613002/diff/140001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/event_handl... ui/android/event_handler.cc:58: MotionEventData event = MotionEventData::ForTouch( On 2017/02/28 22:44:54, boliu wrote: > nit: you could avoid declaring this local variable here and below, maybe that > can enable some compiler optimizations.. Done. https://codereview.chromium.org/2708613002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:166: if (mNativeEventHandler == 0) return false; On 2017/02/28 22:44:54, boliu wrote: > ditto not required Done. https://codereview.chromium.org/2708613002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:181: return true; On 2017/02/28 22:44:54, boliu wrote: > this is not totally clear to me, why is the native method not returning anything > now, and this is just always returning true? Unlike OnTouchEvent, OnMouseEvent (before refactoring) simply was returning true. I just made it simpler. https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_androi... ui/android/view_android.cc:218: DCHECK(!match_parent || (x == 0 && y == 0 && width == 0 && height == 0)); On 2017/02/28 22:44:54, boliu wrote: > can you write this in a way such that when it fails, it prints out an useful > error message of what exactly was wrong? like maybe print out the actual values, > or split into multiple DCHECKs Added a message about the condition when |match_parent| is true. https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client... ui/android/view_client.h:18: struct MotionEventData { On 2017/02/28 22:44:54, boliu wrote: > so why do we need a new struct if MotionEventAndroid is 99% there already? can > we just add the missing field to the existing thing instead? Because - MEA creates and assigns a unique id for every instance. Not sure if it makes sense in the new structure where we're likely to create many instances for one Java motion event. - |is_touch_handle_event| was not defined in it in the first place. It didn't look right (for consistency's sake) to add in MEA the |action_button| used only for Mouse event and also passed separately. +aelias, owner of this class for his thought: Do you think it is better to modify MotionEventAndroid rather than add this temporary holder for motion event info?
jinsukkim@chromium.org changed reviewers: + aelias@chromium.org
aelias@ Could you give your thought? https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client...
https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client... ui/android/view_client.h:18: struct MotionEventData { I tend to agree with Bo. > - MEA creates and assigns a unique id for every instance. Not sure if it makes sense in the new structure where we're likely to create many instances for one Java motion event. This id doesn't seem to be used outside of unit tests. So it doesn't really matter, we can spam-increment it. > - |is_touch_handle_event| was not defined in it in the first place. It didn't look right (for consistency's sake) to add in MEA the |action_button| used only for Mouse event and also passed separately. I think is_touch_handle_event probably should not be considered a MotionEvent at all but be passed via special touch-handle-specific side channel. As for action_button, it sounds fine to add to MEA as long as there is a null value for non-mouse events.
https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client... ui/android/view_client.h:18: struct MotionEventData { On 2017/03/02 at 04:17:42, aelias wrote: > I think is_touch_handle_event probably should not be considered a MotionEvent at all but be passed via special touch-handle-specific side channel. As for action_button, it sounds fine to add to MEA as long as there is a null value for non-mouse events. In the long run, C++ touch handles could be child ViewAndroids of the content ViewAndroid and the hit testing could happen on the content/ C++ side. Either way, the handles should fully absorb the MotionEvent. It's pretty weird that right now we do hit testing on the handle, then continue to propagate the MotionEvents with a special flag.
https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/EventHandler.java:128: mFocusPreOSKViewportRect.setEmpty(); On 2017/03/02 04:08:34, Jinsuk Kim wrote: > On 2017/02/28 22:44:50, boliu wrote: > > On 2017/02/28 06:56:04, Jinsuk Kim wrote: > > > On 2017/02/27 19:21:56, boliu wrote: > > > > this should stay in CVC. > > > > > > Put it back to CVC and defined an interface (EventNotifier) to handle it in > > CVC. > > > > Is that really necessary? Can CVC just catch all the cases where ACTION_DOWN > > happens. Or there a corner case I'm missing? > > Touch event is not going through CVC any more.. the only way I could think of > doing it in CVC is to call CVC.onTouchEvent() as well from ContentView. See if > that's aligned with what you want. Ok, looked at what this actually does. So CVC is trying to delay scrolling the focused editable into view, until *after* the resize that's triggered by onscreen keyboard coming up. And this focus is cancelled if user touches the screen again before the resize. This all makes sense to me. I don't think we want to move this logic to whatever is calling onTouchEvent, because then it has to be duplicated in a bunch of places. I was thinking maybe a generic "event observer" is fine, but I just realized view_client already does that. Just have WCVA look for touch_down, then tell CVC to cancel this. I know it's going back and forth between jni, but that is the right thing to do, since we are moving event dispatching to native, but the response here happens in java. Does that make sense? https://codereview.chromium.org/2708613002/diff/160001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2708613002/diff/160001/content/browser/androi... content/browser/android/content_view_core_impl.cc:874: jboolean ContentViewCoreImpl::SendMouseEvent(JNIEnv* env, can be deleted as well? https://codereview.chromium.org/2708613002/diff/160001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1423: view_android->SetLayout(0, 0, 0, 0, true); /* match parent */ https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.cc:24: ScopedJavaLocalRef<jobject> EventHandler::CreateJavaObject() { should create only one java side for each native side, and as discussed before, it's ok to hold a strong ref here because java side doesn't reference anything else https://codereview.chromium.org/2708613002/diff/160001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:30: public static void detect(Context context) { you can just use ContextUtils.getApplicationContext? https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:108: ViewAndroid* ViewAndroid::GetViewRoot() { So.. viewroot is the immediate children of windowandroid? I'm not sure we really want to enforce that yet. I imagine chrome refactor will probably want to separate changes to structure of the tree vs when event forwarding is updated. I think we should just not have a viewroot concept right now (root is the windowandroid), and go from there. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:229: << "Should give empty dimension when view layout matches its parent."; this can be better still. like if I trigger this DCHECK, then I have to add *more* logging to figure out what if match_parent was true or false and etc.. just do that here https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:294: matched = bound.Contains(e.GetX(), e.GetY()); can we abstract out hit testing into its own method, rather than duplicate the logic in two places? probably would make it easier to test as well?
Only looked at ui/ for now. I'll take a look at content/ changes as well. Hope this helps. https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:19: EventHandler(); Not used? https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:20: base::android::ScopedJavaLocalRef<jobject> CreateJavaObject(); Should this class cache the java object instead of creating a new one each time? May be this should be called GetJavaObject instead? This way we should be able to clean up the native pointer from the java object in the dtor as well. https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:22: static EventHandler* Create(ViewAndroid* view); nit: Do we need this Create method? We don't do any requisite initializations there. Can the ctor just be public? And if returning a raw ptr, please do mention that the ownership is passed to the caller. https://codereview.chromium.org/2708613002/diff/160001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:30: public static void detect(Context context) { nit: initialize sounds better. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) That feels like a lot of unnecessary complexity just to ensure that we don't create multiple java bridges in the tree. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:109: DCHECK(parent_) << "View is not attached to a tree yet."; nit: This doesn't really need to be a DCHECK. Just return a null root if detached? https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:148: children_.splice(children_.rbegin().base(), children_, it); nit: children_.end() instead of children_.rbegin().base()? https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:223: void ViewAndroid::SetLayout(int x, Do you want to split this into 2 methods: SetBounds and SetMatchParent? https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:290: for (auto it = children_.rbegin(); it != children_.rend(); ++it) { nit: for (auto* child : children_)? https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:301: if (client_ && client_->OnTouchEvent(event)) nit: return client_ ? client_->OnTouchEvent(event) : false; https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.h:89: EventHandler* CreateEventHandler(); Why create a new instance each time? Shouldn't this be GetEventHandler that lazily creates the object?
https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:22: static EventHandler* Create(ViewAndroid* view); On 2017/03/02 21:46:15, Khushal wrote: > nit: Do we need this Create method? We don't do any requisite initializations > there. Can the ctor just be public? > > And if returning a raw ptr, please do mention that the ownership is passed to > the caller. should return unique_ptr for passing ownership https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 21:46:15, Khushal wrote: > That feels like a lot of unnecessary complexity just to ensure that we don't > create multiple java bridges in the tree. I asked for these checks. Imo it's a correctness guarantee that nothing can receive events from two different sources. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:290: for (auto it = children_.rbegin(); it != children_.rend(); ++it) { On 2017/03/02 21:46:15, Khushal wrote: > nit: for (auto* child : children_)? back to forward order is desired here. we modeled this on cc::Layer, which is painted in forward order, which means for hit testing should be in the reverse order probably worth a comment though
https://codereview.chromium.org/2708613002/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_android.h (right): https://codereview.chromium.org/2708613002/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_android.h:203: base::android::ScopedJavaLocalRef<jobject> CreateEventHandler( nit: GetOrCreateEventHandler since it will create the single EventHandler for this contents only the first time this is called. https://codereview.chromium.org/2708613002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2708613002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:949: private boolean sendMouseEvent(MotionEvent event) { May be this can go away now too? https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:22: static EventHandler* Create(ViewAndroid* view); On 2017/03/02 22:02:16, boliu wrote: > On 2017/03/02 21:46:15, Khushal wrote: > > nit: Do we need this Create method? We don't do any requisite initializations > > there. Can the ctor just be public? > > > > And if returning a raw ptr, please do mention that the ownership is passed to > > the caller. > > should return unique_ptr for passing ownership Agreed. That was just a general comment if for some reason you do end up returning a raw ptr and expect the caller to take ownership. In this case we should have the ctor be public. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 22:02:16, boliu wrote: > On 2017/03/02 21:46:15, Khushal wrote: > > That feels like a lot of unnecessary complexity just to ensure that we don't > > create multiple java bridges in the tree. > > I asked for these checks. Imo it's a correctness guarantee that nothing can > receive events from two different sources. What 2 sources are we worried about? The event methods are public on VA, so what exactly are we trying to restrict? https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:290: for (auto it = children_.rbegin(); it != children_.rend(); ++it) { On 2017/03/02 22:02:16, boliu wrote: > On 2017/03/02 21:46:15, Khushal wrote: > > nit: for (auto* child : children_)? > > back to forward order is desired here. we modeled this on cc::Layer, which is > painted in forward order, which means for hit testing should be in the reverse > order > > probably worth a comment though Oh Duh, didn't notice that. Yeah, a comment for that would be great. Would the following read cleaner: for (auto* child : base::Reversed(children_))? https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_client... ui/android/view_client.h:143: virtual bool OnTouchEvent(const MotionEventData& m); Since we'll be using MotionEvent here, may be you can just pass a bool for is_handle_event on this method.
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 22:08:59, Khushal wrote: > On 2017/03/02 22:02:16, boliu wrote: > > On 2017/03/02 21:46:15, Khushal wrote: > > > That feels like a lot of unnecessary complexity just to ensure that we don't > > > create multiple java bridges in the tree. > > > > I asked for these checks. Imo it's a correctness guarantee that nothing can > > receive events from two different sources. > > What 2 sources are we worried about? The event methods are public on VA, so what > exactly are we trying to restrict? I want to make sure nothing can be receiving events from two different sources (ie EventHandlers) The original condition I wanted was there's at most one EventHandler along any leaf to root path. The currently implemented thing is that only the immediate children of WindowAndroid can have EventHandlers, which is more restrictive. This is all bikeshedding a bit though.. The public methods are easy to fix though, make the private and friend EventHandler.
https://codereview.chromium.org/2708613002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/2708613002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1003: requestUnbufferedDispatch(event); Did we miss to move this to EventHandler?
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 22:17:41, boliu wrote: > On 2017/03/02 22:08:59, Khushal wrote: > > On 2017/03/02 22:02:16, boliu wrote: > > > On 2017/03/02 21:46:15, Khushal wrote: > > > > That feels like a lot of unnecessary complexity just to ensure that we > don't > > > > create multiple java bridges in the tree. > > > > > > I asked for these checks. Imo it's a correctness guarantee that nothing can > > > receive events from two different sources. > > > > What 2 sources are we worried about? The event methods are public on VA, so > what > > exactly are we trying to restrict? > > I want to make sure nothing can be receiving events from two different sources > (ie EventHandlers) Is there a good reason for why we need to have a constraint like this though? With android views, anyone can inject an event into a view. We use it to do things like parents stealing events from their children mid-gesture and send synthetic events to the child. > > The original condition I wanted was there's at most one EventHandler along any > leaf to root path. The currently implemented thing is that only the immediate > children of WindowAndroid can have EventHandlers, which is more restrictive. > > This is all bikeshedding a bit though.. > > The public methods are easy to fix though, make the private and friend > EventHandler.
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 22:34:34, Khushal wrote: > On 2017/03/02 22:17:41, boliu wrote: > > On 2017/03/02 22:08:59, Khushal wrote: > > > On 2017/03/02 22:02:16, boliu wrote: > > > > On 2017/03/02 21:46:15, Khushal wrote: > > > > > That feels like a lot of unnecessary complexity just to ensure that we > > don't > > > > > create multiple java bridges in the tree. > > > > > > > > I asked for these checks. Imo it's a correctness guarantee that nothing > can > > > > receive events from two different sources. > > > > > > What 2 sources are we worried about? The event methods are public on VA, so > > what > > > exactly are we trying to restrict? > > > > I want to make sure nothing can be receiving events from two different sources > > (ie EventHandlers) > > Is there a good reason for why we need to have a constraint like this though? > With android views, anyone can inject an event into a view. We use it to do > things like parents stealing events from their children mid-gesture and send > synthetic events to the child. > > > > > The original condition I wanted was there's at most one EventHandler along any > > leaf to root path. The currently implemented thing is that only the immediate > > children of WindowAndroid can have EventHandlers, which is more restrictive. > > > > This is all bikeshedding a bit though.. > > > > The public methods are easy to fix though, make the private and friend > > EventHandler. Can keep doing that *within* ViewAndroids/ViewClients. And we can build in explicit support if current thing is not good enough. I don't think that's equivalent to having multiple event sources into ViewAndroid tree. And imo it's a lot easier to make a mistake of creating a source on the wrong level.
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 22:41:50, boliu wrote: > On 2017/03/02 22:34:34, Khushal wrote: > > On 2017/03/02 22:17:41, boliu wrote: > > > On 2017/03/02 22:08:59, Khushal wrote: > > > > On 2017/03/02 22:02:16, boliu wrote: > > > > > On 2017/03/02 21:46:15, Khushal wrote: > > > > > > That feels like a lot of unnecessary complexity just to ensure that we > > > don't > > > > > > create multiple java bridges in the tree. > > > > > > > > > > I asked for these checks. Imo it's a correctness guarantee that nothing > > can > > > > > receive events from two different sources. > > > > > > > > What 2 sources are we worried about? The event methods are public on VA, > so > > > what > > > > exactly are we trying to restrict? > > > > > > I want to make sure nothing can be receiving events from two different > sources > > > (ie EventHandlers) > > > > Is there a good reason for why we need to have a constraint like this though? > > With android views, anyone can inject an event into a view. We use it to do > > things like parents stealing events from their children mid-gesture and send > > synthetic events to the child. > > > > > > > > The original condition I wanted was there's at most one EventHandler along > any > > > leaf to root path. The currently implemented thing is that only the > immediate > > > children of WindowAndroid can have EventHandlers, which is more restrictive. > > > > > > This is all bikeshedding a bit though.. > > > > > > The public methods are easy to fix though, make the private and friend > > > EventHandler. > > Can keep doing that *within* ViewAndroids/ViewClients. And we can build in > explicit support if current thing is not good enough. > > I don't think that's equivalent to having multiple event sources into > ViewAndroid tree. And imo it's a lot easier to make a mistake of creating a > source on the wrong level. So here is where my understanding of source. We are basically arguing whether there should be a constraint that only the root of the VA tree can get the event and should trickle it down to the descendants or should anyone be able to inject an event into any View right? Because I don't think merely having an EventHandler hook that lets you plumb events from java should be seen as a different source. And I don't think there is a huge potential for error with someone using a public event API on a View incorrectly. The android View has one, and I don't think that has been a problem so far...
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 22:41:50, boliu wrote: > On 2017/03/02 22:34:34, Khushal wrote: > > On 2017/03/02 22:17:41, boliu wrote: > > > On 2017/03/02 22:08:59, Khushal wrote: > > > > On 2017/03/02 22:02:16, boliu wrote: > > > > > On 2017/03/02 21:46:15, Khushal wrote: > > > > > > That feels like a lot of unnecessary complexity just to ensure that we > > > don't > > > > > > create multiple java bridges in the tree. > > > > > > > > > > I asked for these checks. Imo it's a correctness guarantee that nothing > > can > > > > > receive events from two different sources. > > > > > > > > What 2 sources are we worried about? The event methods are public on VA, > so > > > what > > > > exactly are we trying to restrict? > > > > > > I want to make sure nothing can be receiving events from two different > sources > > > (ie EventHandlers) > > > > Is there a good reason for why we need to have a constraint like this though? > > With android views, anyone can inject an event into a view. We use it to do > > things like parents stealing events from their children mid-gesture and send > > synthetic events to the child. > > > > > > > > The original condition I wanted was there's at most one EventHandler along > any > > > leaf to root path. The currently implemented thing is that only the > immediate > > > children of WindowAndroid can have EventHandlers, which is more restrictive. > > > > > > This is all bikeshedding a bit though.. > > > > > > The public methods are easy to fix though, make the private and friend > > > EventHandler. > > Can keep doing that *within* ViewAndroids/ViewClients. And we can build in > explicit support if current thing is not good enough. > > I don't think that's equivalent to having multiple event sources into > ViewAndroid tree. And imo it's a lot easier to make a mistake of creating a > source on the wrong level. So here is where my understanding of source. We are basically arguing whether there should be a constraint that only the root of the VA tree can get the event and should trickle it down to the descendants or should anyone be able to inject an event into any View right? Because I don't think merely having an EventHandler hook that lets you plumb events from java should be seen as a different source. And I don't think there is a huge potential for error with someone using a public event API on a View incorrectly. The android View has one, and I don't think that has been a problem so far...
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 23:11:36, Khushal wrote: > On 2017/03/02 22:41:50, boliu wrote: > > On 2017/03/02 22:34:34, Khushal wrote: > > > On 2017/03/02 22:17:41, boliu wrote: > > > > On 2017/03/02 22:08:59, Khushal wrote: > > > > > On 2017/03/02 22:02:16, boliu wrote: > > > > > > On 2017/03/02 21:46:15, Khushal wrote: > > > > > > > That feels like a lot of unnecessary complexity just to ensure that > we > > > > don't > > > > > > > create multiple java bridges in the tree. > > > > > > > > > > > > I asked for these checks. Imo it's a correctness guarantee that > nothing > > > can > > > > > > receive events from two different sources. > > > > > > > > > > What 2 sources are we worried about? The event methods are public on VA, > > so > > > > what > > > > > exactly are we trying to restrict? > > > > > > > > I want to make sure nothing can be receiving events from two different > > sources > > > > (ie EventHandlers) > > > > > > Is there a good reason for why we need to have a constraint like this > though? > > > With android views, anyone can inject an event into a view. We use it to do > > > things like parents stealing events from their children mid-gesture and send > > > synthetic events to the child. > > > > > > > > > > > The original condition I wanted was there's at most one EventHandler along > > any > > > > leaf to root path. The currently implemented thing is that only the > > immediate > > > > children of WindowAndroid can have EventHandlers, which is more > restrictive. > > > > > > > > This is all bikeshedding a bit though.. > > > > > > > > The public methods are easy to fix though, make the private and friend > > > > EventHandler. > > > > Can keep doing that *within* ViewAndroids/ViewClients. And we can build in > > explicit support if current thing is not good enough. > > > > I don't think that's equivalent to having multiple event sources into > > ViewAndroid tree. And imo it's a lot easier to make a mistake of creating a > > source on the wrong level. > > So here is where my understanding of source. We are basically arguing whether > there should be a constraint that only the root of the VA tree can get the event > and should trickle it down to the descendants or should anyone be able to inject > an event into any View right? Because I don't think merely having an > EventHandler hook that lets you plumb events from java should be seen as a > different source. > > And I don't think there is a huge potential for error with someone using a > public event API on a View incorrectly. The android View has one, and I don't > think that has been a problem so far... Totally disagree with that assessment. Having any view inject its own event stream has caused loads of integration/refactor problem or just plain odd behavior, from experience working on webview
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/02 23:16:06, boliu wrote: > On 2017/03/02 23:11:36, Khushal wrote: > > On 2017/03/02 22:41:50, boliu wrote: > > > On 2017/03/02 22:34:34, Khushal wrote: > > > > On 2017/03/02 22:17:41, boliu wrote: > > > > > On 2017/03/02 22:08:59, Khushal wrote: > > > > > > On 2017/03/02 22:02:16, boliu wrote: > > > > > > > On 2017/03/02 21:46:15, Khushal wrote: > > > > > > > > That feels like a lot of unnecessary complexity just to ensure > that > > we > > > > > don't > > > > > > > > create multiple java bridges in the tree. > > > > > > > > > > > > > > I asked for these checks. Imo it's a correctness guarantee that > > nothing > > > > can > > > > > > > receive events from two different sources. > > > > > > > > > > > > What 2 sources are we worried about? The event methods are public on > VA, > > > so > > > > > what > > > > > > exactly are we trying to restrict? > > > > > > > > > > I want to make sure nothing can be receiving events from two different > > > sources > > > > > (ie EventHandlers) > > > > > > > > Is there a good reason for why we need to have a constraint like this > > though? > > > > With android views, anyone can inject an event into a view. We use it to > do > > > > things like parents stealing events from their children mid-gesture and > send > > > > synthetic events to the child. > > > > > > > > > > > > > > The original condition I wanted was there's at most one EventHandler > along > > > any > > > > > leaf to root path. The currently implemented thing is that only the > > > immediate > > > > > children of WindowAndroid can have EventHandlers, which is more > > restrictive. > > > > > > > > > > This is all bikeshedding a bit though.. > > > > > > > > > > The public methods are easy to fix though, make the private and friend > > > > > EventHandler. > > > > > > Can keep doing that *within* ViewAndroids/ViewClients. And we can build in > > > explicit support if current thing is not good enough. > > > > > > I don't think that's equivalent to having multiple event sources into > > > ViewAndroid tree. And imo it's a lot easier to make a mistake of creating a > > > source on the wrong level. > > > > So here is where my understanding of source. We are basically arguing whether > > there should be a constraint that only the root of the VA tree can get the > event > > and should trickle it down to the descendants or should anyone be able to > inject > > an event into any View right? Because I don't think merely having an > > EventHandler hook that lets you plumb events from java should be seen as a > > different source. > > > > And I don't think there is a huge potential for error with someone using a > > public event API on a View incorrectly. The android View has one, and I don't > > think that has been a problem so far... > > Totally disagree with that assessment. Having any view inject its own event > stream has caused loads of integration/refactor problem or just plain odd > behavior, from experience working on webview Okay, if you know this has caused pain in the past. :) I tried taking a look at aura to see what happens there, and it seems like it is structured around this idea too. So final goal would be to have only ViewRoot have a public API for getting events, and VA get them only from their parent. For things we do in UI, we can find a way to structure them in the VA framework then. ui/ folks will be able to comment on it better too. It would be nice to not have these methods be public on VA then. Friend-ing EventHandler sounds better.
https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/EventHandler.java:128: mFocusPreOSKViewportRect.setEmpty(); On 2017/03/02 20:58:14, boliu wrote: > On 2017/03/02 04:08:34, Jinsuk Kim wrote: > > On 2017/02/28 22:44:50, boliu wrote: > > > On 2017/02/28 06:56:04, Jinsuk Kim wrote: > > > > On 2017/02/27 19:21:56, boliu wrote: > > > > > this should stay in CVC. > > > > > > > > Put it back to CVC and defined an interface (EventNotifier) to handle it > in > > > CVC. > > > > > > Is that really necessary? Can CVC just catch all the cases where ACTION_DOWN > > > happens. Or there a corner case I'm missing? > > > > Touch event is not going through CVC any more.. the only way I could think of > > doing it in CVC is to call CVC.onTouchEvent() as well from ContentView. See if > > that's aligned with what you want. > > Ok, looked at what this actually does. So CVC is trying to delay scrolling the > focused editable into view, until *after* the resize that's triggered by > onscreen keyboard coming up. And this focus is cancelled if user touches the > screen again before the resize. This all makes sense to me. > > I don't think we want to move this logic to whatever is calling onTouchEvent, > because then it has to be duplicated in a bunch of places. I was thinking maybe > a generic "event observer" is fine, but I just realized view_client already does > that. Just have WCVA look for touch_down, then tell CVC to cancel this. I know > it's going back and forth between jni, but that is the right thing to do, since > we are moving event dispatching to native, but the response here happens in > java. > > Does that make sense? Sounds fair to me. Done. Only thing that gets tricky is that this check was being done before SPen conversion (i.e. SPEN_ACTION_DOWN is not counted as ACTION_DOWN) but I guess that's a bug, since SPEN should also be able to trigger this too? https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client... ui/android/view_client.h:18: struct MotionEventData { On 2017/03/02 04:17:42, aelias wrote: > I tend to agree with Bo. > > > - MEA creates and assigns a unique id for every instance. Not sure if it makes > sense in the new structure where we're likely to create many instances for one > Java motion event. > > This id doesn't seem to be used outside of unit tests. So it doesn't really > matter, we can spam-increment it. > > > - |is_touch_handle_event| was not defined in it in the first place. It didn't > look right (for consistency's sake) to add in MEA the |action_button| used only > for Mouse event and also passed separately. > > I think is_touch_handle_event probably should not be considered a MotionEvent at > all but be passed via special touch-handle-specific side channel. As for > action_button, it sounds fine to add to MEA as long as there is a null value for > non-mouse events. Thanks for the input. Done. Added another ctor to create a new instance from another with an offset. It may look odd but it was to avoid copy/assign which is not allowed. https://codereview.chromium.org/2708613002/diff/140001/ui/android/view_client... ui/android/view_client.h:18: struct MotionEventData { On 2017/03/02 04:21:46, aelias wrote: > On 2017/03/02 at 04:17:42, aelias wrote: > > I think is_touch_handle_event probably should not be considered a MotionEvent > at all but be passed via special touch-handle-specific side channel. As for > action_button, it sounds fine to add to MEA as long as there is a null value for > non-mouse events. > > In the long run, C++ touch handles could be child ViewAndroids of the content > ViewAndroid and the hit testing could happen on the content/ C++ side. Either > way, the handles should fully absorb the MotionEvent. It's pretty weird that > right now we do hit testing on the handle, then continue to propagate the > MotionEvents with a special flag. Acknowledged. Left a TODO in rwhva about removing the distinction of this boolean flag as a reminder for future refactoring. https://codereview.chromium.org/2708613002/diff/160001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2708613002/diff/160001/content/browser/androi... content/browser/android/content_view_core_impl.cc:874: jboolean ContentViewCoreImpl::SendMouseEvent(JNIEnv* env, On 2017/03/02 20:58:14, boliu wrote: > can be deleted as well? Done. https://codereview.chromium.org/2708613002/diff/160001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1423: view_android->SetLayout(0, 0, 0, 0, true); On 2017/03/02 20:58:14, boliu wrote: > /* match parent */ Done. https://codereview.chromium.org/2708613002/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_android.h (right): https://codereview.chromium.org/2708613002/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_android.h:203: base::android::ScopedJavaLocalRef<jobject> CreateEventHandler( On 2017/03/02 22:08:59, Khushal wrote: > nit: GetOrCreateEventHandler since it will create the single EventHandler for > this contents only the first time this is called. Done. https://codereview.chromium.org/2708613002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/2708613002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1003: requestUnbufferedDispatch(event); On 2017/03/02 22:17:43, Khushal wrote: > Did we miss to move this to EventHandler? Good catch. Added while in review. Done. https://codereview.chromium.org/2708613002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2708613002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:949: private boolean sendMouseEvent(MotionEvent event) { On 2017/03/02 22:08:59, Khushal wrote: > May be this can go away now too? Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.cc:24: ScopedJavaLocalRef<jobject> EventHandler::CreateJavaObject() { On 2017/03/02 20:58:14, boliu wrote: > should create only one java side for each native side, and as discussed before, > it's ok to hold a strong ref here because java side doesn't reference anything > else Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:19: EventHandler(); On 2017/03/02 21:46:15, Khushal wrote: > Not used? Removed. https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:20: base::android::ScopedJavaLocalRef<jobject> CreateJavaObject(); On 2017/03/02 21:46:15, Khushal wrote: > Should this class cache the java object instead of creating a new one each time? > May be this should be called GetJavaObject instead? This way we should be able > to clean up the native pointer from the java object in the dtor as well. Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:22: static EventHandler* Create(ViewAndroid* view); On 2017/03/02 21:46:15, Khushal wrote: > nit: Do we need this Create method? We don't do any requisite initializations > there. Can the ctor just be public? > > And if returning a raw ptr, please do mention that the ownership is passed to > the caller. Removed the method. https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:22: static EventHandler* Create(ViewAndroid* view); On 2017/03/02 22:02:16, boliu wrote: > On 2017/03/02 21:46:15, Khushal wrote: > > nit: Do we need this Create method? We don't do any requisite initializations > > there. Can the ctor just be public? > > > > And if returning a raw ptr, please do mention that the ownership is passed to > > the caller. > > should return unique_ptr for passing ownership Acknowledged. https://codereview.chromium.org/2708613002/diff/160001/ui/android/event_handl... ui/android/event_handler.h:22: static EventHandler* Create(ViewAndroid* view); On 2017/03/02 22:08:59, Khushal wrote: > On 2017/03/02 22:02:16, boliu wrote: > > On 2017/03/02 21:46:15, Khushal wrote: > > > nit: Do we need this Create method? We don't do any requisite > initializations > > > there. Can the ctor just be public? > > > > > > And if returning a raw ptr, please do mention that the ownership is passed > to > > > the caller. > > > > should return unique_ptr for passing ownership > > Agreed. That was just a general comment if for some reason you do end up > returning a raw ptr and expect the caller to take ownership. In this case we > should have the ctor be public. Turned the ctor public. ViewAndroid is the owner and keeps a unique_ptr to it. https://codereview.chromium.org/2708613002/diff/160001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/SPenSupport.java (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:30: public static void detect(Context context) { On 2017/03/02 20:58:14, boliu wrote: > you can just use ContextUtils.getApplicationContext? Ah. that was what I was looking for. Turned this to private and enabled lazy init. https://codereview.chromium.org/2708613002/diff/160001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/SPenSupport.java:30: public static void detect(Context context) { On 2017/03/02 21:46:15, Khushal wrote: > nit: initialize sounds better. Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:102: DCHECK(!ViewTreeHasEventHandler(GetViewRoot())) On 2017/03/03 00:37:55, Khushal wrote: > On 2017/03/02 23:16:06, boliu wrote: > > On 2017/03/02 23:11:36, Khushal wrote: > > > On 2017/03/02 22:41:50, boliu wrote: > > > > On 2017/03/02 22:34:34, Khushal wrote: > > > > > On 2017/03/02 22:17:41, boliu wrote: > > > > > > On 2017/03/02 22:08:59, Khushal wrote: > > > > > > > On 2017/03/02 22:02:16, boliu wrote: > > > > > > > > On 2017/03/02 21:46:15, Khushal wrote: > > > > > > > > > That feels like a lot of unnecessary complexity just to ensure > > that > > > we > > > > > > don't > > > > > > > > > create multiple java bridges in the tree. > > > > > > > > > > > > > > > > I asked for these checks. Imo it's a correctness guarantee that > > > nothing > > > > > can > > > > > > > > receive events from two different sources. > > > > > > > > > > > > > > What 2 sources are we worried about? The event methods are public on > > VA, > > > > so > > > > > > what > > > > > > > exactly are we trying to restrict? > > > > > > > > > > > > I want to make sure nothing can be receiving events from two different > > > > sources > > > > > > (ie EventHandlers) > > > > > > > > > > Is there a good reason for why we need to have a constraint like this > > > though? > > > > > With android views, anyone can inject an event into a view. We use it to > > do > > > > > things like parents stealing events from their children mid-gesture and > > send > > > > > synthetic events to the child. > > > > > > > > > > > > > > > > > The original condition I wanted was there's at most one EventHandler > > along > > > > any > > > > > > leaf to root path. The currently implemented thing is that only the > > > > immediate > > > > > > children of WindowAndroid can have EventHandlers, which is more > > > restrictive. > > > > > > > > > > > > This is all bikeshedding a bit though.. > > > > > > > > > > > > The public methods are easy to fix though, make the private and friend > > > > > > EventHandler. > > > > > > > > Can keep doing that *within* ViewAndroids/ViewClients. And we can build in > > > > explicit support if current thing is not good enough. > > > > > > > > I don't think that's equivalent to having multiple event sources into > > > > ViewAndroid tree. And imo it's a lot easier to make a mistake of creating > a > > > > source on the wrong level. > > > > > > So here is where my understanding of source. We are basically arguing > whether > > > there should be a constraint that only the root of the VA tree can get the > > event > > > and should trickle it down to the descendants or should anyone be able to > > inject > > > an event into any View right? Because I don't think merely having an > > > EventHandler hook that lets you plumb events from java should be seen as a > > > different source. > > > > > > And I don't think there is a huge potential for error with someone using a > > > public event API on a View incorrectly. The android View has one, and I > don't > > > think that has been a problem so far... > > > > Totally disagree with that assessment. Having any view inject its own event > > stream has caused loads of integration/refactor problem or just plain odd > > behavior, from experience working on webview > > Okay, if you know this has caused pain in the past. :) I tried taking a look at > aura to see what happens there, and it seems like it is structured around this > idea too. So final goal would be to have only ViewRoot have a public API for > getting events, and VA get them only from their parent. For things we do in UI, > we can find a way to structure them in the VA framework then. ui/ folks will be > able to comment on it better too. > > It would be nice to not have these methods be public on VA then. Friend-ing > EventHandler sounds better. Hid native EventHandler instance (owned by ViewAndroid), and let VA be a friend of EventHandler so only VA can get java EventHandler object. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:108: ViewAndroid* ViewAndroid::GetViewRoot() { On 2017/03/02 20:58:14, boliu wrote: > So.. viewroot is the immediate children of windowandroid? > > I'm not sure we really want to enforce that yet. I imagine chrome refactor will > probably want to separate changes to structure of the tree vs when event > forwarding is updated. > > I think we should just not have a viewroot concept right now (root is the > windowandroid), and go from there. Removed GetViewRoot(). It was used for |ViewTreeHasEventHandler| only - rewrote not to use it. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:109: DCHECK(parent_) << "View is not attached to a tree yet."; On 2017/03/02 21:46:15, Khushal wrote: > nit: This doesn't really need to be a DCHECK. Just return a null root if > detached? Removed this method. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:148: children_.splice(children_.rbegin().base(), children_, it); On 2017/03/02 21:46:15, Khushal wrote: > nit: children_.end() instead of children_.rbegin().base()? Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:223: void ViewAndroid::SetLayout(int x, On 2017/03/02 21:46:15, Khushal wrote: > Do you want to split this into 2 methods: SetBounds and SetMatchParent? I haven't found a case that needs the splitting yet. I guess it would be okay to do that on the first time I run into it. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:229: << "Should give empty dimension when view layout matches its parent."; On 2017/03/02 20:58:14, boliu wrote: > this can be better still. like if I trigger this DCHECK, then I have to add > *more* logging to figure out what if match_parent was true or false and etc.. > just do that here Logged all the parameters. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:290: for (auto it = children_.rbegin(); it != children_.rend(); ++it) { On 2017/03/02 22:08:59, Khushal wrote: > On 2017/03/02 22:02:16, boliu wrote: > > On 2017/03/02 21:46:15, Khushal wrote: > > > nit: for (auto* child : children_)? > > > > back to forward order is desired here. we modeled this on cc::Layer, which is > > painted in forward order, which means for hit testing should be in the reverse > > order > > > > probably worth a comment though > > Oh Duh, didn't notice that. Yeah, a comment for that would be great. Would the > following read cleaner: > for (auto* child : base::Reversed(children_))? Thanks for the suggestion. Reversed looks nicer. Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:294: matched = bound.Contains(e.GetX(), e.GetY()); On 2017/03/02 20:58:14, boliu wrote: > can we abstract out hit testing into its own method, rather than duplicate the > logic in two places? probably would make it easier to test as well? Good idea. Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:301: if (client_ && client_->OnTouchEvent(event)) On 2017/03/02 21:46:15, Khushal wrote: > nit: return client_ ? client_->OnTouchEvent(event) : false; Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_client... ui/android/view_client.h:143: virtual bool OnTouchEvent(const MotionEventData& m); On 2017/03/02 22:08:59, Khushal wrote: > Since we'll be using MotionEvent here, may be you can just pass a bool for > is_handle_event on this method. Done.
replying to old comment I missed.. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:223: void ViewAndroid::SetLayout(int x, On 2017/03/03 06:29:22, Jinsuk Kim wrote: > On 2017/03/02 21:46:15, Khushal wrote: > > Do you want to split this into 2 methods: SetBounds and SetMatchParent? > > I haven't found a case that needs the splitting yet. I guess it would be okay to > do that on the first time I run into it. Actually, should not do split this. This is our version of setLayoutParams, everything should be set in one call. We can always create a struct or something if parameter list gets too long, but this is fine for now..
pretty close now \o/ https://codereview.chromium.org/2708613002/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2708613002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1307: if (mShouldRequestUnbufferedDispatch) requestUnbufferedDispatch(event); this unbuffereddispatch thing is fine for now, but I think eventually this probably should move to EventHandler or something like that. also the android API is just bad bad bad :/ https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... ui/android/event_handler.h:15: class UI_ANDROID_EXPORT EventHandler { I think now that the native side EventHandler is no longer "public" part of ui/, can remove this export? https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... ui/android/event_handler.h:72: friend class ViewAndroid; nit: move friend line right below "private:" https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:337: bool ViewAndroid::HitTest(const EventSender& es, what you need here is argument binding, which of course is what base::Bind is for HitTest can take a callback that just takes a ViewAndroid to do the equivalent of SendToClient. OnTouchEvent than can bind the for_touch_handle arg into the callback. Then HitTest can just recurse on itself. If you want to be fancy, base::Bind can bind lambdas, so you don't even need to declare methods. also a note that virtual struct is very unidiomatic c++ https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.h:132: bool OnTouchEvent(const MotionEventAndroid& event, bool for_touch_handle); these are still public..? I think the original comment was the other way around, that these two methods should be private https://codereview.chromium.org/2708613002/diff/180001/ui/events/android/moti... File ui/events/android/motion_event_android.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/events/android/moti... ui/events/android/motion_event_android.h:66: MotionEventAndroid(const MotionEventAndroid& e, float x, float y); can we just have an offset(x,y) method that returns a new instance?
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:141: void ViewAndroid::MoveToTop(ViewAndroid* child) { MoveToFront feels more clear https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:223: void ViewAndroid::SetLayout(int x, On 2017/03/03 19:46:18, boliu wrote: > On 2017/03/03 06:29:22, Jinsuk Kim wrote: > > On 2017/03/02 21:46:15, Khushal wrote: > > > Do you want to split this into 2 methods: SetBounds and SetMatchParent? > > > > I haven't found a case that needs the splitting yet. I guess it would be okay > to > > do that on the first time I run into it. > > Actually, should not do split this. This is our version of setLayoutParams, > everything should be set in one call. We can always create a struct or something > if parameter list gets too long, but this is fine for now.. The behavior of this feels a bit weird to me. match_parent just overrides the rest of the parameters. Referring to LayoutParams, the width and height use special constants for the match_parent effect, and x and y aren't part of that call at all. So it's impossible to create a call that disregards half the parameters of the struct.
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:223: void ViewAndroid::SetLayout(int x, On 2017/03/03 23:04:34, David Trainor wrote: > On 2017/03/03 19:46:18, boliu wrote: > > On 2017/03/03 06:29:22, Jinsuk Kim wrote: > > > On 2017/03/02 21:46:15, Khushal wrote: > > > > Do you want to split this into 2 methods: SetBounds and SetMatchParent? > > > > > > I haven't found a case that needs the splitting yet. I guess it would be > okay > > to > > > do that on the first time I run into it. > > > > Actually, should not do split this. This is our version of setLayoutParams, > > everything should be set in one call. We can always create a struct or > something > > if parameter list gets too long, but this is fine for now.. > > The behavior of this feels a bit weird to me. match_parent just overrides the > rest of the parameters. Referring to LayoutParams, the width and height use > special constants for the match_parent effect, and x and y aren't part of that > call at all. So it's impossible to create a call that disregards half the > parameters of the struct. We certainly do the same thing. Make a struct with nice factory methods.
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:223: void ViewAndroid::SetLayout(int x, On 2017/03/03 19:46:18, boliu wrote: > On 2017/03/03 06:29:22, Jinsuk Kim wrote: > > On 2017/03/02 21:46:15, Khushal wrote: > > > Do you want to split this into 2 methods: SetBounds and SetMatchParent? > > > > I haven't found a case that needs the splitting yet. I guess it would be okay > to > > do that on the first time I run into it. > > Actually, should not do split this. This is our version of setLayoutParams, > everything should be set in one call. We can always create a struct or something > if parameter list gets too long, but this is fine for now.. It looked a bit awkward for me to see an API where you ask for 2 params which are always supposed to be mutually exclusive. https://codereview.chromium.org/2708613002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:403: return false; // let the children handle the actual event. Don't you always go to the child first, before giving the ancestor a chance to handle the event? https://codereview.chromium.org/2708613002/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2708613002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1307: if (mShouldRequestUnbufferedDispatch) requestUnbufferedDispatch(event); On 2017/03/03 22:01:31, boliu wrote: > this unbuffereddispatch thing is fine for now, but I think eventually this > probably should move to EventHandler or something like that. also the android > API is just bad bad bad :/ Yeah, I was also thinking if there is a clean way to move this to EventHandler. https://codereview.chromium.org/2708613002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1478: public void setCurrentTouchEventOffsets(float dx, float dy) { Are these values still used in this class? Or can you remove this method also now? https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... ui/android/event_handler.cc:19: EventHandler::~EventHandler() { Clear the native ptr in java. https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... ui/android/event_handler.h:72: friend class ViewAndroid; I think you need the other way around. ViewAndroid needs to add EventHandler as a friend to expose the OnTouch methods. And now that we are going with really limiting the access to this class for anyone, might as well make the ctor for this private. VA is the only one that can construct an EventHandler for itself. https://codereview.chromium.org/2708613002/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:62: private boolean sendTouchEvent(MotionEvent event, boolean isTouchHandleEvent) { Since you'll reset the native ptr when the native EventHandler dies, its nice to assert that the nativePtr is not null when you get these calls. https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:150: DCHECK(!ChildrenHaveEventHandler(child) || !ViewTreeHasEventHandler(this)) nit: this is a bit confusing. How about: DCHECK(!child->has_event_handler() || !ViewTreeHasEventHandler(this)); https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:176: if (child->ChildrenHaveEventHandler(child)) You're calling a static method on child? And these 2 private static methods can be in anonymous namespace. https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:349: if (matched && es.SendToView(child, e)) Given that we first send the event to the children, and if consumed the event is never given to the parent. Will the code in WCVA that is relying on seeing the event work? It will never see the event...
https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:141: void ViewAndroid::MoveToTop(ViewAndroid* child) { On 2017/03/03 23:04:34, David Trainor wrote: > MoveToFront feels more clear Done. https://codereview.chromium.org/2708613002/diff/160001/ui/android/view_androi... ui/android/view_android.cc:223: void ViewAndroid::SetLayout(int x, On 2017/03/03 19:46:18, boliu wrote: > On 2017/03/03 06:29:22, Jinsuk Kim wrote: > > On 2017/03/02 21:46:15, Khushal wrote: > > > Do you want to split this into 2 methods: SetBounds and SetMatchParent? > > > > I haven't found a case that needs the splitting yet. I guess it would be okay > to > > do that on the first time I run into it. > > Actually, should not do split this. This is our version of setLayoutParams, > everything should be set in one call. We can always create a struct or something > if parameter list gets too long, but this is fine for now.. Added a struct to make match_parent and other params exclusive. PTAL (I'm afraid I've got too many a reviewer on this :/) https://codereview.chromium.org/2708613002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:403: return false; // let the children handle the actual event. On 2017/03/03 23:36:07, Khushal wrote: > Don't you always go to the child first, before giving the ancestor a chance to > handle the event? Good catch. After some thought, I changed the checking order from parent to child. ViewAndroid tree hierarchy (WCVA -> RWHVA) does not (or has to) follow the Android UI view hierarchy - they always are of the same layout, and the order in which an event is handled can be independent of Java layer. Android view order is reflected in the order of sibling VA's of WCVA, which determines which content is at the front and should receive an event first. https://codereview.chromium.org/2708613002/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2708613002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1307: if (mShouldRequestUnbufferedDispatch) requestUnbufferedDispatch(event); On 2017/03/03 22:01:31, boliu wrote: > this unbuffereddispatch thing is fine for now, but I think eventually this > probably should move to EventHandler or something like that. also the android > API is just bad bad bad :/ Acknowledged. https://codereview.chromium.org/2708613002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1307: if (mShouldRequestUnbufferedDispatch) requestUnbufferedDispatch(event); On 2017/03/03 23:36:07, Khushal wrote: > On 2017/03/03 22:01:31, boliu wrote: > > this unbuffereddispatch thing is fine for now, but I think eventually this > > probably should move to EventHandler or something like that. also the android > > API is just bad bad bad :/ > > Yeah, I was also thinking if there is a clean way to move this to EventHandler. Acknowledged. https://codereview.chromium.org/2708613002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1478: public void setCurrentTouchEventOffsets(float dx, float dy) { On 2017/03/03 23:36:07, Khushal wrote: > Are these values still used in this class? Or can you remove this method also > now? It's used by Chrome. https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... ui/android/event_handler.cc:19: EventHandler::~EventHandler() { On 2017/03/03 23:36:07, Khushal wrote: > Clear the native ptr in java. Done. https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... ui/android/event_handler.h:15: class UI_ANDROID_EXPORT EventHandler { On 2017/03/03 22:01:32, boliu wrote: > I think now that the native side EventHandler is no longer "public" part of ui/, > can remove this export? Done. https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... ui/android/event_handler.h:72: friend class ViewAndroid; On 2017/03/03 22:01:31, boliu wrote: > nit: move friend line right below "private:" Done. https://codereview.chromium.org/2708613002/diff/180001/ui/android/event_handl... ui/android/event_handler.h:72: friend class ViewAndroid; On 2017/03/03 23:36:07, Khushal wrote: > I think you need the other way around. ViewAndroid needs to add EventHandler as > a friend to expose the OnTouch methods. > > And now that we are going with really limiting the access to this class for > anyone, might as well make the ctor for this private. VA is the only one that > can construct an EventHandler for itself. Done. https://codereview.chromium.org/2708613002/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:62: private boolean sendTouchEvent(MotionEvent event, boolean isTouchHandleEvent) { On 2017/03/03 23:36:07, Khushal wrote: > Since you'll reset the native ptr when the native EventHandler dies, its nice to > assert that the nativePtr is not null when you get these calls. Done. https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:150: DCHECK(!ChildrenHaveEventHandler(child) || !ViewTreeHasEventHandler(this)) On 2017/03/03 23:36:07, Khushal wrote: > nit: this is a bit confusing. How about: > DCHECK(!child->has_event_handler() || !ViewTreeHasEventHandler(this)); has_event_handler() won't be enough since it only check direct children. The checking needs to do against all the way down. Renamed the method to |SubtreeHasEventHandler| which I hope makes it look a bit nicer. https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:176: if (child->ChildrenHaveEventHandler(child)) On 2017/03/03 23:36:08, Khushal wrote: > You're calling a static method on child? > And these 2 private static methods can be in anonymous namespace. They access non-public member/method which I'd rather keep them as is now. https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:337: bool ViewAndroid::HitTest(const EventSender& es, On 2017/03/03 22:01:32, boliu wrote: > what you need here is argument binding, which of course is what base::Bind is > for > > HitTest can take a callback that just takes a ViewAndroid to do the equivalent > of SendToClient. OnTouchEvent than can bind the for_touch_handle arg into the > callback. Then HitTest can just recurse on itself. > > If you want to be fancy, base::Bind can bind lambdas, so you don't even need to > declare methods. > > > also a note that virtual struct is very unidiomatic c++ Maybe I need your help on this. I tried Bind here but not can'get it done to make it work for both OnTouchEvent and OnMouseEvent. Uploaded patch doesn't compile - just to show you what I'm doing: OnMouseEvent has the version that uses Bind + methods (SendToClient,SendToView). But in this way I need another set of the methods for OnTouchEvent, which is not good. OnTouchEvent is to show how I tried to address that by using lambda - but Bind doesn't like lambda with capture list - it seems to work only for captureless one. https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:349: if (matched && es.SendToView(child, e)) On 2017/03/03 23:36:08, Khushal wrote: > Given that we first send the event to the children, and if consumed the event is > never given to the parent. Will the code in WCVA that is relying on seeing the > event work? It will never see the event... You're correct. See my other comment about switching the order. https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.h:132: bool OnTouchEvent(const MotionEventAndroid& event, bool for_touch_handle); On 2017/03/03 22:01:32, boliu wrote: > these are still public..? I think the original comment was the other way around, > that these two methods should be private Got the comments wrong. Done. Now ViewAndroid/EventHandler are friends to each other - EventHandler ctor is private and VA is the only class that can instantiate it. https://codereview.chromium.org/2708613002/diff/180001/ui/events/android/moti... File ui/events/android/motion_event_android.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/events/android/moti... ui/events/android/motion_event_android.h:66: MotionEventAndroid(const MotionEventAndroid& e, float x, float y); On 2017/03/03 22:01:32, boliu wrote: > can we just have an offset(x,y) method that returns a new instance? Done this way since copy/Assign is disallowed. See my old reply to aelias@' command.
Patchset #8 (id:200001) has been deleted
replying to old comment https://codereview.chromium.org/2708613002/diff/180001/ui/events/android/moti... File ui/events/android/motion_event_android.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/events/android/moti... ui/events/android/motion_event_android.h:66: MotionEventAndroid(const MotionEventAndroid& e, float x, float y); On 2017/03/06 04:07:34, Jinsuk Kim wrote: > On 2017/03/03 22:01:32, boliu wrote: > > can we just have an offset(x,y) method that returns a new instance? > > Done this way since copy/Assign is disallowed. See my old reply to aelias@' > command. oh is it because the return by value is a copy? Parent class has std::unique_ptr<MotionEvent> MotionEvent::Clone. so I guess you can return a unique_ptr from offset too? Anyway, this is a really odd subclass.. Calling Clone on this class will return a MotionEventGeneric right now afaict o_O
another continuation of old comment.. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.cc:355: send_to_view, you don't need send_to_view, HitTest can just recurse on itself? ahh, you are right about lambda having to be captureless, no lambdas then :/
https://codereview.chromium.org/2708613002/diff/220001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/event_handl... ui/android/event_handler.cc:20: Java_EventHandler_destroy(base::android::AttachCurrentThread(), java_obj_); null check https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:76: static LayoutParams MatchParent() { return {true, 0, 0, 0, 0}; } curious.. did clang not complain about inlining these? https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:90: const bool match_parent; "_" suffix, or you can keep members public and make this a struct https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:205: gfx::Point origin_; // In parent's coordinate space. move comments here to LayoutParams maybe consider just storing a LayoutParam?
https://codereview.chromium.org/2708613002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:403: return false; // let the children handle the actual event. On 2017/03/06 04:07:33, Jinsuk Kim wrote: > On 2017/03/03 23:36:07, Khushal wrote: > > Don't you always go to the child first, before giving the ancestor a chance to > > handle the event? > > Good catch. After some thought, I changed the checking order from parent to > child. ViewAndroid tree hierarchy (WCVA -> RWHVA) does not (or has to) follow > the Android UI view hierarchy - they always are of the same layout, and the > order in which an event is handled can be independent of Java layer. Android > view order is reflected in the order of sibling VA's of WCVA, which determines > which content is at the front and should receive an event first. > I think android just uses a different callback for giving the event to the parent first (onInterceptTouchEvent). But this works well too. :) https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:150: DCHECK(!ChildrenHaveEventHandler(child) || !ViewTreeHasEventHandler(this)) On 2017/03/06 04:07:34, Jinsuk Kim wrote: > On 2017/03/03 23:36:07, Khushal wrote: > > nit: this is a bit confusing. How about: > > DCHECK(!child->has_event_handler() || !ViewTreeHasEventHandler(this)); > > has_event_handler() won't be enough since it only check direct children. The > checking needs to do against all the way down. Renamed the method to > |SubtreeHasEventHandler| which I hope makes it look a bit nicer. Sorry I missed that. Much clearer now. Thanks. https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:176: if (child->ChildrenHaveEventHandler(child)) On 2017/03/06 04:07:34, Jinsuk Kim wrote: > On 2017/03/03 23:36:08, Khushal wrote: > > You're calling a static method on child? > > And these 2 private static methods can be in anonymous namespace. > > They access non-public member/method which I'd rather keep them as is now. > That should still be possible if you put it in the anonymous namespace of this cc file. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:32: class UI_ANDROID_EXPORT ViewAndroid { May be add a comment about the hit-testing order? https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:138: void SetLayout(LayoutParams params); Not for this patch but wanted to ask if this is in the works. Should updating the Layout trickle it down to its descendants to let the ViewClients know they were resized? I'm assuming that's how we're planning to propagate size changes in the hierarchy? https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... File ui/android/view_android_unittests.cc (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android_unittests.cc:98: ExpectHit(client2_); Could you also check that all ancestors to the node that consumes the event get the OnTouchEvent call.
Patchset #9 (id:240001) has been deleted
https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:176: if (child->ChildrenHaveEventHandler(child)) On 2017/03/07 04:17:07, Khushal wrote: > On 2017/03/06 04:07:34, Jinsuk Kim wrote: > > On 2017/03/03 23:36:08, Khushal wrote: > > > You're calling a static method on child? > > > And these 2 private static methods can be in anonymous namespace. > > > > They access non-public member/method which I'd rather keep them as is now. > > > > That should still be possible if you put it in the anonymous namespace of this > cc file. Sorry I still don't get it - |view->children_|, |view->parent_|, and |view->has_event_handler()| all fail if these methods are put in anonymous namespace. https://codereview.chromium.org/2708613002/diff/180001/ui/events/android/moti... File ui/events/android/motion_event_android.h (right): https://codereview.chromium.org/2708613002/diff/180001/ui/events/android/moti... ui/events/android/motion_event_android.h:66: MotionEventAndroid(const MotionEventAndroid& e, float x, float y); On 2017/03/06 22:08:04, boliu wrote: > On 2017/03/06 04:07:34, Jinsuk Kim wrote: > > On 2017/03/03 22:01:32, boliu wrote: > > > can we just have an offset(x,y) method that returns a new instance? > > > > Done this way since copy/Assign is disallowed. See my old reply to aelias@' > > command. > > oh is it because the return by value is a copy? Parent class has > std::unique_ptr<MotionEvent> MotionEvent::Clone. so I guess you can return a > unique_ptr from offset too? > > > Anyway, this is a really odd subclass.. Calling Clone on this class will return > a MotionEventGeneric right now afaict o_O Added |Offset| that returns std::unique_ptr, and also added the private copy ctr for that, since it's hard to make use of the existing ctr to create a new instance from another one (most params are translated to something else at construction). https://codereview.chromium.org/2708613002/diff/220001/ui/android/event_handl... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/event_handl... ui/android/event_handler.cc:20: Java_EventHandler_destroy(base::android::AttachCurrentThread(), java_obj_); On 2017/03/06 22:27:45, boliu wrote: > null check Done. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.cc:355: send_to_view, On 2017/03/06 22:19:59, boliu wrote: > you don't need send_to_view, HitTest can just recurse on itself? > > ahh, you are right about lambda having to be captureless, no lambdas then :/ Sorry missed your comment... now made HitTest recurse itself. Then I only need to have |SendXXXEventToClient| per event type. Will do it this way if you think this is better than the EventSender functor. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:32: class UI_ANDROID_EXPORT ViewAndroid { On 2017/03/07 04:17:08, Khushal wrote: > May be add a comment about the hit-testing order? Done. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:76: static LayoutParams MatchParent() { return {true, 0, 0, 0, 0}; } On 2017/03/06 22:27:45, boliu wrote: > curious.. did clang not complain about inlining these? No it didn't... https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:90: const bool match_parent; On 2017/03/06 22:27:45, boliu wrote: > "_" suffix, or you can keep members public and make this a struct Done. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:138: void SetLayout(LayoutParams params); On 2017/03/07 04:17:08, Khushal wrote: > Not for this patch but wanted to ask if this is in the works. Should updating > the Layout trickle it down to its descendants to let the ViewClients know they > were resized? I'm assuming that's how we're planning to propagate size changes > in the hierarchy? Ah yes - for now it is not necessary to do that since child VA (WCVA) is always set to 'match parent' mode. But the layout change should definitely propagate down to children once that is not true any more. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:205: gfx::Point origin_; // In parent's coordinate space. On 2017/03/06 22:27:45, boliu wrote: > move comments here to LayoutParams > > maybe consider just storing a LayoutParam? Done. Add copy ctor to |LayoutParams| for this (used in |SetLayout|) https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... File ui/android/view_android_unittests.cc (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android_unittests.cc:98: ExpectHit(client2_); On 2017/03/07 04:17:08, Khushal wrote: > Could you also check that all ancestors to the node that consumes the event get > the OnTouchEvent call. Good suggestion. Added |EventCalled()| and applied to the ancestor nodes of the one that got hit - see MatchesViewsWithOffset for example.
https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.cc:355: send_to_view, On 2017/03/07 05:02:23, Jinsuk Kim wrote: > On 2017/03/06 22:19:59, boliu wrote: > > you don't need send_to_view, HitTest can just recurse on itself? > > > > ahh, you are right about lambda having to be captureless, no lambdas then :/ > > Sorry missed your comment... now made HitTest recurse itself. Then I only need > to have |SendXXXEventToClient| per event type. Will do it this way if you think > this is better than the EventSender functor. I meant no lambda, but yes callbacks
https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:176: if (child->ChildrenHaveEventHandler(child)) On 2017/03/07 05:02:23, Jinsuk Kim wrote: > On 2017/03/07 04:17:07, Khushal wrote: > > On 2017/03/06 04:07:34, Jinsuk Kim wrote: > > > On 2017/03/03 23:36:08, Khushal wrote: > > > > You're calling a static method on child? > > > > And these 2 private static methods can be in anonymous namespace. > > > > > > They access non-public member/method which I'd rather keep them as is now. > > > > > > > That should still be possible if you put it in the anonymous namespace of this > > cc file. > > Sorry I still don't get it - |view->children_|, |view->parent_|, and > |view->has_event_handler()| all fail if these methods are put in anonymous > namespace. Sorry for being unclear. I meant |ChildrenHaveEventHandler| and |ViewTreeHasEventHandler| can be moved to anonymous namespace. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:138: void SetLayout(LayoutParams params); On 2017/03/07 05:02:24, Jinsuk Kim wrote: > On 2017/03/07 04:17:08, Khushal wrote: > > Not for this patch but wanted to ask if this is in the works. Should updating > > the Layout trickle it down to its descendants to let the ViewClients know they > > were resized? I'm assuming that's how we're planning to propagate size changes > > in the hierarchy? > > Ah yes - for now it is not necessary to do that since child VA (WCVA) is always > set to 'match parent' mode. But the layout change should definitely propagate > down to children once that is not true any more. Actually propagating them would be required precisely for the match parent case, isn't it. Because if a View is set to match its parent, the ViewClient still needs to know what that size is so it can layout its contents accordingly. I think on android you get that as an OnSizeChanged event. And in content, we effectively do this with ContentViewCoreImpl->RWHVA size updates. RWHVA is also match parent. https://codereview.chromium.org/2708613002/diff/260001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/260001/ui/android/view_androi... ui/android/view_android.h:81: LayoutParams(const LayoutParams& p) LayoutParams(const LayoutParams& p) = default; will work too.
https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:176: if (child->ChildrenHaveEventHandler(child)) On 2017/03/07 06:32:05, Khushal wrote: > On 2017/03/07 05:02:23, Jinsuk Kim wrote: > > On 2017/03/07 04:17:07, Khushal wrote: > > > On 2017/03/06 04:07:34, Jinsuk Kim wrote: > > > > On 2017/03/03 23:36:08, Khushal wrote: > > > > > You're calling a static method on child? > > > > > And these 2 private static methods can be in anonymous namespace. > > > > > > > > They access non-public member/method which I'd rather keep them as is now. > > > > > > > > > > That should still be possible if you put it in the anonymous namespace of > this > > > cc file. > > > > Sorry I still don't get it - |view->children_|, |view->parent_|, and > > |view->has_event_handler()| all fail if these methods are put in anonymous > > namespace. > > Sorry for being unclear. I meant |ChildrenHaveEventHandler| and > |ViewTreeHasEventHandler| can be moved to anonymous namespace. I see it now. :l Please ignore me here.
https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/180001/ui/android/view_androi... ui/android/view_android.cc:176: if (child->ChildrenHaveEventHandler(child)) On 2017/03/07 06:43:13, Khushal wrote: > On 2017/03/07 06:32:05, Khushal wrote: > > On 2017/03/07 05:02:23, Jinsuk Kim wrote: > > > On 2017/03/07 04:17:07, Khushal wrote: > > > > On 2017/03/06 04:07:34, Jinsuk Kim wrote: > > > > > On 2017/03/03 23:36:08, Khushal wrote: > > > > > > You're calling a static method on child? > > > > > > And these 2 private static methods can be in anonymous namespace. > > > > > > > > > > They access non-public member/method which I'd rather keep them as is > now. > > > > > > > > > > > > > That should still be possible if you put it in the anonymous namespace of > > this > > > > cc file. > > > > > > Sorry I still don't get it - |view->children_|, |view->parent_|, and > > > |view->has_event_handler()| all fail if these methods are put in anonymous > > > namespace. > > > > Sorry for being unclear. I meant |ChildrenHaveEventHandler| and > > |ViewTreeHasEventHandler| can be moved to anonymous namespace. > > I see it now. :l Please ignore me here. Acknowledged. https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.cc:355: send_to_view, On 2017/03/07 05:08:49, boliu wrote: > On 2017/03/07 05:02:23, Jinsuk Kim wrote: > > On 2017/03/06 22:19:59, boliu wrote: > > > you don't need send_to_view, HitTest can just recurse on itself? > > > > > > ahh, you are right about lambda having to be captureless, no lambdas then :/ > > > > Sorry missed your comment... now made HitTest recurse itself. Then I only need > > to have |SendXXXEventToClient| per event type. Will do it this way if you > think > > this is better than the EventSender functor. > > I meant no lambda, but yes callbacks Yes already done. PTAL https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/220001/ui/android/view_androi... ui/android/view_android.h:138: void SetLayout(LayoutParams params); On 2017/03/07 06:32:05, Khushal wrote: > On 2017/03/07 05:02:24, Jinsuk Kim wrote: > > On 2017/03/07 04:17:08, Khushal wrote: > > > Not for this patch but wanted to ask if this is in the works. Should > updating > > > the Layout trickle it down to its descendants to let the ViewClients know > they > > > were resized? I'm assuming that's how we're planning to propagate size > changes > > > in the hierarchy? > > > > Ah yes - for now it is not necessary to do that since child VA (WCVA) is > always > > set to 'match parent' mode. But the layout change should definitely propagate > > down to children once that is not true any more. > > Actually propagating them would be required precisely for the match parent case, > isn't it. Because if a View is set to match its parent, the ViewClient still > needs to know what that size is so it can layout its contents accordingly. I > think on android you get that as an OnSizeChanged event. > And in content, we effectively do this with ContentViewCoreImpl->RWHVA size > updates. RWHVA is also match parent. Sorry I meant say 'child VA (RWHVA) is always set to match parent' as you pointed out. The actual view size used for layout is still obtained through CVC. Thanks for the reminder. Will keep this in the next item for refactoring. https://codereview.chromium.org/2708613002/diff/260001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/260001/ui/android/view_androi... ui/android/view_android.h:81: LayoutParams(const LayoutParams& p) On 2017/03/07 06:32:05, Khushal wrote: > LayoutParams(const LayoutParams& p) = default; will work too. nice. Done.
lgtm /w a few nits https://codereview.chromium.org/2708613002/diff/280001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/280001/ui/android/view_androi... ui/android/view_android.h:166: bool HitTest(const base::Callback< nit: alias the callback type so you don't have to type it more than once https://codereview.chromium.org/2708613002/diff/280001/ui/events/android/moti... File ui/events/android/motion_event_android.cc (right): https://codereview.chromium.org/2708613002/diff/280001/ui/events/android/moti... ui/events/android/motion_event_android.cc:243: MotionEventAndroid* event = new MotionEventAndroid(*this); nit: always keep things in unique_ptr, so this can just be a auto event = base::MakeUnique<MotionEventAndroid>(*this) I think
https://codereview.chromium.org/2708613002/diff/280001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2708613002/diff/280001/ui/android/view_androi... ui/android/view_android.h:166: bool HitTest(const base::Callback< On 2017/03/08 02:23:43, boliu wrote: > nit: alias the callback type so you don't have to type it more than once Done. https://codereview.chromium.org/2708613002/diff/280001/ui/events/android/moti... File ui/events/android/motion_event_android.cc (right): https://codereview.chromium.org/2708613002/diff/280001/ui/events/android/moti... ui/events/android/motion_event_android.cc:243: MotionEventAndroid* event = new MotionEventAndroid(*this); On 2017/03/08 02:23:43, boliu wrote: > nit: always keep things in unique_ptr, so this can just be a auto event = > base::MakeUnique<MotionEventAndroid>(*this) I think base::MakeUnique can't instantiate MotionEventAndroid since the ctor is private. Used a standard declaration style instead.
dtrainor@ Could you give owner's seal on chrome/android?
aelias@ Could you review the changes in ui/events/android? I need your approval.
ui/events/android lgtm https://codereview.chromium.org/2708613002/diff/300001/ui/events/android/moti... File ui/events/android/motion_event_android.cc (right): https://codereview.chromium.org/2708613002/diff/300001/ui/events/android/moti... ui/events/android/motion_event_android.cc:428: MotionEventAndroid::CachedPointer MotionEventAndroid::FromCachedPointer( nit: clearer name would be OffsetCachedPointer()
chrome/android lgtm!
lgtm
Patchset #12 (id:320001) has been deleted
Thanks everyone for helpful comments. https://codereview.chromium.org/2708613002/diff/300001/ui/events/android/moti... File ui/events/android/motion_event_android.cc (right): https://codereview.chromium.org/2708613002/diff/300001/ui/events/android/moti... ui/events/android/motion_event_android.cc:428: MotionEventAndroid::CachedPointer MotionEventAndroid::FromCachedPointer( On 2017/03/08 22:23:44, aelias wrote: > nit: clearer name would be OffsetCachedPointer() Done.
The CQ bit was checked by jinsukkim@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...)
The CQ bit was checked by jinsukkim@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 jinsukkim@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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Build on bots failed with following error: /b/c/b/android_n5x_swarming_rel/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold: error: obj/ui/android/android/event_handler.o: multiple definition of 'ui::EventHandler::~EventHandler()' /b/c/b/android_n5x_swarming_rel/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold: obj/ui/events/events/event_handler.o: previous definition here /b/c/b/android_n5x_swarming_rel/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold: error: obj/ui/android/android/event_handler.o: multiple definition of 'ui::EventHandler::~EventHandler()' /b/c/b/android_n5x_swarming_rel/src/third_party/android_tools/ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld.gold: obj/ui/events/events/event_handler.o: previous definition here Local builds are okay though (either with clang/gcc). Renamed EventHandler to EventForwarder to avoid the name clash.
> Local builds are okay though (either with clang/gcc). Renamed EventHandler to > EventForwarder to avoid the name clash. ...should have tried release build... that was it.
On 2017/03/09 07:39:04, Jinsuk Kim wrote: > > Local builds are okay though (either with clang/gcc). Renamed EventHandler to > > EventForwarder to avoid the name clash. > > ...should have tried release build... that was it. so static builds, I guess for component builds, they are not built into same so
jinsukkim@chromium.org changed reviewers: + avi@chromium.org
avi@: Would you review ui/android/DEPS? It's got a new path in the dependency I
need your approval for.
'+ui/base/layout.h'
lgtm
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, aelias@chromium.org, khushalsagar@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2708613002/#ps400001 (title: "EventForwarder...")
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": 400001, "attempt_start_ts": 1489101200115350,
"parent_rev": "f1122bdaf85b4438f6748c0258717de7e4686ab8", "commit_rev":
"5c3a120fa6ff650943131fd0a288adcaf6ccd895"}
Message was sent while issue was closed.
Description was changed from ========== Add EventForwarder for routing touch events EventForwarder replaces ContentViewCore for forwarding Java input/view events to native content. The instance that embedders uses can be obtained via |WebContents.getEventForwarder()|. BUG=671401 ========== to ========== Add EventForwarder for routing touch events EventForwarder replaces ContentViewCore for forwarding Java input/view events to native content. The instance that embedders uses can be obtained via |WebContents.getEventForwarder()|. BUG=671401 Review-Url: https://codereview.chromium.org/2708613002 Cr-Commit-Position: refs/heads/master@{#455913} Committed: https://chromium.googlesource.com/chromium/src/+/5c3a120fa6ff650943131fd0a288... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:400001) as https://chromium.googlesource.com/chromium/src/+/5c3a120fa6ff650943131fd0a288... |
