Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2645353004:
ViewRoot class for event forwarding on Android (Closed)
|
Created:
3 years, 11 months ago by Jinsuk Kim Modified:
3 years, 10 months ago Reviewers:
boliu CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionViewRoot class for event forwarding on Android
This CL introduces a new interface ViewRoot to forward Java
view/input events down to native with a view to replacing the flow
through ContentViewCore eventually.
In native side, ViewRoot is a subclass of ViewAndroid. It acts as
the root of a ViewAndroid tree, and receives touch/view events
from embedders. It has a reference to WindowAndroid, and partially
takes over the role it played as the tree root.
ViewRoot is mapped 1:1 to WindowAndroid and shared across all the
contents. WebView is an exception where all activities share the WA
instance.
ViewRoot, ViewAndroid, and ViewClient are put together to adopt
the pattern "chain of responsibility" dispatching the events to
the classes implementing |ViewClient| along the view hierarchy,
and conditionally stopping the processing when required.
ViewRoot is not in use yet. In the follow-up patches it will replace
WindowAndroid. This CL constructs ViewAndroid layout/bound that
mirrors that of Android view in preparation for the upcoming changes.
BUG=671401
Review-Url: https://codereview.chromium.org/2645353004
Cr-Commit-Position: refs/heads/master@{#448833}
Committed: https://chromium.googlesource.com/chromium/src/+/a3d353091d8189530b26c905d6fb688b333fc819
Patch Set 1 : fix build #Patch Set 2 : fix tests #
Total comments: 41
Patch Set 3 : comments #
Total comments: 4
Patch Set 4 : comments #
Total comments: 10
Patch Set 5 : no ui/display #
Total comments: 4
Patch Set 6 : nit #
Messages
Total messages: 31 (17 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Description was changed from ========== ViewRoot class for event forwarding on Android This CL introduces a new interface ViewRoot to forward Java view/input events down to native with a view to replacing the flow through ContentViewCore eventually. In native side, ViewRoot is a subclass of ViewAndroid. It acts as the root of a ViewAndroid tree, and receives touch/view events from embedders. It has a reference to WindowAndroid, and paritially takes over the role it played as the tree root. ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents. WebView is an exception where all activities share the WA instance. ViewRoot, ViewAndroid, and ViewClient are put together to adopt the pattern "chain of responsibility" dispatching the events to the classes implementing |ViewClient| along the view hierarchy, and conditionally stopping the processing when required. ViewRoot is not in use yet. In the follow-up patches it will replace WindowAndroid. This CL constructs ViewAndroid layout/bound that mirrors that of Android view, and a few other stuff mainly in ui/android (see below) in preparation for the upcoming changes. - a new command line switch --enable-view-root - |DisplayAndroidManager::GetDisplayNearestWindowAndroid| BUG=671401 ========== to ========== ViewRoot class for event forwarding on Android This CL introduces a new interface ViewRoot to forward Java view/input events down to native with a view to replacing the flow through ContentViewCore eventually. In native side, ViewRoot is a subclass of ViewAndroid. It acts as the root of a ViewAndroid tree, and receives touch/view events from embedders. It has a reference to WindowAndroid, and partially takes over the role it played as the tree root. ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents. WebView is an exception where all activities share the WA instance. ViewRoot, ViewAndroid, and ViewClient are put together to adopt the pattern "chain of responsibility" dispatching the events to the classes implementing |ViewClient| along the view hierarchy, and conditionally stopping the processing when required. ViewRoot is not in use yet. In the follow-up patches it will replace WindowAndroid. This CL constructs ViewAndroid layout/bound that mirrors that of Android view in preparation for the upcoming changes. Also added an Android-only method |DisplayAndroidManager::GetDisplayNearestWindowAndroid|. BUG=671401 ==========
Patchset #1 (id:1) has been deleted
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org
Note: MotionEventData is a temporary container of the info about touch/pinch events used while hit testing is performed. Didn't use MotionEventAndroid but create this struct because I needed to a light-weight object that I can create multiple instances of, with various offsets as hit testing going down the child views. MEA doesn't allow copy/assign, which makes it hard to do that. One of the unit tests fails only on build bot. Can't reproduce it locally. Will take another look soon.
really need a design doc.. https://codereview.chromium.org/2645353004/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/ViewRoot.java (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewRoot.java:15: public class ViewRoot { probably should add a TODO or a note to all the new classes are added but not yet used, linking to the bug or design doc https://codereview.chromium.org/2645353004/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewRoot.java:39: final int pointerCount = event.getPointerCount(); can you point out where this code came from? https://codereview.chromium.org/2645353004/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewRoot.java:80: long nativeView = getNativePtr(); if I call destroy twice, this re-initialize native, and then destroy it again? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:80: bool width_matched = (rect_.width() == Bounds::kMatchParent) ? this only holds during the root to child recursion, it doesn't hold if I get a random node and just call IsInBounds there is only one caller, maybe should just move the logic to callsite https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:114: children_.push_front(child); why push front? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:229: gfx::Point delta = bounds_.origin(); this is not wrong, but just a bit odd while reading this the invariant right now is when calling view_root->OnTouchEventInternal, the coordinate of the event is in the coordiante space of *parent* of view_root why not have it be the child's space? ie a child will never see the event at all if it's outside of its bounds hmm.. perhaps the bounds of the child should be stored in the parent, or maybe just the coordinate? hmm... https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:230: const MotionEventData& e = I'm blanking on whether this is safe c++.. if IsOrigin is false, you are retaining a reference to a temporary variable, in which case the variable just goes away immediately after the statement? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:239: if (client_ && client_->OnTouchEvent(event)) hmm, should this happen before looping over children? feels like we need a design doc or something about all these decisions we are just making implicitly here, also maybe should look at what aura does? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:66: static const int kMatchParent = 0x7FFFFFFF; there is no need to use special values here, just use a boolean or an enum, and only use rect_ if boolean/enum says it's not match parent also, do we really need a new class here? why not just two member variables? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:99: virtual float GetDipScale(); protected, and add a comment that says it's virtual for testing https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:113: // Set the bounds used to do hit testing against motion events. relative to parent? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:114: void SetBounds(const gfx::Point& origin, int width, int height); or just take a rect? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:117: bool IsInBounds(const MotionEventData& event); does this need to be public? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:150: scoped_refptr<cc::Layer> layer_; eventually constructing the layer tree probably should move into view_android as well and use bounds from view_android to construct that tree? will that make sense? should we do some research up front? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... File ui/android/view_android_unittest.cc (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android_unittest.cc:79: // +-------+ err, first I got confused this is a diagram of coordinates I think the tree structure ones are overkill, pretty each to see how the tree is constructed https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android_unittest.cc:84: EXPECT_TRUE(client1_.EventHandled()); check view2 did not handle event, here and everywhere else https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_client.... ui/android/view_client.h:18: struct MotionEventData { is there an equivalent of this in content already? https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_root.h File ui/android/view_root.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_root.h#... ui/android/view_root.h:30: void MoveToFront(ViewAndroid* child); why only on the root? https://codereview.chromium.org/2645353004/diff/40001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/display/screen.h#new... ui/display/screen.h:64: // This method is needed since NativeWindow is not a NativeView on Android. wait.. this hasn't happened yet in this CL? https://codereview.chromium.org/2645353004/diff/40001/ui/display/test/test_sc... File ui/display/test/test_screen.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/display/test/test_sc... ui/display/test/test_screen.h:33: Display GetDisplayNearestWindowAndroid(gfx::NativeWindow window) OS_ANDROID
I can write a doc if the changes keep looking confusing. One thing I'd like to get clarified is whether Android view hierarchy is strictly applicable to that of ViewAndroid (WCVA - RWHVA). I think they are quite similar but to some limit. I'd like to know where the similarity stops. https://codereview.chromium.org/2645353004/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/ViewRoot.java (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewRoot.java:15: public class ViewRoot { On 2017/01/24 23:47:12, boliu wrote: > probably should add a TODO or a note to all the new classes are added but not > yet used, linking to the bug or design doc ViewAndroid has a TODO for that. Also added in ViewRoot java/native, ViewClient. https://codereview.chromium.org/2645353004/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewRoot.java:39: final int pointerCount = event.getPointerCount(); On 2017/01/24 23:47:12, boliu wrote: > can you point out where this code came from? ContentViewCore.sendTouchEvent. https://codereview.chromium.org/2645353004/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/ViewRoot.java:80: long nativeView = getNativePtr(); On 2017/01/24 23:47:12, boliu wrote: > if I call destroy twice, this re-initialize native, and then destroy it again? Good catch. Done. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:80: bool width_matched = (rect_.width() == Bounds::kMatchParent) ? On 2017/01/24 23:47:12, boliu wrote: > this only holds during the root to child recursion, it doesn't hold if I get a > random node and just call IsInBounds > > there is only one caller, maybe should just move the logic to callsite Removed the method and check it directly in the callsite (OnTouchEventInternal) https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:114: children_.push_front(child); On 2017/01/24 23:47:12, boliu wrote: > why push front? This is for newly added view to go to front among the siblings. I put a comment at the definition of |children_| in the header. Added additional comment here to make it more explicit/clear. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:229: gfx::Point delta = bounds_.origin(); On 2017/01/24 23:47:12, boliu wrote: > this is not wrong, but just a bit odd while reading this > > the invariant right now is when calling view_root->OnTouchEventInternal, the > coordinate of the event is in the coordiante space of *parent* of view_root > > why not have it be the child's space? ie a child will never see the event at all > if it's outside of its bounds > > hmm.. perhaps the bounds of the child should be stored in the parent, or maybe > just the coordinate? hmm... Would you elaborate? I *think* the event is translated to child's coord by offsetting by parent's origin.... Would it help if I do the layout checking before getting into children's OnTouchEventInternal? I think it will come to the same effect of out-of-bounds child views not seeing the event at all. FYI my intention is that view_root always matches its parent's layout, so it won't be in the way at the beginning of hit testing. And will be matched last after all the children are tested. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:230: const MotionEventData& e = On 2017/01/24 23:47:12, boliu wrote: > I'm blanking on whether this is safe c++.. > > if IsOrigin is false, you are retaining a reference to a temporary variable, in > which case the variable just goes away immediately after the statement? Const guarantees its lifetime: https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-... https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.cc:239: if (client_ && client_->OnTouchEvent(event)) On 2017/01/24 23:47:12, boliu wrote: > hmm, should this happen before looping over children? > > feels like we need a design doc or something about all these decisions we are > just making implicitly here, also maybe should look at what aura does? To the best of my knowledge, child views receives events first(unless their parent intercepts it), and the parent receives it afterwards if there's no match in Android. Let me check how Aura does it in general. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:66: static const int kMatchParent = 0x7FFFFFFF; On 2017/01/24 23:47:12, boliu wrote: > there is no need to use special values here, just use a boolean or an enum, and > only use rect_ if boolean/enum says it's not match parent > > > also, do we really need a new class here? why not just two member variables? Done. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:99: virtual float GetDipScale(); On 2017/01/24 23:47:12, boliu wrote: > protected, and add a comment that says it's virtual for testing Done. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:113: // Set the bounds used to do hit testing against motion events. On 2017/01/24 23:47:12, boliu wrote: > relative to parent? Done. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:114: void SetBounds(const gfx::Point& origin, int width, int height); On 2017/01/24 23:47:13, boliu wrote: > or just take a rect? Changed to get a rect and a bool. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:117: bool IsInBounds(const MotionEventData& event); On 2017/01/24 23:47:12, boliu wrote: > does this need to be public? No... made it private ( and then removed) https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android.h:150: scoped_refptr<cc::Layer> layer_; On 2017/01/24 23:47:12, boliu wrote: > eventually constructing the layer tree probably should move into view_android as > well and use bounds from view_android to construct that tree? will that make > sense? should we do some research up front? Layer tree makes use of physical backing size for its bounds, but it is not always same as view bounds. OverlayPanel's view bounds keeps changing as user interacts with it. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... File ui/android/view_android_unittest.cc (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android_unittest.cc:79: // +-------+ On 2017/01/24 23:47:13, boliu wrote: > err, first I got confused this is a diagram of coordinates > > I think the tree structure ones are overkill, pretty each to see how the tree is > constructed Removed diagrams. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_android... ui/android/view_android_unittest.cc:84: EXPECT_TRUE(client1_.EventHandled()); On 2017/01/24 23:47:13, boliu wrote: > check view2 did not handle event, here and everywhere else Done. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_client.... ui/android/view_client.h:18: struct MotionEventData { On 2017/01/24 23:47:13, boliu wrote: > is there an equivalent of this in content already? No. Previously MotionEventAndroid is created in CVCImpl and used from there. I'm thinking of doing the same - creating one in one of ViewClient impls that wants to consume the event. Until then this struct will be used while checking its coord against ViewAndroids' layout. https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_root.h File ui/android/view_root.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_root.h#... ui/android/view_root.h:30: void MoveToFront(ViewAndroid* child); On 2017/01/24 23:47:13, boliu wrote: > why only on the root? Practically this is necessary only at the top level to decide which web content the event should be routed to. I can make it general and put it in ViewAndroid. But not sure what it means to have more than one child(RWHVA) below it at the same time and switching their order in the view list...? https://codereview.chromium.org/2645353004/diff/40001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/display/screen.h#new... ui/display/screen.h:64: // This method is needed since NativeWindow is not a NativeView on Android. On 2017/01/24 23:47:13, boliu wrote: > wait.. this hasn't happened yet in this CL? It was in the huge CL I'm splitting now. Or shall I take it out to a separate CL? It's not exactly related with view root but more with WindowAndroid that will come later. Put together here only because I had in mind that this CL is about the changes in ui/ https://codereview.chromium.org/2645353004/diff/40001/ui/display/test/test_sc... File ui/display/test/test_screen.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/display/test/test_sc... ui/display/test/test_screen.h:33: Display GetDisplayNearestWindowAndroid(gfx::NativeWindow window) On 2017/01/24 23:47:13, boliu wrote: > OS_ANDROID Done.
On 2017/01/25 02:34:34, Jinsuk Kim wrote: > I can write a doc if the changes keep looking confusing. > It's not confusing. It's that a lot of design decisions are being made here, and those should be discussed in a doc. When there are multiple ways to do things correctly, how do you know you are picking the best one? > One thing I'd like to get clarified is whether Android view hierarchy is > strictly applicable to that of ViewAndroid (WCVA - RWHVA). I think they are > quite similar but to some limit. I'd like to know where the similarity stops. Not sure what you are trying to ask here.
On 2017/01/25 02:45:24, boliu wrote: > On 2017/01/25 02:34:34, Jinsuk Kim wrote: > > I can write a doc if the changes keep looking confusing. > > > > It's not confusing. It's that a lot of design decisions are being made here, and > those should be discussed in a doc. When there are multiple ways to do things > correctly, how do you know you are picking the best one? > > > One thing I'd like to get clarified is whether Android view hierarchy is > > strictly applicable to that of ViewAndroid (WCVA - RWHVA). I think they are > > quite similar but to some limit. I'd like to know where the similarity stops. > > Not sure what you are trying to ask here. Sorry for not being clear :/ I summarized what I had in mind for the implementation in this doc https://docs.google.com/a/google.com/document/d/1Fpvv6aaNwVDTsvHKhgEXZjyEoEgQ... Would be happy to be corrected on any wrong observations/assumptions.
https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_root.h File ui/android/view_root.h (right): https://codereview.chromium.org/2645353004/diff/40001/ui/android/view_root.h#... ui/android/view_root.h:30: void MoveToFront(ViewAndroid* child); On 2017/01/25 02:34:34, Jinsuk Kim wrote: > On 2017/01/24 23:47:13, boliu wrote: > > why only on the root? > > Practically this is necessary only at the top level to decide which web content > the event should be routed to. I can make it general and put it in ViewAndroid. > But not sure what it means to have more than one child(RWHVA) below it at the > same time and switching their order in the view list...? On second thought, this should be in ViewAndroid. Will move it in the next patch.
https://codereview.chromium.org/2645353004/diff/60001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2645353004/diff/60001/ui/android/view_android... ui/android/view_android.cc:100: children_.push_front(child); I think we should keep this push_back to match cc::Layer however since cc paints in layer order, the view that's at the top is the last one, so when doing hit testing, should hit test from the *end* of the child list https://codereview.chromium.org/2645353004/diff/60001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2645353004/diff/60001/ui/android/view_android... ui/android/view_android.h:138: gfx::Rect bounds_; separate position from the size size is something inherent to this view (unless it's match parent..) but position is something in relationship to the parent, and is analogous to the transform in cc::layer, it's just we only support translation transforms here
https://codereview.chromium.org/2645353004/diff/60001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2645353004/diff/60001/ui/android/view_android... ui/android/view_android.cc:100: children_.push_front(child); On 2017/02/05 19:20:23, boliu wrote: > I think we should keep this push_back to match cc::Layer > > however since cc paints in layer order, the view that's at the top is the last > one, so when doing hit testing, should hit test from the *end* of the child list Done. Hit testing is reversed order is done in OnTouchEventInternal. https://codereview.chromium.org/2645353004/diff/60001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2645353004/diff/60001/ui/android/view_android... ui/android/view_android.h:138: gfx::Rect bounds_; On 2017/02/05 19:20:23, boliu wrote: > separate position from the size > > size is something inherent to this view (unless it's match parent..) > > but position is something in relationship to the parent, and is analogous to the > transform in cc::layer, it's just we only support translation transforms here Done.
https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android... ui/android/view_android.cc:82: child->parent_ = nullptr; call RemoveChild to avoid code duplication, though probably need to iterate over copy of children_ https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android... ui/android/view_android.cc:126: origin_.SetPoint(x, y); specify what origin and size are supposed to be when match_parent is true, and DCHECKs to make sure it always happens You have two choices here: 1) make size/origin invalid (maybe origin = 0,0, size=-1,-1) and always query the parent pointer for those values 2) keep size/origin update to date, which means parent has the responsibility of updating all of its match-parent children when its size changes either one is fine https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android... ui/android/view_android.h:141: gfx::Point origin_; add a comment that this is in parent's coordinate space https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_root.h File ui/android/view_root.h (right): https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_root.h#... ui/android/view_root.h:23: void Destroy(JNIEnv* env, nit: group jni methods together https://codereview.chromium.org/2645353004/diff/80001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2645353004/diff/80001/ui/display/screen.h#new... ui/display/screen.h:65: virtual display::Display GetDisplayNearestWindowAndroid( add this when gfx::NativeWindow is no longer a subclass of NativeView, so remove it from this CL
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android... ui/android/view_android.cc:82: child->parent_ = nullptr; On 2017/02/06 18:29:26, boliu wrote: > call RemoveChild to avoid code duplication, though probably need to iterate over > copy of children_ Done. https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android... ui/android/view_android.cc:126: origin_.SetPoint(x, y); On 2017/02/06 18:29:26, boliu wrote: > specify what origin and size are supposed to be when match_parent is true, and > DCHECKs to make sure it always happens > > You have two choices here: > 1) make size/origin invalid (maybe origin = 0,0, size=-1,-1) and always query > the parent pointer for those values > 2) keep size/origin update to date, which means parent has the responsibility of > updating all of its match-parent children when its size changes > > either one is fine Chose 1). Used Point.IsOrigin() and Size.IsEmpty() for that. (negative width/height is not allowed in Size). https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_android... ui/android/view_android.h:141: gfx::Point origin_; On 2017/02/06 18:29:26, boliu wrote: > add a comment that this is in parent's coordinate space Done. https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_root.h File ui/android/view_root.h (right): https://codereview.chromium.org/2645353004/diff/80001/ui/android/view_root.h#... ui/android/view_root.h:23: void Destroy(JNIEnv* env, On 2017/02/06 18:29:26, boliu wrote: > nit: group jni methods together Done. https://codereview.chromium.org/2645353004/diff/80001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2645353004/diff/80001/ui/display/screen.h#new... ui/display/screen.h:65: virtual display::Display GetDisplayNearestWindowAndroid( On 2017/02/06 18:29:26, boliu wrote: > add this when gfx::NativeWindow is no longer a subclass of NativeView, so remove > it from this CL Done.
lgtm https://codereview.chromium.org/2645353004/diff/120001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2645353004/diff/120001/ui/android/view_androi... ui/android/view_android.cc:128: DCHECK(!match_parent || (origin_.IsOrigin() && size_.IsEmpty())); nit: DCHECK input parameters, as the first thing in the method, not after they are already assigned to state https://codereview.chromium.org/2645353004/diff/120001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2645353004/diff/120001/ui/android/view_androi... ui/android/view_android.h:141: gfx::Point origin_; // in parent's coordinate space nit: capitalize first letter of comment and add a period at the end
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...
Description was changed from ========== ViewRoot class for event forwarding on Android This CL introduces a new interface ViewRoot to forward Java view/input events down to native with a view to replacing the flow through ContentViewCore eventually. In native side, ViewRoot is a subclass of ViewAndroid. It acts as the root of a ViewAndroid tree, and receives touch/view events from embedders. It has a reference to WindowAndroid, and partially takes over the role it played as the tree root. ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents. WebView is an exception where all activities share the WA instance. ViewRoot, ViewAndroid, and ViewClient are put together to adopt the pattern "chain of responsibility" dispatching the events to the classes implementing |ViewClient| along the view hierarchy, and conditionally stopping the processing when required. ViewRoot is not in use yet. In the follow-up patches it will replace WindowAndroid. This CL constructs ViewAndroid layout/bound that mirrors that of Android view in preparation for the upcoming changes. Also added an Android-only method |DisplayAndroidManager::GetDisplayNearestWindowAndroid|. BUG=671401 ========== to ========== ViewRoot class for event forwarding on Android This CL introduces a new interface ViewRoot to forward Java view/input events down to native with a view to replacing the flow through ContentViewCore eventually. In native side, ViewRoot is a subclass of ViewAndroid. It acts as the root of a ViewAndroid tree, and receives touch/view events from embedders. It has a reference to WindowAndroid, and partially takes over the role it played as the tree root. ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents. WebView is an exception where all activities share the WA instance. ViewRoot, ViewAndroid, and ViewClient are put together to adopt the pattern "chain of responsibility" dispatching the events to the classes implementing |ViewClient| along the view hierarchy, and conditionally stopping the processing when required. ViewRoot is not in use yet. In the follow-up patches it will replace WindowAndroid. This CL constructs ViewAndroid layout/bound that mirrors that of Android view in preparation for the upcoming changes. BUG=671401 ==========
https://codereview.chromium.org/2645353004/diff/120001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2645353004/diff/120001/ui/android/view_androi... ui/android/view_android.cc:128: DCHECK(!match_parent || (origin_.IsOrigin() && size_.IsEmpty())); On 2017/02/07 17:49:44, boliu wrote: > nit: DCHECK input parameters, as the first thing in the method, not after they > are already assigned to state Done. https://codereview.chromium.org/2645353004/diff/120001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2645353004/diff/120001/ui/android/view_androi... ui/android/view_android.h:141: gfx::Point origin_; // in parent's coordinate space On 2017/02/07 17:49:44, boliu wrote: > nit: capitalize first letter of comment and add a period at the end Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2645353004/#ps140001 (title: "nit")
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": 140001, "attempt_start_ts": 1486514875428250,
"parent_rev": "6559a668099214ba5ef9c5f2dfc514658e326ec2", "commit_rev":
"a3d353091d8189530b26c905d6fb688b333fc819"}
Message was sent while issue was closed.
Description was changed from ========== ViewRoot class for event forwarding on Android This CL introduces a new interface ViewRoot to forward Java view/input events down to native with a view to replacing the flow through ContentViewCore eventually. In native side, ViewRoot is a subclass of ViewAndroid. It acts as the root of a ViewAndroid tree, and receives touch/view events from embedders. It has a reference to WindowAndroid, and partially takes over the role it played as the tree root. ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents. WebView is an exception where all activities share the WA instance. ViewRoot, ViewAndroid, and ViewClient are put together to adopt the pattern "chain of responsibility" dispatching the events to the classes implementing |ViewClient| along the view hierarchy, and conditionally stopping the processing when required. ViewRoot is not in use yet. In the follow-up patches it will replace WindowAndroid. This CL constructs ViewAndroid layout/bound that mirrors that of Android view in preparation for the upcoming changes. BUG=671401 ========== to ========== ViewRoot class for event forwarding on Android This CL introduces a new interface ViewRoot to forward Java view/input events down to native with a view to replacing the flow through ContentViewCore eventually. In native side, ViewRoot is a subclass of ViewAndroid. It acts as the root of a ViewAndroid tree, and receives touch/view events from embedders. It has a reference to WindowAndroid, and partially takes over the role it played as the tree root. ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents. WebView is an exception where all activities share the WA instance. ViewRoot, ViewAndroid, and ViewClient are put together to adopt the pattern "chain of responsibility" dispatching the events to the classes implementing |ViewClient| along the view hierarchy, and conditionally stopping the processing when required. ViewRoot is not in use yet. In the follow-up patches it will replace WindowAndroid. This CL constructs ViewAndroid layout/bound that mirrors that of Android view in preparation for the upcoming changes. BUG=671401 Review-Url: https://codereview.chromium.org/2645353004 Cr-Commit-Position: refs/heads/master@{#448833} Committed: https://chromium.googlesource.com/chromium/src/+/a3d353091d8189530b26c905d6fb... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a3d353091d8189530b26c905d6fb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
