Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2502763003:
Introduce ViewRoot to forward input/view events to native (Closed)
|
Created:
4 years, 1 month ago by Jinsuk Kim Modified:
3 years, 11 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, mdjones+watch_chromium.org, donnd+watch_chromium.org, shaktisahu+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, jam, scf+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, agrieve+watch_chromium.org, darin-cc_chromium.org, dtrainor+watch-blimp_chromium.org, android-webview-reviews_chromium.org, khushalsagar+watch-blimp_chromium.org, wychen, David Trainor- moved to gerrit Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce ViewRoot forwarding input/view events to native
This CL introduces a new interface ViewRoot to forward Java
view/input events down to native, replacing the flow through
ContentViewCore.
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.
Embedders can obtain the interface from WindowAndroid
whose native part is the root of the ViewAndroid hierarchy.
WebView is an exception where all activities share
the WA instance, hence cannot tell which path in the hierarchy
to dispatch the event to. For this reason WebView gets ViewRoot
from the top ViewAndroid (the one right below WA) instead.
|OnPhysicalBackingSizedChanged| is used for an example.
BUG=671401
Committed: https://crrev.com/603de249d822522b152b29184be793550695f2c8
Cr-Commit-Position: refs/heads/master@{#439636}
Patch Set 1 #
Total comments: 4
Patch Set 2 : ViewClient #Patch Set 3 : event handler #Patch Set 4 : lazy creation #
Total comments: 22
Patch Set 5 : addressed comments #
Total comments: 25
Patch Set 6 : WindowAndroid.GetEventHandler #
Total comments: 28
Patch Set 7 : addressed comments #
Total comments: 27
Patch Set 8 : event_handler.h/cc merged to view_android #Patch Set 9 : call EventHandler.create directly #
Total comments: 6
Patch Set 10 : tidy up #
Total comments: 4
Patch Set 11 : no overloaded GetEventHandler #
Total comments: 6
Patch Set 12 : ViewRoot #
Total comments: 2
Patch Set 13 : rebased/fixed tests #
Total comments: 7
Patch Set 14 : prevent view_root_ gc'ed #Messages
Total messages: 93 (42 generated)
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org
Note that the patch is not fully functional (i.e. migration of handling onPhysicalBackingSizeChanged from CVC to ViewAndroidDelegate is not complete and may have bugs) but would like to get your initial feedback on how ViewAndroid forwards the call from Java to Native. Also added back CVC.getViewDelegate() just because too much plumbing changes would be necessary without it. I left a TODO to remove it again once embedders expose the access to ViewAndroidDelegate and we don't have to go through CVC to get it.
so physical size just controls the cc layer size, which is now owned by ViewAndroid, so there is no point in all that set changed stuff I don't think going through ViewAndroidDelegate is the right thing, it's a class implemented by embedder, so embedder should pretty much never directly call anything on it. Also I believe Delegate has a one-to-many relationship with the ViewAndroid tree, so can't really map from Delegate to ViewAndroid. In this case, embedder should send signals either to the right view, if it knows which view exactly, or to the root of the view. Either case, for resize, it should just go directly to either the java WebContents, then eventually to WebContentsViewAndroid, which happen to own both the (non-window) root and also the right ViewAndroid. https://codereview.chromium.org/2502763003/diff/1/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/1/ui/android/view_android.cc#... ui/android/view_android.cc:89: Java_ViewAndroidDelegate_setNativePointer(env, delegate has a many-to-one relationship with view, right? don't think this will work
Thanks for the feedback. Will look into forwarding through WebContents. https://codereview.chromium.org/2502763003/diff/1/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/1/ui/android/view_android.cc#... ui/android/view_android.cc:89: Java_ViewAndroidDelegate_setNativePointer(env, On 2016/11/15 04:51:03, boliu wrote: > delegate has a many-to-one relationship with view, right? don't think this will > work My intention was to maintain only one handler per each api to keep one-to-one mapping.
Description was changed from ========== Send OnPhysicalBackingSizeChanged event down through ViewAndroid Defined a method to forward the Java view event down to native through ViewAndroid replacing the flow through ContentViewCore. The ViewAndroid that has the java delegate keeps the handlers that receive the call, and forward them to the object that registered the handler in advance. |OnPhysicalBackingSizedChanged| is used for an example. BUG=598880 ========== to ========== Send OnPhysicalBackingSizeChanged event down through ViewAndroid Defined a method to forward the Java view event down to native through ViewAndroid replacing the flow through ContentViewCore. The ViewAndroid that has the java delegate keeps the handlers that receive the call, and forward them to the object that registered the handler in advance. |OnPhysicalBackingSizedChanged| is used for an example. BUG=598880 ==========
Message was sent while issue was closed.
changwan@chromium.org changed reviewers: + changwan@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2502763003/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2502763003/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:23: public abstract class ViewAndroidDelegate { How about separating this class into ViewAndroid and ViewAndroid.Delegate? Then container View can only access public methods in ViewAndroid.Delegate, while native view_android can only access other set of methods. In case, we want to separate life cycles of view_android and ContentView.java, we can simply do null check in the embedder. Also this allows different implementation for webview and chrome. And IMO renaming this class to ViewAndroid matches view_android.cpp implementation better.
Message was sent while issue was closed.
https://codereview.chromium.org/2502763003/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right): https://codereview.chromium.org/2502763003/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:23: public abstract class ViewAndroidDelegate { On 2016/11/17 01:23:50, Changwan Ryu wrote: > How about separating this class into ViewAndroid and ViewAndroid.Delegate? > > Then container View can only access public methods in ViewAndroid.Delegate, > while native view_android can only access other set of methods. > > In case, we want to separate life cycles of view_android and ContentView.java, > we can simply do null check in the embedder. > > Also this allows different implementation for webview and chrome. And IMO > renaming this class to ViewAndroid matches view_android.cpp implementation > better. There was a note from daniel somewhere that we should avoid java ViewAndroids if possible. Also that this is the ui layer, where the "embdder" is either content, or blimp. I think as discussed in the meeting, the entry point for input events or size changes can just come in at WindowAndroid, which is always the root of the tree. We will need "delegates" for each ViewAndroid, not just one for the whole tree. Maybe we can sit down tomorrow and discuss this while you guys are still here :)
Message was sent while issue was closed.
> How about separating this class into ViewAndroid and ViewAndroid.Delegate? > > Then container View can only access public methods in ViewAndroid.Delegate, > while native view_android can only access other set of methods. > > In case, we want to separate life cycles of view_android and ContentView.java, > we can simply do null check in the embedder. > > Also this allows different implementation for webview and chrome. And IMO > renaming this class to ViewAndroid matches view_android.cpp implementation > better. Had tried it before but was suggested that they be merted into one. See sievers@'s comments: https://codereview.chromium.org/2103243002/diff/140001/ui/android/java/src/or... So this was designed to be unidirectional from the beginning. I think your suggestion is possible - with just a different name for the interface you have in mind.
Description was changed from ========== Send OnPhysicalBackingSizeChanged event down through ViewAndroid Defined a method to forward the Java view event down to native through ViewAndroid replacing the flow through ContentViewCore. The ViewAndroid that has the java delegate keeps the handlers that receive the call, and forward them to the object that registered the handler in advance. |OnPhysicalBackingSizedChanged| is used for an example. BUG=598880 ========== to ========== Send OnPhysicalBackingSizeChanged event down through EventHandler Defined a method to forward the Java view event down to native through a new interface EventHandler replacing the flow through ContentViewCore. |OnPhysicalBackingSizedChanged| is used for an example. BUG=598880 ==========
just adding a comment here so it goes into my review queue
EventHandler implementation should be in ui/ I don't think there needs to be a native EventHandler interface for embedder to use It should be created lazily by ViewAndroid when asked by embedder. And we should maintain and DCHECK the property that each leave ViewAndroid can have at most one EventHandler created on its path to the root. Then I guess chrome can create one from the root, and webview can create it from ViewAndroid attached to WebContentsViewAndroid
PTAL. > EventHandler implementation should be in ui/ > > I don't think there needs to be a native EventHandler interface for embedder to > use Removed interfaces from both sides - it seems to work. > > It should be created lazily by ViewAndroid when asked by embedder. And we should > maintain and DCHECK the property that each leave ViewAndroid can have at most > one EventHandler created on its path to the root. > > Then I guess chrome can create one from the root, and webview can create it from > ViewAndroid attached to WebContentsViewAndroid Turned to a lazy creation on native side. It's not clear to me how to tell the embedder in ViewAndroid. I let the top ViewAndroid (the one right beneath the root) maintain the EventHandler to keep it unique through the VA chain. This works for both. Or please elaborate what you suggested.
mdjones@: Please review the changes made in OverlayPanel. Tested it on N7 where the panel size is smaller than parent and it seems to work.
mdjones@chromium.org changed reviewers: + mdjones@chromium.org
OverlayPanel lgtm
This is not a pure refactor. We are going to change how this size is propagated: it's injected by EventHandler at a ViewAndroid, and then propagate to all the children. Any child added should also have its size updated to match its new parent. EventHandler should not hold state, it just calls stuff on ViewAndroid. We need to maintain the property that there is at most one EventHandler for any path from a root to a particular leaf. It is going to involve traversing the whole ViewAndroid tree (wonder if that will make a good interview question :p). Then need to check every time a EventHandler is added, and any time a child is added (removing won't break the property). Yes it's going to be expensive, so only do it on debug builds. https://codereview.chromium.org/2502763003/diff/60001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2502763003/diff/60001/content/browser/android... content/browser/android/content_view_core_impl.cc:221: gfx::Size physical_size(GetViewAndroid()->GetPhysicalBackingSize()); this and below seems redundant, retrieve the physical size from ViewAndroid, and then set it somewhere inside ViewAndroid can ViewAndroid maintain internally the size is set on the layer? https://codereview.chromium.org/2502763003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2502763003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:244: view_.GetLayer()->SetBounds(physical_size); ditto about keeping the layer in sync https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handler.h File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handle... ui/android/event_handler.h:18: class UI_ANDROID_EXPORT EventHandler { ViewAndroid::GetEventHandler definitely should not be public Is it possible to not have this in a header at all? Just make all the java->native calls static so the generator doesn't need a separate header for the class, then can directly implement things internally in view_android.cc? https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handle... ui/android/event_handler.h:33: int physical_width_pix_; this is a property on ViewAndroid, not handler this is where the whole ViewAndroid hierarchy comes in, the size needs to be propagated down the tree, and when a child is added, the size should be propagated to the child as well https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.cc:95: if (!parent_ || parent_ == GetWindowAndroid()) { what is the second half of this condition checking? is !parent_ check for root not enough? https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.h:62: class ViewClient { put this in a separate file, presumably this will grow a lot more in the future https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.h:64: bool OnPhysicalBackingSizeChanged(int width, int height) { return false; } virtual? also I think inlining virtual method is not allowed https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.h:88: void SetViewClient(ViewClient* client) { client_ = client; } can we pass this in on construction?
On 2016/12/02 17:01:51, mdjones wrote: > OverlayPanel lgtm Oops sorry - I meant to ask you to review https://crrev.com/2536223003 Would you take a look at that?
> We need to maintain the property that there is at most one EventHandler for any > path from a root to a particular leaf. It is going to involve traversing the > whole ViewAndroid tree (wonder if that will make a good interview question :p). > Then need to check every time a EventHandler is added, and any time a child is > added (removing won't break the property). Yes it's going to be expensive, so > only do it on debug builds. I think the uniqueness of the handler can be guaranteed by always setting it at the top VA. Please see the updated code and let me know what you think. https://codereview.chromium.org/2502763003/diff/60001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2502763003/diff/60001/content/browser/android... content/browser/android/content_view_core_impl.cc:221: gfx::Size physical_size(GetViewAndroid()->GetPhysicalBackingSize()); On 2016/12/02 23:53:22, boliu wrote: > this and below seems redundant, retrieve the physical size from ViewAndroid, and > then set it somewhere inside ViewAndroid > > can ViewAndroid maintain internally the size is set on the layer? I don't think it is necessary to set the bounds here because physical backing size at this moment would be zero and soon the updated size will come down through event handler. Removed. https://codereview.chromium.org/2502763003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2502763003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:244: view_.GetLayer()->SetBounds(physical_size); On 2016/12/02 23:53:22, boliu wrote: > ditto about keeping the layer in sync Defined UpdateLayerBounds() and called it when physical backing size is updated. This is not complete but help keep the layer size in sync with that of view. https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handler.h File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handle... ui/android/event_handler.h:18: class UI_ANDROID_EXPORT EventHandler { On 2016/12/02 23:53:22, boliu wrote: > ViewAndroid::GetEventHandler definitely should not be public > > Is it possible to not have this in a header at all? Just make all the > java->native calls static so the generator doesn't need a separate header for > the class, then can directly implement things internally in view_android.cc? Removed the class EventHandler and defined the example method static. https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handle... ui/android/event_handler.h:33: int physical_width_pix_; On 2016/12/02 23:53:22, boliu wrote: > this is a property on ViewAndroid, not handler > > this is where the whole ViewAndroid hierarchy comes in, the size needs to be > propagated down the tree, and when a child is added, the size should be > propagated to the child as well Moved the properties to ViewAndroid. I *think* the size of the new child is being updated already somewhere else when added. Do you suggest it be done in ViewAndroid::AddChild? https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.cc:95: if (!parent_ || parent_ == GetWindowAndroid()) { On 2016/12/02 23:53:22, boliu wrote: > what is the second half of this condition checking? is !parent_ check for root > not enough? In the tree WindowAndroid - ViewAndroid(1) - ViewAndroid(2), I'm adding the handler in VA(1) not in WA which is the root. Because it won't work for WebView. VA(1) works for both Chrome and WebView. The second part is actually the condition that checks if we are VA(1). !parent_ was for sentinel just in case VA is not linked to WA. I haven't seen it yet though. https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.h:62: class ViewClient { On 2016/12/02 23:53:22, boliu wrote: > put this in a separate file, presumably this will grow a lot more in the future Done. https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.h:64: bool OnPhysicalBackingSizeChanged(int width, int height) { return false; } On 2016/12/02 23:53:22, boliu wrote: > virtual? also I think inlining virtual method is not allowed Done. https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.h:88: void SetViewClient(ViewClient* client) { client_ = client; } On 2016/12/02 23:53:22, boliu wrote: > can we pass this in on construction? Done.
Just replying to previous comments. On 2016/12/05 11:07:45, Jinsuk Kim wrote: > I think the uniqueness of the handler can be guaranteed by always setting it at > the top VA. Please see the updated code and let me know what you think. I think for chrome (and all other platforms except webview), EventHandler should be created at the root, ie on the WindowAndroid. Webview is the special one here.. So if you want simpler checks that only the root or its immediate children can have an EventHandler, I guess that's fine. I don't think that's actually much simpler than the general property though, since you still need to verify a whole subtree when a new node is added. https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handler.h File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handle... ui/android/event_handler.h:33: int physical_width_pix_; On 2016/12/05 11:07:44, Jinsuk Kim wrote: > On 2016/12/02 23:53:22, boliu wrote: > > this is a property on ViewAndroid, not handler > > > > this is where the whole ViewAndroid hierarchy comes in, the size needs to be > > propagated down the tree, and when a child is added, the size should be > > propagated to the child as well > > Moved the properties to ViewAndroid. > > I *think* the size of the new child is being updated already somewhere else when > added. Do you suggest it be done in ViewAndroid::AddChild? Yes https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.cc:95: if (!parent_ || parent_ == GetWindowAndroid()) { On 2016/12/05 11:07:44, Jinsuk Kim wrote: > On 2016/12/02 23:53:22, boliu wrote: > > what is the second half of this condition checking? is !parent_ check for root > > not enough? > > In the tree WindowAndroid - ViewAndroid(1) - ViewAndroid(2), I'm adding the > handler in VA(1) not in WA which is the root. Because it won't work for WebView. > VA(1) works for both Chrome and WebView. The second part is actually the > condition that checks if we are VA(1). !parent_ was for sentinel just in case VA > is not linked to WA. I haven't seen it yet though. Ahh I see.. But for chrome (and other non-webview platforms), EventHandler *should* be created for WindowAndroid. Also I think GetEventHandler should not be a recursive call. It's more like "GetOrCreateEventHandlerForThisViewAndroid", which may be illegal and DCHECK if it violates the above property.
only looked at content and ui https://codereview.chromium.org/2502763003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2502763003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:245: RenderWidgetHostView* rwhv = !web_contents_->ShowingInterstitialPage() ? it's not clear to me why WasResized should only called on the current RWHVA, but not the "background" one, is there a precedence where the old code used to do this? eg is there code that updates the "background" RWHVA after the interstitial is dismissed? WebContentsImpl::DidProceedOnInterstitial or DetachInterstitialPage doesn't do this it seems on the other hand, shouldn't each RWHVA get its own OnPhysicalBackingSizeChanged implementation, and then can observe size updates? https://codereview.chromium.org/2502763003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java (right): https://codereview.chromium.org/2502763003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:75: getEventHandler().onPhysicalBackingSizeChanged(width, height); looks like this class is used only by chromecast, and it's supposed to be some global thing, so directing this to the window is fine odd it's in content, feels like it should be in chromecast instead https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handle... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handle... ui/android/event_handler.cc:15: void OnPhysicalBackingSizeChanged(JNIEnv* env, is this static in the generated header? if so, add "// static", if not, then that's not good because it's supposed to be static.. https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handle... ui/android/event_handler.cc:26: } nit: blank line below https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handler.h File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handle... ui/android/event_handler.h:12: bool RegisterEventHandler(JNIEnv* env); so I was thinking maybe this h/cc pair should just be part of view_android.h/cc, but I guess that's breaking an existing pattern, so maybe not. up to you https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.cc:184: if (!parent_ || parent_ == GetWindowAndroid()) should just return own size, not recurse ie we should pay the cost to update the whole tree in SetPhysicalBackingSize, so that the getter is free https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.cc:207: return parent_->SetPhysicalBackingSize(width, height); recursion in the wrong direction.. this value should be propagated *down*, not up? oh actually, looking at OnPhysicalBackingSizeChanged below, this should not recurse at all? https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.cc:212: layer_->SetBounds(GetPhysicalBackingSize()); just do this inside SetPhyiscalBackingSize, then client don't ever need to worry about UpdateLayerBounds https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.h:58: ViewAndroid(ViewClient* client); explicit https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.h:112: ViewClient* client_; ViewClient* const client_;, it should be const, right? https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_client.... ui/android/view_client.h:18: virtual bool OnPhysicalBackingSizeChanged(int width, int height); if this were touch events, then returning a bool would make sense but why would anything want to stop propagating a size change? I guess I need to look at actual implementation to see what uses this, but this seems odd, just looking at ui code
Removed Java WebContents.getEventHandler() and moved it to WindowAndroid so that embedders can use WA to get the event handler associated with native view root (WA). WebView is an exception that uses its own AwContents.getEventHandler() which gets the ViewAndroid owned by WebContentsViewAndroid. Added DCHECK (in ViewAndroid::AddChild + ViewAndroid::OnSetEventHandler) to make sure there will be a unique event handler in a certain view hierarchy. https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handler.h File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handle... ui/android/event_handler.h:33: int physical_width_pix_; On 2016/12/06 00:10:17, boliu wrote: > On 2016/12/05 11:07:44, Jinsuk Kim wrote: > > On 2016/12/02 23:53:22, boliu wrote: > > > this is a property on ViewAndroid, not handler > > > > > > this is where the whole ViewAndroid hierarchy comes in, the size needs to be > > > propagated down the tree, and when a child is added, the size should be > > > propagated to the child as well > > > > Moved the properties to ViewAndroid. > > > > I *think* the size of the new child is being updated already somewhere else > when > > added. Do you suggest it be done in ViewAndroid::AddChild? > > Yes Traced the code and found that child nodes are all added before the first physical backing size change happens - so it has not been necessary to update the size to that of the parent so far. Do you believe it will necessary to do that in the future? I think simply calling child->OnPhysicalBackingSizeChanged() in AddChild if the parent's size is valid will do the trick. https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/view_android... ui/android/view_android.cc:95: if (!parent_ || parent_ == GetWindowAndroid()) { On 2016/12/06 00:10:17, boliu wrote: > On 2016/12/05 11:07:44, Jinsuk Kim wrote: > > On 2016/12/02 23:53:22, boliu wrote: > > > what is the second half of this condition checking? is !parent_ check for > root > > > not enough? > > > > In the tree WindowAndroid - ViewAndroid(1) - ViewAndroid(2), I'm adding the > > handler in VA(1) not in WA which is the root. Because it won't work for > WebView. > > VA(1) works for both Chrome and WebView. The second part is actually the > > condition that checks if we are VA(1). !parent_ was for sentinel just in case > VA > > is not linked to WA. I haven't seen it yet though. > > Ahh I see.. > > But for chrome (and other non-webview platforms), EventHandler *should* be > created for WindowAndroid. > > Also I think GetEventHandler should not be a recursive call. It's more like > "GetOrCreateEventHandlerForThisViewAndroid", which may be illegal and DCHECK if > it violates the above property. Addressed in the new patch. https://codereview.chromium.org/2502763003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2502763003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:245: RenderWidgetHostView* rwhv = !web_contents_->ShowingInterstitialPage() ? On 2016/12/06 00:34:14, boliu wrote: > it's not clear to me why WasResized should only called on the current RWHVA, but > not the "background" one, is there a precedence where the old code used to do > this? > > eg is there code that updates the "background" RWHVA after the interstitial is > dismissed? WebContentsImpl::DidProceedOnInterstitial or DetachInterstitialPage > doesn't do this it seems > > > on the other hand, shouldn't each RWHVA get its own OnPhysicalBackingSizeChanged > implementation, and then can observe size updates? I don't have good answers to your questions. You may be right in that each RWHVA should get PBSC event but that's not what CVC was doing before this CL. It only updates current RWHVA (either actual content or interstitial). https://codereview.chromium.org/2502763003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java (right): https://codereview.chromium.org/2502763003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:75: getEventHandler().onPhysicalBackingSizeChanged(width, height); On 2016/12/06 00:34:14, boliu wrote: > looks like this class is used only by chromecast, and it's supposed to be some > global thing, so directing this to the window is fine > > odd it's in content, feels like it should be in chromecast instead Ack. I'll see about moving it out of content in a follow up CL https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handle... File ui/android/event_handler.cc (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handle... ui/android/event_handler.cc:15: void OnPhysicalBackingSizeChanged(JNIEnv* env, On 2016/12/06 00:34:14, boliu wrote: > is this static in the generated header? if so, add "// static", if not, then > that's not good because it's supposed to be static.. Yes it has static. Put it here too. https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handle... ui/android/event_handler.cc:26: } On 2016/12/06 00:34:14, boliu wrote: > nit: blank line below Done. https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handler.h File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/event_handle... ui/android/event_handler.h:12: bool RegisterEventHandler(JNIEnv* env); On 2016/12/06 00:34:14, boliu wrote: > so I was thinking maybe this h/cc pair should just be part of view_android.h/cc, > but I guess that's breaking an existing pattern, so maybe not. up to you Acknowledged. https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android.cc File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.cc:184: if (!parent_ || parent_ == GetWindowAndroid()) On 2016/12/06 00:34:14, boliu wrote: > should just return own size, not recurse > > ie we should pay the cost to update the whole tree in SetPhysicalBackingSize, so > that the getter is free Done. https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.cc:207: return parent_->SetPhysicalBackingSize(width, height); On 2016/12/06 00:34:14, boliu wrote: > recursion in the wrong direction.. this value should be propagated *down*, not > up? > > oh actually, looking at OnPhysicalBackingSizeChanged below, this should not > recurse at all? Done. https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.cc:212: layer_->SetBounds(GetPhysicalBackingSize()); On 2016/12/06 00:34:14, boliu wrote: > just do this inside SetPhyiscalBackingSize, then client don't ever need to worry > about UpdateLayerBounds Done. https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.h:58: ViewAndroid(ViewClient* client); On 2016/12/06 00:34:14, boliu wrote: > explicit Done. https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_android... ui/android/view_android.h:112: ViewClient* client_; On 2016/12/06 00:34:14, boliu wrote: > ViewClient* const client_;, it should be const, right? Yes. Done. https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2502763003/diff/80001/ui/android/view_client.... ui/android/view_client.h:18: virtual bool OnPhysicalBackingSizeChanged(int width, int height); On 2016/12/06 00:34:14, boliu wrote: > if this were touch events, then returning a bool would make sense > > but why would anything want to stop propagating a size change? I guess I need to > look at actual implementation to see what uses this, but this seems odd, just > looking at ui code > Not really a good example for the change :/ Turned to void. Still not sure why current code does not update all the RWVHA.
replying to a few comments https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handler.h File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handle... ui/android/event_handler.h:33: int physical_width_pix_; On 2016/12/06 07:35:07, Jinsuk Kim wrote: > On 2016/12/06 00:10:17, boliu wrote: > > On 2016/12/05 11:07:44, Jinsuk Kim wrote: > > > On 2016/12/02 23:53:22, boliu wrote: > > > > this is a property on ViewAndroid, not handler > > > > > > > > this is where the whole ViewAndroid hierarchy comes in, the size needs to > be > > > > propagated down the tree, and when a child is added, the size should be > > > > propagated to the child as well > > > > > > Moved the properties to ViewAndroid. > > > > > > I *think* the size of the new child is being updated already somewhere else > > when > > > added. Do you suggest it be done in ViewAndroid::AddChild? > > > > Yes > > Traced the code and found that child nodes are all added before the first > physical backing size change happens - so it has not been necessary to update > the size to that of the parent so far. Do you believe it will necessary to do > that in the future? I don't see why not, in the name of correctness. You can avoid propagation overhead by just returning immediately if nothing actually changed. > I think simply calling child->OnPhysicalBackingSizeChanged() > in AddChild if the parent's size is valid will do the trick. https://codereview.chromium.org/2502763003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2502763003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:245: RenderWidgetHostView* rwhv = !web_contents_->ShowingInterstitialPage() ? On 2016/12/06 07:35:07, Jinsuk Kim wrote: > On 2016/12/06 00:34:14, boliu wrote: > > it's not clear to me why WasResized should only called on the current RWHVA, > but > > not the "background" one, is there a precedence where the old code used to do > > this? > > > > eg is there code that updates the "background" RWHVA after the interstitial is > > dismissed? WebContentsImpl::DidProceedOnInterstitial or DetachInterstitialPage > > doesn't do this it seems > > > > > > on the other hand, shouldn't each RWHVA get its own > OnPhysicalBackingSizeChanged > > implementation, and then can observe size updates? > > I don't have good answers to your questions. You may be right in that each RWHVA > should get PBSC event but that's not what CVC was doing before this CL. It only > updates current RWHVA (either actual content or interstitial). afaict that's a bug in CVCImpl. Update both.
view_android looks much better :) https://codereview.chromium.org/2502763003/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2502763003/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1237: GetViewAndroid()->UpdateLayerBounds(); hmm, why is this still needed? did physical bounds didn't actually change here? https://codereview.chromium.org/2502763003/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1557: view_android->SetLayer(cc::Layer::Create()); is this to avoid null checks in ViewAndroid for updating size? if ViewAndroid has a null layer pointer, then it should just skip updating the layer https://codereview.chromium.org/2502763003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_android.h (left): https://codereview.chromium.org/2502763003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_android.h:186: random line deleted.. https://codereview.chromium.org/2502763003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2502763003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:243: web_contents_->SendScreenRects(); so this code is duplicated from ContentViewCoreImpl::SendScreenRectsAndResizeWidget, right? I think the size change should not need to be forwarded to RWHVA from here. Instead, RWHVA can be ViewClient as well, and listen to size updates. https://codereview.chromium.org/2502763003/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:13: public class EventHandler { should add a comment that the java object can only be used while the native ViewAndroid is alive, which is a super hard guarantee to maintain without checks I think ViewAndroid should hold a ref to this object, and clear the native pointer in ViewAndroid destructor then can assert in methods like onPhysicalBackingSizeChanged that the native pointer is not cleared https://codereview.chromium.org/2502763003/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:697: mEventHandler = new EventHandler(mNativeWindowAndroid); creating EventHandler is unnecessarily duplicated.. is it possible to have ViewAndroid create the java object and return a JavaRef to it? https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.cc:107: child->parent_ = this; should update child size here https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.cc:196: for (auto it = children_.begin(); it != children_.end(); ++it) { style: you can use for-each style "for (auto& child : children_)" https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.cc:217: void ViewAndroid::SetPhysicalBackingSize(int width, int height) { nit: maybe this can just be inlined, a bit confusing to have both this and OnPhysicalBackingSizeChanged https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.cc:235: for (auto it = children_.begin(); it != children_.end(); ++it) ditto for-each loop https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.h:79: bool has_event_handler() { return has_event_handler_; } private https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.h:82: // The |EventHandler| must be unique in the tree to which this ViewAndroid "unique in the tree" is not the phrase here, I don't have a good phrase either, so just skip that part and describe what the condition actually is https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.h:120: int GetPhysicalBackingSizeWidthPix(); remove these two https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_client... ui/android/view_client.h:19: virtual void OnPhysicalBackingSizeChanged(int width, int height); = 0? I don't see an implementation for this
Patchset #7 (id:120001) has been deleted
Couldn't test the binary after a few changes at the final step but all the comments were addressed. Feel free to review it now or later when I verify the apk. https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handler.h File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/60001/ui/android/event_handle... ui/android/event_handler.h:33: int physical_width_pix_; On 2016/12/06 19:13:59, boliu wrote: > On 2016/12/06 07:35:07, Jinsuk Kim wrote: > > On 2016/12/06 00:10:17, boliu wrote: > > > On 2016/12/05 11:07:44, Jinsuk Kim wrote: > > > > On 2016/12/02 23:53:22, boliu wrote: > > > > > this is a property on ViewAndroid, not handler > > > > > > > > > > this is where the whole ViewAndroid hierarchy comes in, the size needs > to > > be > > > > > propagated down the tree, and when a child is added, the size should be > > > > > propagated to the child as well > > > > > > > > Moved the properties to ViewAndroid. > > > > > > > > I *think* the size of the new child is being updated already somewhere > else > > > when > > > > added. Do you suggest it be done in ViewAndroid::AddChild? > > > > > > Yes > > > > Traced the code and found that child nodes are all added before the first > > physical backing size change happens - so it has not been necessary to update > > the size to that of the parent so far. Do you believe it will necessary to do > > that in the future? > > I don't see why not, in the name of correctness. You can avoid propagation > overhead by just returning immediately if nothing actually changed. > > > I think simply calling child->OnPhysicalBackingSizeChanged() > > in AddChild if the parent's size is valid will do the trick. > Done. https://codereview.chromium.org/2502763003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2502763003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:245: RenderWidgetHostView* rwhv = !web_contents_->ShowingInterstitialPage() ? On 2016/12/06 19:13:59, boliu wrote: > On 2016/12/06 07:35:07, Jinsuk Kim wrote: > > On 2016/12/06 00:34:14, boliu wrote: > > > it's not clear to me why WasResized should only called on the current RWHVA, > > but > > > not the "background" one, is there a precedence where the old code used to > do > > > this? > > > > > > eg is there code that updates the "background" RWHVA after the interstitial > is > > > dismissed? WebContentsImpl::DidProceedOnInterstitial or > DetachInterstitialPage > > > doesn't do this it seems > > > > > > > > > on the other hand, shouldn't each RWHVA get its own > > OnPhysicalBackingSizeChanged > > > implementation, and then can observe size updates? > > > > I don't have good answers to your questions. You may be right in that each > RWHVA > > should get PBSC event but that's not what CVC was doing before this CL. It > only > > updates current RWHVA (either actual content or interstitial). > > afaict that's a bug in CVCImpl. Update both. Done. I think the bug is addressed by letting each RWHVA observe physical backing size update as in the updated patch. https://codereview.chromium.org/2502763003/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2502763003/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1237: GetViewAndroid()->UpdateLayerBounds(); On 2016/12/06 23:15:45, boliu wrote: > hmm, why is this still needed? did physical bounds didn't actually change here? Removed. https://codereview.chromium.org/2502763003/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1557: view_android->SetLayer(cc::Layer::Create()); On 2016/12/06 23:15:45, boliu wrote: > is this to avoid null checks in ViewAndroid for updating size? > > if ViewAndroid has a null layer pointer, then it should just skip updating the > layer Yes, also the line now looked like an odd one out after the physical backing size update in the ctr is removed. Wanted to place calls on view android all here. Guarded layer bound update with null check too. https://codereview.chromium.org/2502763003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_android.h (left): https://codereview.chromium.org/2502763003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_android.h:186: On 2016/12/06 23:15:45, boliu wrote: > random line deleted.. Done. https://codereview.chromium.org/2502763003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2502763003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:243: web_contents_->SendScreenRects(); On 2016/12/06 23:15:46, boliu wrote: > so this code is duplicated from > ContentViewCoreImpl::SendScreenRectsAndResizeWidget, right? > > I think the size change should not need to be forwarded to RWHVA from here. > Instead, RWHVA can be ViewClient as well, and listen to size updates. Done. https://codereview.chromium.org/2502763003/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:13: public class EventHandler { On 2016/12/06 23:15:46, boliu wrote: > should add a comment that the java object can only be used while the native > ViewAndroid is alive, which is a super hard guarantee to maintain without checks > > I think ViewAndroid should hold a ref to this object, and clear the native > pointer in ViewAndroid destructor > > then can assert in methods like onPhysicalBackingSizeChanged that the native > pointer is not cleared Done. https://codereview.chromium.org/2502763003/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:697: mEventHandler = new EventHandler(mNativeWindowAndroid); On 2016/12/06 23:15:46, boliu wrote: > creating EventHandler is unnecessarily duplicated.. is it possible to have > ViewAndroid create the java object and return a JavaRef to it? Done. I found that it more convenient to create it in native WindowAndroid. Chrome comes through WA java to the native, and WebView reaches it via AwContents -> ViewAndroid -> WA. Now that VA keeps the weak reference to Java ref, it replaced the prev boolean |has_event_handler_| for checking the presence of handler for each VA. https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.cc:107: child->parent_ = this; On 2016/12/06 23:15:46, boliu wrote: > should update child size here Done. https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.cc:196: for (auto it = children_.begin(); it != children_.end(); ++it) { On 2016/12/06 23:15:46, boliu wrote: > style: you can use for-each style "for (auto& child : children_)" Done. https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.cc:217: void ViewAndroid::SetPhysicalBackingSize(int width, int height) { On 2016/12/06 23:15:46, boliu wrote: > nit: maybe this can just be inlined, a bit confusing to have both this and > OnPhysicalBackingSizeChanged Done. https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.cc:235: for (auto it = children_.begin(); it != children_.end(); ++it) On 2016/12/06 23:15:46, boliu wrote: > ditto for-each loop Done. https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.h:79: bool has_event_handler() { return has_event_handler_; } On 2016/12/06 23:15:46, boliu wrote: > private Removed. https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.h:82: // The |EventHandler| must be unique in the tree to which this ViewAndroid On 2016/12/06 23:15:46, boliu wrote: > "unique in the tree" is not the phrase here, I don't have a good phrase either, > so just skip that part and describe what the condition actually is Done. https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_androi... ui/android/view_android.h:120: int GetPhysicalBackingSizeWidthPix(); On 2016/12/06 23:15:46, boliu wrote: > remove these two Done. https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_client.h File ui/android/view_client.h (right): https://codereview.chromium.org/2502763003/diff/100001/ui/android/view_client... ui/android/view_client.h:19: virtual void OnPhysicalBackingSizeChanged(int width, int height); On 2016/12/06 23:15:46, boliu wrote: > = 0? I don't see an implementation for this Ah forgot to git add view_client.cc... Meant to provide with default implementations of the methods that does nothing or return false.
https://codereview.chromium.org/2502763003/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2502763003/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1572: view_android->SetLayer(cc::Layer::Create()); if this ViewAndroid didn't have a layer, then don't create an extra one, ViewAndroid can have null checks on the layer I think I typed this comment before elsewhere..? https://codereview.chromium.org/2502763003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2502763003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:162: void OnPhysicalBackingSizeChanged(int width, int height); override https://codereview.chromium.org/2502763003/diff/140001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/event_handl... ui/android/event_handler.h:14: void OnDestroyNativeView( ok... this really implies this .h/cc files should just be merged into view_android.h/cc breaks convention, but oh well https://codereview.chromium.org/2502763003/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:19: public EventHandler(long nativeView) { hide the constructor so that only ui code can create instances of this object, ie imo android_webview code is doing an unnecessary layer violation I'd prefer view_android create this from a static Create method. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.cc:74: ViewAndroid::ViewAndroid(ViewClient* client) : parent_(nullptr), fwiw, inline init for member variable with static initial values is encouraged now, so you could put "parent_ = nullptr;" in the header if you want, same with width/height but this is fine too https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.cc:170: return parent_ ? parent_->CreateEventHandler(native_view) this doesn't make any sense to me, why ask the root? EventHandler should be created directly by ViewAndroid, should not involve WindowAndroid at all https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.cc:189: layer_ = layer; need to update layer size if new layer is not null https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:70: virtual base::android::ScopedJavaLocalRef<jobject> GetEventHandler(); shouldn't be virtual https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:121: void SetPhysicalBackingSize(int width, int height) { don't inline this general rule is: inline is allowed for no-op, or simple getter only, ie the body is a single line returning a member variable or a constant https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:127: bool HasEventHandler() { return !event_handler_.is_uninitialized(); } don't inline this https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:132: JavaObjectWeakGlobalRef event_handler_; this can actually be strong, we create EventHandler directly, and can ensure it's not holding onto other objects strongly
jinsukkim@chromium.org changed reviewers: - mdjones@chromium.org
https://codereview.chromium.org/2502763003/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2502763003/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1572: view_android->SetLayer(cc::Layer::Create()); On 2016/12/08 05:01:06, boliu wrote: > if this ViewAndroid didn't have a layer, then don't create an extra one, > ViewAndroid can have null checks on the layer > > I think I typed this comment before elsewhere..? Yes you did, and this was my reply : "Yes, also the line now looked like an odd one out after the physical backing size update in the ctr is removed. Wanted to place calls on view android all here. Guarded layer bound update with null check too." Just to clarify: this is not a new line added but simply moved from ContentViewCoreImpl ctr. It is just called in a few lines sooner. I think I should keep it here. Or move it back? https://codereview.chromium.org/2502763003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2502763003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:162: void OnPhysicalBackingSizeChanged(int width, int height); On 2016/12/08 05:01:06, boliu wrote: > override Done. https://codereview.chromium.org/2502763003/diff/140001/ui/android/event_handl... File ui/android/event_handler.h (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/event_handl... ui/android/event_handler.h:14: void OnDestroyNativeView( On 2016/12/08 05:01:06, boliu wrote: > ok... this really implies this .h/cc files should just be merged into > view_android.h/cc > > breaks convention, but oh well Done. https://codereview.chromium.org/2502763003/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:19: public EventHandler(long nativeView) { On 2016/12/08 05:01:06, boliu wrote: > hide the constructor so that only ui code can create instances of this object, > ie imo android_webview code is doing an unnecessary layer violation > > I'd prefer view_android create this from a static Create method. Done. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.cc:74: ViewAndroid::ViewAndroid(ViewClient* client) : parent_(nullptr), On 2016/12/08 05:01:06, boliu wrote: > fwiw, inline init for member variable with static initial values is encouraged > now, so you could put "parent_ = nullptr;" in the header if you want, same with > width/height > > but this is fine too Acknowledged. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.cc:170: return parent_ ? parent_->CreateEventHandler(native_view) On 2016/12/08 05:01:06, boliu wrote: > this doesn't make any sense to me, why ask the root? EventHandler should be > created directly by ViewAndroid, should not involve WindowAndroid at all This is how I addressed your comment: "creating EventHandler is unnecessarily duplicated.. is it possible to have ViewAndroid create the java object and return a JavaRef to it?" WindowAndroid needs a method creating EventHandler for Chrome, so I'm making use of it for AW too. Though this is going up to WindowAndroid it doesn't mean WindowAndroid will have the handler. The method given the ViewAndroid instance (native_view) which will be associated with it. Are you suggesting I add createEventHandler in ViewAndroidDelegate too? Then both WindowAndroid/ViewAndroidDelegate would have a method creating EventHandler, and I thought it is what you wanted to avoid. See the updated patch. Note that not all ViewAndroid has ViewAndroidDelegate. So the request may go up to parent ViewAndroid to reach Java layer and have the create object returned. So the parameter native_view is still necessary. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.cc:189: layer_ = layer; On 2016/12/08 05:01:06, boliu wrote: > need to update layer size if new layer is not null Done. Null check is performed in |UpdateLayerBounds|. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:70: virtual base::android::ScopedJavaLocalRef<jobject> GetEventHandler(); On 2016/12/08 05:01:06, boliu wrote: > shouldn't be virtual Done. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:121: void SetPhysicalBackingSize(int width, int height) { On 2016/12/08 05:01:06, boliu wrote: > don't inline this > > general rule is: inline is allowed for no-op, or simple getter only, ie the body > is a single line returning a member variable or a constant You asked me to :/ 217 void ViewAndroid::SetPhysicalBackingSize(int width, int height) { boliu 2016/12/06 23:15:46 nit: maybe this can just be inlined, a bit confusing to have both this and OnPhysicalBackingSizeChanged https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:127: bool HasEventHandler() { return !event_handler_.is_uninitialized(); } On 2016/12/08 05:01:06, boliu wrote: > don't inline this Done. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:132: JavaObjectWeakGlobalRef event_handler_; On 2016/12/08 05:01:06, boliu wrote: > this can actually be strong, we create EventHandler directly, and can ensure > it's not holding onto other objects strongly Done. Now event handler is created directly in view_android.
https://codereview.chromium.org/2502763003/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2502763003/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.cc:1572: view_android->SetLayer(cc::Layer::Create()); On 2016/12/13 23:20:38, Jinsuk Kim wrote: > On 2016/12/08 05:01:06, boliu wrote: > > if this ViewAndroid didn't have a layer, then don't create an extra one, > > ViewAndroid can have null checks on the layer > > > > I think I typed this comment before elsewhere..? > > Yes you did, and this was my reply : "Yes, also the line now looked like an odd > one out after the physical backing > size update in the ctr is removed. Wanted to place calls on view android all > here. > > Guarded layer bound update with null check too." > > Just to clarify: this is not a new line added but simply moved from > ContentViewCoreImpl ctr. It is just called in a few lines sooner. I think I > should keep it here. Or move it back? Ohh, didn't notice it's moved. That's fine then https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.cc:170: return parent_ ? parent_->CreateEventHandler(native_view) On 2016/12/13 23:20:38, Jinsuk Kim wrote: > On 2016/12/08 05:01:06, boliu wrote: > > this doesn't make any sense to me, why ask the root? EventHandler should be > > created directly by ViewAndroid, should not involve WindowAndroid at all > > This is how I addressed your comment: "creating EventHandler is unnecessarily > duplicated.. is it possible to > have ViewAndroid create the java object and return a JavaRef to it?" > WindowAndroid needs a method creating EventHandler for Chrome, so I'm making use > of it for AW too. Though this is going up to WindowAndroid it doesn't mean > WindowAndroid will have the handler. The method given the ViewAndroid instance > (native_view) which will be associated with it. > > Are you suggesting I add createEventHandler in ViewAndroidDelegate too? Then > both WindowAndroid/ViewAndroidDelegate would have a method creating > EventHandler, and I thought it is what you wanted to avoid. See the updated > patch. > > Note that not all ViewAndroid has ViewAndroidDelegate. So the request may go up > to parent ViewAndroid to reach Java layer and have the create object returned. > So the parameter native_view is still necessary. No, I'm suggesting that you mark EventHandler.create @CalledFromNative, and just call Java_EventHandler_create from here directly. WindowAndroid doesn't need to be involved at all. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:121: void SetPhysicalBackingSize(int width, int height) { On 2016/12/13 23:20:38, Jinsuk Kim wrote: > On 2016/12/08 05:01:06, boliu wrote: > > don't inline this > > > > general rule is: inline is allowed for no-op, or simple getter only, ie the > body > > is a single line returning a member variable or a constant > > You asked me to :/ > > 217 void ViewAndroid::SetPhysicalBackingSize(int width, int height) { > boliu 2016/12/06 23:15:46 > nit: maybe this can just be inlined, a bit confusing to have both this and > OnPhysicalBackingSizeChanged Ohh... overloading the word "inline", I meant don't define this method at all, and just move the body to where it's called, since there's only one call site.
https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.cc:170: return parent_ ? parent_->CreateEventHandler(native_view) > No, I'm suggesting that you mark EventHandler.create @CalledFromNative, and just > call Java_EventHandler_create from here directly. > > WindowAndroid doesn't need to be involved at all. Ah much simpler this way. The parameter native_view can go away too. Done. https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/140001/ui/android/view_androi... ui/android/view_android.h:121: void SetPhysicalBackingSize(int width, int height) { On 2016/12/13 23:50:41, boliu wrote: > On 2016/12/13 23:20:38, Jinsuk Kim wrote: > > On 2016/12/08 05:01:06, boliu wrote: > > > don't inline this > > > > > > general rule is: inline is allowed for no-op, or simple getter only, ie the > > body > > > is a single line returning a member variable or a constant > > > > You asked me to :/ > > > > 217 void ViewAndroid::SetPhysicalBackingSize(int width, int height) { > > boliu 2016/12/06 23:15:46 > > nit: maybe this can just be inlined, a bit confusing to have both this and > > OnPhysicalBackingSizeChanged > > Ohh... overloading the word "inline", I meant don't define this method at all, > and just move the body to where it's called, since there's only one call site. I see. Should have made a better guess :) Done.
https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_androi... ui/android/view_android.cc:273: Java_EventHandler_onDestroyNativeView(env, event_handler); this is a single line function with a single call site remove the function and directly call the jni thing in that one call site? https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_androi... ui/android/view_android.h:118: virtual base::android::ScopedJavaLocalRef<jobject> CreateEventHandler(); doesn't need to be virtual anymore https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_androi... ui/android/view_android.h:134: void OnDestroyNativeView( hmm?
https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_androi... ui/android/view_android.cc:273: Java_EventHandler_onDestroyNativeView(env, event_handler); On 2016/12/14 00:58:11, boliu wrote: > this is a single line function with a single call site > > remove the function and directly call the jni thing in that one call site? Done. https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_androi... ui/android/view_android.h:118: virtual base::android::ScopedJavaLocalRef<jobject> CreateEventHandler(); On 2016/12/14 00:58:12, boliu wrote: > doesn't need to be virtual anymore Done. https://codereview.chromium.org/2502763003/diff/180001/ui/android/view_androi... ui/android/view_android.h:134: void OnDestroyNativeView( On 2016/12/14 00:58:12, boliu wrote: > hmm? .... removed.
https://codereview.chromium.org/2502763003/diff/200001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/2502763003/diff/200001/ui/android/window_andr... ui/android/window_android.h:77: base::android::ScopedJavaLocalRef<jobject> GetEventHandler(JNIEnv* env, nit: this is function overloading, which is banned, even if it's accidental.. call this something else? GetOrCreateEventHandler? GetEventHandlerForJava? https://codereview.chromium.org/2502763003/diff/200001/ui/android/window_andr... ui/android/window_android.h:94: using ViewAndroid::GetEventHandler; this should not be required I think? a function that can't be resolved in the child class should automatically call the parent one?
https://codereview.chromium.org/2502763003/diff/200001/ui/android/window_andr... File ui/android/window_android.h (right): https://codereview.chromium.org/2502763003/diff/200001/ui/android/window_andr... ui/android/window_android.h:77: base::android::ScopedJavaLocalRef<jobject> GetEventHandler(JNIEnv* env, On 2016/12/14 01:26:42, boliu wrote: > nit: this is function overloading, which is banned, even if it's accidental.. > call this something else? GetOrCreateEventHandler? GetEventHandlerForJava? Done. Chose the latter. https://codereview.chromium.org/2502763003/diff/200001/ui/android/window_andr... ui/android/window_android.h:94: using ViewAndroid::GetEventHandler; On 2016/12/14 01:26:42, boliu wrote: > this should not be required I think? a function that can't be resolved in the > child class should automatically call the parent one? GetEventHandler above was hiding the overloaded one in the parent. Now that it was renamed, this is not necessary any more. Removed. http://stackoverflow.com/questions/15556535/calling-base-classs-overloaded-me...
lgtm
Description was changed from ========== Send OnPhysicalBackingSizeChanged event down through EventHandler Defined a method to forward the Java view event down to native through a new interface EventHandler replacing the flow through ContentViewCore. |OnPhysicalBackingSizedChanged| is used for an example. BUG=598880 ========== to ========== Introduce EventHandler forwarding input/view events to native This CL introduces a new interface EventHandler to forward Java view/input events down to native, replacing the flow through ContentViewCore. EventHandler, 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. For most of the embedders the interface can be obtained from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets EventHandler from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=598880 ==========
jinsukkim@chromium.org changed reviewers: + khushalsagar@chromium.org, tedchoc@chromium.org
khushalsagar@chromium.org: Please review changes in blimp/ tedchoc@chromium.org: Please review changes in chrome/, ui/ Please note that ui/ was also reviewed by boliu@ who needed to look into it in conjunction with the changes in content/.
jinsukkim@chromium.org changed reviewers: + bshe@chromium.org
bshe@chromium.org: Please review changes in vr_shell
https://codereview.chromium.org/2502763003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java (right): https://codereview.chromium.org/2502763003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:75: getEventHandler().onPhysicalBackingSizeChanged(width, height); On 2016/12/06 07:35:07, Jinsuk Kim wrote: > On 2016/12/06 00:34:14, boliu wrote: > > looks like this class is used only by chromecast, and it's supposed to be some > > global thing, so directing this to the window is fine > > > > odd it's in content, feels like it should be in chromecast instead > > Ack. I'll see about moving it out of content in a follow up CL Found that this class is also used by content shell.
On 2016/12/14 02:06:40, Jinsuk Kim wrote: > https://codereview.chromium.org/2502763003/diff/80001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java > (right): > > https://codereview.chromium.org/2502763003/diff/80001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:75: > getEventHandler().onPhysicalBackingSizeChanged(width, height); > On 2016/12/06 07:35:07, Jinsuk Kim wrote: > > On 2016/12/06 00:34:14, boliu wrote: > > > looks like this class is used only by chromecast, and it's supposed to be > some > > > global thing, so directing this to the window is fine > > > > > > odd it's in content, feels like it should be in chromecast instead > > > > Ack. I'll see about moving it out of content in a follow up CL > > Found that this class is also used by content shell. vr shell lgtm
Description was changed from ========== Introduce EventHandler forwarding input/view events to native This CL introduces a new interface EventHandler to forward Java view/input events down to native, replacing the flow through ContentViewCore. EventHandler, 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. For most of the embedders the interface can be obtained from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets EventHandler from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=598880 ========== to ========== Introduce EventHandler forwarding input/view events to native This CL introduces a new interface EventHandler to forward Java view/input events down to native, replacing the flow through ContentViewCore. EventHandler, 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. For most of the embedders the interface can be obtained from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets EventHandler from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 ==========
ted, dtrainor and I talked today about how to structure this. We arrived at a slightly different design to accommodate webview: * Introduce a "ViewRoot" class. It's a subclass of ViewAndroid, acting as the root root of a ViewAndroid tree. It will receive events like touch events (for both chrome and webview). And it will have a java peer. * WindowAndroid will no longer inherit from ViewAndroid * There is a 1-to-many relationship between WindowAndroid and ViewRoot (to accommodate webview) * chrome/ and android_webview/ code is responsible for passing in a ViewRoot when creating a new WebContents. Please ask if you have any questions. Good news this CL is mostly a step towards the new design already. Just rename EventHandler to ViewRoot in this CL and we can just land it as is. And can get to the rest of the refactor in follow ups.
lgtm based on boliu@'s previous comment https://codereview.chromium.org/2502763003/diff/220001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2502763003/diff/220001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:14: public class EventHandler { from the discussion yesterday, I think EventDisptacher or EventRouter is probably better than Handler. Handler to me implies it does something and this is just an intermediary. And talking with boliu@, I think EventForwarder is potentially a better alternative. I then struggled with the concept that this class should exist only once for each conceptual root of a view tree (WindowAndroid on Chrome, the child ViewAndroid's in WebView). While I think the need for this discrepancy is unfortunate, this certainly predates your change is as orthogonal as you can get. To make it even clearer that this should exist only at the "root" of these views, I actually propose we change the name to ViewRootEventForwarder. Then it should be clear that you can't get one of these for each view and start sending events (which is what I "thought" you could do without reading this further/talking to Bo). And talking further with dtrainor@ and boliu@, we think the name of this should just be ViewRoot. https://codereview.chromium.org/2502763003/diff/220001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:28: public void onPhysicalBackingSizeChanged(int width, int height) { can you add javadoc here...physical backing size isn't something that is immediately obvious to those not in the know https://codereview.chromium.org/2502763003/diff/220001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/220001/ui/android/view_androi... ui/android/view_android.cc:246: void ViewAndroid::OnPhysicalBackingSizeChanged(int width, int height) { does physical backing size make sense on anything but the root? isn't it the discrepancy between the texture allocation size and the draw region (to compensate for showing the keyboard and not resizing the compositor). I don't know enough of the state of the compositor to understand whether this is an implementation detail or whether there will be multiple sizes associated with each view.
jinsukkim@chromium.org changed reviewers: + dtrainor@chromium.org - khushalsagar@chromium.org
dtrainor@ please review changes in blimp/ https://codereview.chromium.org/2502763003/diff/220001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/EventHandler.java (right): https://codereview.chromium.org/2502763003/diff/220001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:14: public class EventHandler { On 2016/12/15 23:36:44, Ted C wrote: > from the discussion yesterday, I think EventDisptacher or EventRouter is > probably better than Handler. Handler to me implies it does something and this > is just an intermediary. > > And talking with boliu@, I think EventForwarder is potentially a better > alternative. > > I then struggled with the concept that this class should exist only once for > each conceptual root of a view tree (WindowAndroid on Chrome, the child > ViewAndroid's in WebView). While I think the need for this discrepancy is > unfortunate, this certainly predates your change is as orthogonal as you can > get. > > To make it even clearer that this should exist only at the "root" of these > views, I actually propose we change the name to ViewRootEventForwarder. Then it > should be clear that you can't get one of these for each view and start sending > events (which is what I "thought" you could do without reading this > further/talking to Bo). > > And talking further with dtrainor@ and boliu@, we think the name of this should > just be ViewRoot. Thanks for the discussion for better naming. Done. https://codereview.chromium.org/2502763003/diff/220001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/EventHandler.java:28: public void onPhysicalBackingSizeChanged(int width, int height) { On 2016/12/15 23:36:44, Ted C wrote: > can you add javadoc here...physical backing size isn't something that is > immediately obvious to those not in the know Done. https://codereview.chromium.org/2502763003/diff/220001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/220001/ui/android/view_androi... ui/android/view_android.cc:246: void ViewAndroid::OnPhysicalBackingSizeChanged(int width, int height) { On 2016/12/15 23:36:44, Ted C wrote: > does physical backing size make sense on anything but the root? isn't it the > discrepancy between the texture allocation size and the draw region (to > compensate for showing the keyboard and not resizing the compositor). > > I don't know enough of the state of the compositor to understand whether this is > an implementation detail or whether there will be multiple sizes associated with > each view. In early rounds of patches the size was stored at the root only, and the lower views went up to get it. Later it was changed to the comments suggesting that they be duplicated across the views in the hierarchy since they all share the size. Maybe physical backing size is not the best example I could use for this CL. In most cases the passed information will not be duped or stored in view_android but will be dispatched to a ViewClient impl that wants to handle it.
Description was changed from ========== Introduce EventHandler forwarding input/view events to native This CL introduces a new interface EventHandler to forward Java view/input events down to native, replacing the flow through ContentViewCore. EventHandler, 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. For most of the embedders the interface can be obtained from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets EventHandler from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 ========== to ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 ==========
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
https://codereview.chromium.org/2502763003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java (left): https://codereview.chromium.org/2502763003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:233: nativeContentBoundsChanged(mNativeVrShell, surfaceWidth, surfaceHeight, dpr); did you intend to remove this line? I'm pretty sure this breaks VR rendering.
blimp/ lgtm. Based on Bo's comment to just rename and fix later, this is fine, but for the refactor follow up, I think we want a native class called ViewRootAndroid that extends ViewAndroid. That way all the JNI is isolated there. We can then also make the event calls protected on ViewAndroid, which means only ViewRootAndroid can call them (probably from the JNI bridge), which restricts it like Bo wants.
On 2016/12/16 19:55:26, David Trainor wrote: > blimp/ lgtm. > > Based on Bo's comment to just rename and fix later, this is fine, but for the > refactor follow up, I think we want a native class called ViewRootAndroid that > extends ViewAndroid. That way all the JNI is isolated there. We can then also > make the event calls protected on ViewAndroid, which means only ViewRootAndroid > can call them (probably from the JNI bridge), which restricts it like Bo wants. Also, WindowAndroid then no longer extends from ViewAndroid I believe was the agreement in the discussion. WindowAndroid holds a list of ViewRoot's to compensate for the WebView case.
On 2016/12/16 20:40:40, Ted C wrote: > On 2016/12/16 19:55:26, David Trainor wrote: > > blimp/ lgtm. > > > > Based on Bo's comment to just rename and fix later, this is fine, but for the > > refactor follow up, I think we want a native class called ViewRootAndroid that > > extends ViewAndroid. That way all the JNI is isolated there. We can then > also > > make the event calls protected on ViewAndroid, which means only > ViewRootAndroid > > can call them (probably from the JNI bridge), which restricts it like Bo > wants. > > Also, WindowAndroid then no longer extends from ViewAndroid I believe was > the agreement in the discussion. WindowAndroid holds a list of ViewRoot's > to compensate for the WebView case. I did say all of that above in https://codereview.chromium.org/2502763003/#msg45 :p
> Also, WindowAndroid then no longer extends from ViewAndroid I believe was > the agreement in the discussion. WindowAndroid holds a list of ViewRoot's > to compensate for the WebView case. I like the overall direction of the change. Thanks for having had discussion on this. One question - I suppose embedders no longer need to get ViewRoot from WindowAndroid, from what boliu@ says '[embedders are] responsible for passing in a ViewRoot when creating a new WebContents." which indicates they already hold a reference to ViewRoot. Does WindowAndroid still need to hold a list of ViewRoot's and maintain it?
On 2016/12/18 23:54:17, Jinsuk Kim wrote: > > Also, WindowAndroid then no longer extends from ViewAndroid I believe was > > the agreement in the discussion. WindowAndroid holds a list of ViewRoot's > > to compensate for the WebView case. > > I like the overall direction of the change. Thanks for having had > discussion on this. > > One question - I suppose embedders no longer need to get ViewRoot > from WindowAndroid, from what boliu@ says '[embedders are] > responsible for passing in a ViewRoot when creating a new WebContents." > which indicates they already hold a reference to ViewRoot. > Does WindowAndroid still need to hold a list of ViewRoot's and > maintain it? I don't think so, though it might be convenient.
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...)
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...
Patchset #13 (id:260001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #13 (id:280001) has been deleted
Left a TODO in ViewRoot.java for follow-up refactoring. https://codereview.chromium.org/2502763003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java (left): https://codereview.chromium.org/2502763003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:233: nativeContentBoundsChanged(mNativeVrShell, surfaceWidth, surfaceHeight, dpr); On 2016/12/16 17:54:22, mthiesse wrote: > did you intend to remove this line? I'm pretty sure this breaks VR rendering. Thanks for catching it. Put it back. https://codereview.chromium.org/2502763003/diff/300001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2502763003/diff/300001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1208: mViewRoot = null; Added this line to fix a bug not cleaning up ViewRoot used for prev AwContents. Left uncleared, ViewRoot's native view (ViewAndroid) got already destroyed at this point, which makes the object invalid. New ViewRoot should be created for the new AwContents. https://codereview.chromium.org/2502763003/diff/300001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/300001/ui/android/view_androi... ui/android/view_android.cc:198: if (!HasViewRoot()) { HasViewRoot() makes this method simpler.
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.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org, boliu@chromium.org, tedchoc@chromium.org, bshe@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2502763003/#ps300001 (title: "rebased/fixed tests")
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 jinsukkim@chromium.org
https://codereview.chromium.org/2502763003/diff/300001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2502763003/diff/300001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:1208: mViewRoot = null; On 2016/12/19 04:40:58, Jinsuk Kim wrote: > Added this line to fix a bug not cleaning up ViewRoot used for prev AwContents. > Left uncleared, ViewRoot's native view (ViewAndroid) got already destroyed at > this point, which makes the object invalid. New ViewRoot should be created for > the new AwContents. yep that's fine https://codereview.chromium.org/2502763003/diff/300001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/300001/ui/android/view_androi... ui/android/view_android.cc:198: if (!HasViewRoot()) { On 2016/12/19 04:40:58, Jinsuk Kim wrote: > HasViewRoot() makes this method simpler. simpler appears to be wrong though.. this is not the same check as before HasViewRoot checks whether the reference itself is null the previous check checks whether the referent is null (whether it's never set or already garbage collected) Also in this code, there is no longer any strong reference between CreateViewRoot and the return statement, meaning if gc kicks in at the exact right time, then (in theory) the newly created ViewRoot can be immediately collected https://codereview.chromium.org/2502763003/diff/300001/ui/android/view_androi... ui/android/view_android.cc:200: event_handler_ = JavaObjectWeakGlobalRef(env, CreateViewRoot()); rename the var name view_root_?
https://codereview.chromium.org/2502763003/diff/300001/ui/android/view_androi... File ui/android/view_android.cc (right): https://codereview.chromium.org/2502763003/diff/300001/ui/android/view_androi... ui/android/view_android.cc:198: if (!HasViewRoot()) { On 2016/12/19 17:33:09, boliu wrote: > On 2016/12/19 04:40:58, Jinsuk Kim wrote: > > HasViewRoot() makes this method simpler. > > simpler appears to be wrong though.. > > this is not the same check as before > HasViewRoot checks whether the reference itself is null > the previous check checks whether the referent is null (whether it's never set > or already garbage collected) > > Also in this code, there is no longer any strong reference between > CreateViewRoot and the return statement, meaning if gc kicks in at the exact > right time, then (in theory) the newly created ViewRoot can be immediately > collected Hm reverted. https://codereview.chromium.org/2502763003/diff/300001/ui/android/view_androi... ui/android/view_android.cc:200: event_handler_ = JavaObjectWeakGlobalRef(env, CreateViewRoot()); On 2016/12/19 17:33:09, boliu wrote: > rename the var name view_root_? Done.
you have all the approvals now, right? you can land it now
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.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, boliu@chromium.org, tedchoc@chromium.org, mdjones@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2502763003/#ps320001 (title: "prevent view_root_ gc'ed")
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": 320001, "attempt_start_ts": 1482193672889270,
"parent_rev": "49a5e1b103f64b8d30aeb9b289acf1f970102930", "commit_rev":
"ca215affaa039ae38aa43194309cfe3a1a18c95c"}
Message was sent while issue was closed.
Description was changed from ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 ========== to ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 Review-Url: https://codereview.chromium.org/2502763003 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 Review-Url: https://codereview.chromium.org/2502763003 ========== to ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 Committed: https://crrev.com/603de249d822522b152b29184be793550695f2c8 Cr-Commit-Position: refs/heads/master@{#439636} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/603de249d822522b152b29184be793550695f2c8 Cr-Commit-Position: refs/heads/master@{#439636}
Message was sent while issue was closed.
Description was changed from ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 Committed: https://crrev.com/603de249d822522b152b29184be793550695f2c8 Cr-Commit-Position: refs/heads/master@{#439636} ========== to ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 TBR=boliu@chromium.org,bshe@chromium.org,tedchoc@chromium.org,dtrainor@chromi... Committed: https://crrev.com/603de249d822522b152b29184be793550695f2c8 Cr-Commit-Position: refs/heads/master@{#439636} ==========
Message was sent while issue was closed.
Description was changed from ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 TBR=boliu@chromium.org,bshe@chromium.org,tedchoc@chromium.org,dtrainor@chromi... Committed: https://crrev.com/603de249d822522b152b29184be793550695f2c8 Cr-Commit-Position: refs/heads/master@{#439636} ========== to ========== Introduce ViewRoot forwarding input/view events to native This CL introduces a new interface ViewRoot to forward Java view/input events down to native, replacing the flow through ContentViewCore. 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. Embedders can obtain the interface from WindowAndroid whose native part is the root of the ViewAndroid hierarchy. WebView is an exception where all activities share the WA instance, hence cannot tell which path in the hierarchy to dispatch the event to. For this reason WebView gets ViewRoot from the top ViewAndroid (the one right below WA) instead. |OnPhysicalBackingSizedChanged| is used for an example. BUG=671401 Committed: https://crrev.com/603de249d822522b152b29184be793550695f2c8 Cr-Commit-Position: refs/heads/master@{#439636} ========== |
