New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 622847 link

Starred by 8 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocked on:
issue 628302
issue 671401
issue 704323

Blocking:
issue 622864
issue 598880



Sign in to add a comment

Android: Move motion event, resize, visibility forwarding from ContentViewCore to ViewAndroid

Project Member Reported by siev...@chromium.org, Jun 23 2016

Issue description

This tracks simply moving existing code in a way so that what ContentViewCoreImpl implements here will move to its base class (ui::ViewAndroid) which further forwards it to a Delegate.

For Chrome WebContents the Delegate is RenderWidgetHostViewAndroid.
For Blimp the Delegate could be something else with reusal of ViewAndroid.

Also this implies that these events will be forwarded from the embedding View to ViewAndroid directly, bypassing ContentViewCore.
 
Blocking: 598880
Blocking: 622848
Blocking: 622864

Comment 4 by hush@chromium.org, Jul 11 2016

Cc: hush@chromium.org
yusufo@ might look at routing size through the ViewAndroid since decoupling the Java container View's size from the 'requested size' with an abstraction layer would allow for fixing prerender.

dtrainor@ said he might look at input events.

Comment 6 by yus...@chromium.org, Jul 14 2016

Blockedon: 628302

Comment 7 by sgu...@chromium.org, Jul 14 2016

Cc: sgu...@chromium.org
Cc: bshe@chromium.org
Cc: boliu@chromium.org
Blocking: -622848
I just want to note that while this isn't useful for Blimp reasons anymore, VR would still really like at least resize forwarding to ViewAndroid so that we can override it and ignore Android view sizing events.

I may pick parts of this up myself in January if you don't plan to do it any time soon Bo.

Comment 12 by boliu@chromium.org, Dec 16 2016

we are still doing this, first CL: https://codereview.chromium.org/2502763003/
Blockedon: 671401
Project Member

Comment 14 by bugdroid1@chromium.org, May 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb293a5d27d9f233a801653e4983bef0a92e43e9

commit fb293a5d27d9f233a801653e4983bef0a92e43e9
Author: jinsukkim <jinsukkim@chromium.org>
Date: Fri May 12 05:30:42 2017

Store physical backing size in ViewAndroid

Stores physical backing size info in ViewAndroid tree nodes.
It rids CVC of methods related to physical backing size,
and help eliminate the access from RWHVA to CVC done against
layering hierarchy.

BUG= 626765 ,  622847 

Review-Url: https://codereview.chromium.org/2842693005
Cr-Commit-Position: refs/heads/master@{#471227}

[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/android_webview/browser/aw_contents.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutRenderHost.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/CompositorVisibilityTest.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/MockLayoutHost.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/browser/android/bottombar/overlay_panel_content.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/browser/android/bottombar/overlay_panel_content.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/browser/android/compositor/compositor_view.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/browser/android/compositor/compositor_view.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/browser/android/tab_android.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/browser/android/vr_shell/vr_shell.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/chrome/browser/android/vr_shell/vr_shell.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/browser/android/content_view_render_view.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/browser/android/content_view_render_view.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/ui/android/view_android.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/ui/android/view_android.h
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/ui/android/view_client.cc
[modify] https://crrev.com/fb293a5d27d9f233a801653e4983bef0a92e43e9/ui/android/view_client.h

Blockedon: 704323
Project Member

Comment 16 by bugdroid1@chromium.org, May 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da11b4dc274864786ae57bb4c006b9adfdbefb4e

commit da11b4dc274864786ae57bb4c006b9adfdbefb4e
Author: jinsukkim <jinsukkim@chromium.org>
Date: Fri May 26 06:44:41 2017

Route OnDragEvent through ViewAndroid tree

Moves another event handling flow from CVC to ViewAndroid:

- Introduced a new native class for drag event (DragEventAndroid)
- Updated ViewAndroid to handle both drag/motion events
  in its hit testing logic.
- Clarify that layout in VA is in CSS unit, which was not
  clear and prone to error.

Tested manually with target sdk set to 25.

BUG= 622847 

Review-Url: https://codereview.chromium.org/2896993002
Cr-Commit-Position: refs/heads/master@{#474947}

[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/content/public/android/java/src/org/chromium/content/browser/ContentView.java
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/android/DEPS
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/android/event_forwarder.cc
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/android/event_forwarder.h
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/android/java/src/org/chromium/ui/base/EventForwarder.java
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/android/view_android.cc
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/android/view_android.h
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/android/view_client.cc
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/android/view_client.h
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/events/BUILD.gn
[add] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/events/android/drag_event_android.cc
[add] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/events/android/drag_event_android.h
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/events/android/motion_event_android.cc
[modify] https://crrev.com/da11b4dc274864786ae57bb4c006b9adfdbefb4e/ui/events/android/motion_event_android.h

Project Member

Comment 17 by bugdroid1@chromium.org, Jun 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37ae4cefaf29db963187790e199ddd7b663f0001

commit 37ae4cefaf29db963187790e199ddd7b663f0001
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Fri Jun 16 06:19:53 2017

Forward hover event to EventForwarder

This CL forwards hover event to native through EventForwarder
like other kind of input events. Hover event needs additional
care in order to allow WebContentsAccessibility to intercept
and consume it in accessibility mode instead of sending it down
the default flow to RenderWidgetHostViewAndroid. This is done
through WebContentsAndroid back to Java layer when necessary.

BUG= 622847 

Change-Id: I1c9a501fb7c62f68a5a84b8aa6788a0f6ec3ea28
Reviewed-on: https://chromium-review.googlesource.com/517683
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479966}
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/browser/accessibility/browser_accessibility_manager_android.cc
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/browser/accessibility/browser_accessibility_manager_android.h
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/browser/accessibility/web_contents_accessibility_android.cc
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/browser/accessibility/web_contents_accessibility_android.h
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/public/android/java/src/org/chromium/content/browser/ContentView.java
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/content/public/android/java/src/org/chromium/content/browser/accessibility/WebContentsAccessibility.java
[modify] https://crrev.com/37ae4cefaf29db963187790e199ddd7b663f0001/ui/android/java/src/org/chromium/ui/base/EventForwarder.java

Project Member

Comment 18 by bugdroid1@chromium.org, Sep 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77

commit 86071c86ba17eb7761cd0663bc95ceaaf3c4fb77
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Thu Sep 07 00:49:05 2017

Store view size info ViewAndroid 

As a part of effort handling OnSizeChange event in ViewAndroid,
this CL defines variables for size information and |OnSizeChanged|
interface in ViewAndroid. The changes is not yet in actual use,
but will replace current logic going through ContentView
ContentViewCore later.

The overall change is in https://crrev.com/c/588029

BUG= 622847 

Change-Id: Ie99604a9f9c5109463229e7784829b1616cf9a14
Reviewed-on: https://chromium-review.googlesource.com/634847
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500162}
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/android_webview/browser/aw_contents.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/chrome/browser/android/bottombar/overlay_panel_content.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/chrome/browser/android/vr_shell/vr_shell.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/content/browser/android/content_view_core.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/content/browser/web_contents/web_contents_android.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/content/browser/web_contents/web_contents_android.h
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/ui/android/view_android.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/ui/android/view_android.h
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/ui/android/view_android_unittests.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/ui/android/view_client.cc
[modify] https://crrev.com/86071c86ba17eb7761cd0663bc95ceaaf3c4fb77/ui/android/view_client.h

Project Member

Comment 19 by bugdroid1@chromium.org, Sep 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d04c9ed0f8392fed5341fff32fa5eac1fa20a7b

commit 7d04c9ed0f8392fed5341fff32fa5eac1fa20a7b
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Wed Sep 13 00:22:18 2017

Set ViewAndroid layout properly

In ViewAndroid tree, the top view for WebContents should be of
normal layout that can adjust its size to the OnSizeChanged
event, while the bottom one for RenderWidgetHostViewAndroid
should be of type match-parent to keep its size always in sync with
its parent. This CL takes care of that.

BUG= 622847 

Change-Id: Ie928a68334fc8c7342643081d85cdb88da075bb4
Reviewed-on: https://chromium-review.googlesource.com/654397
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501474}
[modify] https://crrev.com/7d04c9ed0f8392fed5341fff32fa5eac1fa20a7b/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/7d04c9ed0f8392fed5341fff32fa5eac1fa20a7b/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/7d04c9ed0f8392fed5341fff32fa5eac1fa20a7b/ui/android/view_android.cc
[modify] https://crrev.com/7d04c9ed0f8392fed5341fff32fa5eac1fa20a7b/ui/android/view_android.h
[modify] https://crrev.com/7d04c9ed0f8392fed5341fff32fa5eac1fa20a7b/ui/android/view_android_unittests.cc

Cc: -sgu...@chromium.org
Owner: jinsuk...@chromium.org
Status: Assigned (was: Available)
Quoted from https://crrev.com/666598:

---- boliu@
I feel the concept of browser controls shouldn't exist in content or below. All that's required for content should be to pass up the offsets from the renderer frame. Then embedder (ie chrome/) can do whatever it likes with that offset.

I imagine browser controls can just be abstracted as differences between physical size and view size?

otoh, I don't know browser controls very well, maybe that doesn't work, in which case you can let me know which specific cases don't work

------- jinsukkim@

The top control height info is generated by embedder, and propagated down to compositor layer https://codereview.chromium.org/714003002 I think it will take substantial effort to take the concept out of content layer. 

From other perspective, I take you're probably meaning that top control height is just another way to represent top offset, whether the top part is occupied by a control or whatever. In any way, it shall be a task of its own. Cc'ed dtrainor who wrote the code to get his thought. David, let us help figure out where we can go from here.

------ dtrainor@

So yeah I think there are two things we want (and it seems like you're both talking about one in particular).

1. We push two things to the renderer from the browser process in Chrome:
  - The top controls height (so the renderer understands how to adjust each frame based off of renderer-handled scrolling, etc).  This goes through the typical RESIZE message in RWHVA.
  - Whether or not top controls are allowed to be hidden, shown, or both.  This apparently bypasses the world and IPC's directly from tab_android.
2. We get data back about the controls offset for the current frame, which may or may not match what we sent as the controls move on/off the screen, respond to scrolls, etc..

We used to call this "top control offset" but "generalized" it to "browser controls offset" to handle having the toolbar on the top or bottom.

IMO, as Bo suggests, we can probably generalize this a bit more.  e.g. a set of insets around each edge of the content and rules on which edges can show, shrink, or both.  If we don't want to build all four sides, we can just call it top and bottom inset?

I took a brief look today, and found that the concept of browser controls (visibility/size) goes down to Blink https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/BrowserControls.h used in WebFrame, VisualViewport, etc., which makes me think it had a good justification for having been designed in this way. A patch like r428054 shows the scope of the browser controls used in the codebase. Are we really sure that it shouldn't be in content and below?

Cc: bokan@chromium.org
Personally I don't see any obvious way to refactor browser controls to be simpler than they are today.  It's an inherently complex feature and the code is as complex as it needs to be.  Non-browser-controls viewport size is used in some places in Blink to suppress viewport size, for instance.  And top and bottom controls have different reactions to scrolling.  They have so many nuanced behaviors that they wouldn't fit naturally into a generic inset abstraction.  I also feel that adding unnecessary support for x-coordinate insets would be harmful because width is treated very asymmetrically from height in Blink layout and we want to take advantage of the height special case.

Comment 24 by bokan@chromium.org, Oct 12 2017

To add to #23, a few points:

- The browser controls height is needed inside Blink because we adjust layout height based on it. We don't want to resize layout based on browser controls hiding so if they're hidden we shrink the layout height by the browser controls height. You could pass that along with the inset but that breaks the simple abstraction and ends up with essentially what we have today.

- We can't just pass the scroll offsets up from the render frame because scrolling occurs inside Blink/CC. Top controls hide without scrolling the view so when the user moves their finger we need to know -- in Blink/CC -- whether to consume the movement by scrolling the root layer or moving the top controls. If we passed the scrolled offset up and then had to move the top controls, we'd somehow have to "undo" the scroll in the root layer from the browser. This might work more naturally with bottom controls though since they always scroll and show/hide.

- For the same reason as above, Blink/CC needs to know about the allowed states of the URL bar.

- position: fixed bottom Elements need to be offseted as the URL bar hides/shows since Blink won't get the resize until after the URL bar is fully moved (user lifts finger). This must happen in the renderer today since we send over rastered frames, the Elements need to be drawn in their correct locations. If you waited for a round trip of the inset change (renderer sends scroll offset - browser sends back inset adjustment) pos: fixed Elements would be out of sync with browser controls or you'd need some kind of synchronization logic. It might be possible to implement in the Viz process once that work is completed (when we ship DisplayList-type objects out of the renderer rather than rastered frames)

IMHO, it'd be difficult to actually remove any functionality from content so I think using the inset analogy would be a cosmetic change.

Comment 25 by boliu@chromium.org, Oct 12 2017

Spent an hour digging into code.. Trying to summarize ResizeParams members as I understand:

new_size is already scaled down. It shrinks when the on screen keyboard comes up, and it's shrunk by top and bottom controls. Most things use this thing. 

physical_backing_size not scaled down, and is only used by cc at draw time. It doesn't shrink by OSK or browser controls. It's used for CalculateDrawProperties and prioritizing tiles. I guess this makes sense, so we still want tiles available when OSK or browser controls suddenly disappear. But then the cost is still maintaining those tiles and compositing them even when they are not visible.

The top and bottom control sizes refer to size of those things. browser_controls_shrink_blink_size looks fully redundant though. afaict it's used the same way everywhere, to treat both controls as size 0 if it's false, in which case browser side could have just sent down 0s to begin with. So can that bool just be removed?


I'm ok with moving the equivalent of new_size to view_android. But I'd like to keep the control sizes in content. The "adjust size by controls and OSK" bit of code should ideally live in chrome code (although chrome code is so scattered and messy..., maybe that should be cleaned up first). Then webcontents/rwhva can just pass the browser controls sizes down without using them for anything.

Comment 26 by bokan@chromium.org, Oct 13 2017

browser_controls_shrink_blink_size isn't redundant. It tells us whether the renderer's current size (RenderWidget::size_) is with or without controls showing. We don't know that because we don't actually resize the renderer until the user lifts their finger after moving browser controls. Let me try to explain with an example (assuming top controls but this works the same way for bottom):

1. Start with the browser controls hidden. Lets say the controls are 50px tall and the screen/viewport is 600px tall. RenderWidget::size_ is 600px tall. browser_controls_shrink_viewport_size is false.

2. User puts their finger down and, without lifting yet, scrolls up, bringing top controls in.

At this point, the top controls are at least partially showing, but the renderer hasn't been resized since the finger is still down. RenderWidget::size_ is still 600px. All we've done so far is change the shown_ratio in BrowserControls and changed the clip size of some layers in the compositor.

3. The user now lifts their finger

Now, the browser controls cause a resize to come from the browser process into the renderer. new_size will be 550px and browser_controls_shrink_blink_size is set to true.

Without browser_controls_shrink_blink_size, we don't know whether the current size of the renderer is with the controls showing or hidden so we don't know which direction to adjust the position: fixed Elements and clipping layers in CC since the shown ratio doesn't affect the size and the controls height is essentially a const. Basically it lets us calculate the full Chrome viewport using:

viewport height = RenderWidget::size_.height() + (browser_controls_shrink_blink_size ? browser_controls_height_ : 0)
Thanks aelias/bokan for feedback.

> I'm ok with moving the equivalent of new_size to view_android. But I'd like to keep the control sizes in content. The "adjust size by controls and OSK" bit of code should ideally live in chrome code (although chrome code is so scattered and messy..., maybe that should be cleaned up first). Then webcontents/rwhva can just pass the browser controls sizes down without using them for anything.

Assuming this preference is independent of the disposal of |browser_controls_shrink_blink_size|, keeping the control sizes in content will work too. My concern was mainly to make sure |content_view_core_| is not involved, so that's possible by keeping them in RWHVA instead. Then RWHVA can compute viewport size, visible view (control/shrinking taken into account) size without refering to cvc call chain.

Which makes me think that it's more desirable for view_android to keep NOT |new_size| which is affected by control/shrinking, but viewport size as given by |OnSizeChanged|?

Comment 28 by boliu@chromium.org, Oct 13 2017

re #26: Oh fixed positions.. so iiuc, fixed position top elements are pushed down by shown_ratio of browser controls *only if* shrink is false? Does things like scroll range and current scroll position also jump when the shrink bool toggles?

Comment 29 by bokan@chromium.org, Oct 13 2017

No, only bottom position: fixed Elements are affected since the renderer rect itself is pushed down with the browser controls. But to keep the bottom fixed elements from leaving the screen, we push them up by the controls shown amount.

So if the bool is true, bottom-fixed Elements are pushed down (since we're growing the renderer). If it's false, we push them up.

Comment 30 by boliu@chromium.org, Oct 13 2017

Let's see where things should move to.

browser control size should not be in view_android. Like discussed here, they are more or less constant anyway. Maybe they should be moved out of ResizeParams, and into RendererPreference or something to make it even more clear, if it actually is constant throughout..

physical backing size actually doesn't belong on view_android. It should just be stored at the root window instead.

I imagine the shrink bool can life on the root window as well, although I didn't check if that actually holds for chrome. Ideally I'd like to pull this out of view_android as well, but this has to change atomically with the size, so meh.

I think the size (after being adjusted by browser controls and OSK) can live in view_android. But all the adjustment code should ideally happen in chrome code.

> Which makes me think that it's more desirable for view_android to keep NOT |new_size| which is affected by control/shrinking, but viewport size as given by |OnSizeChanged|?

But that means the shrinking happens inside content/view_android?
Cc: -yus...@chromium.org tedc...@chromium.org
> browser control size should not be in view_android. Like discussed here, they are more or less constant anyway. Maybe they should be moved out of ResizeParams, and into RendererPreference or something to make it even more clear, if it actually is constant throughout..

Top/Bottom control size comes from resource, which means they are likely to stay unchanged. Only part I can find that may affect the size in runtime is bottom control whose size can change https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabBottomBarDelegate.java?type=cs&q=onBottomControlsHeightChanged&sq=package:chromium&l=263

tedchoc@ could you help us understand how the control size changes over time? We'd like to know if their sizes are constant once instantiated. If they're constant per content, this may need not updating through resize events via ResizeParams, and find other places to store them.

> But that means the shrinking happens inside content/view_android?

Yes... then view_android may have to store both shrunk/unshrunk sizes. Another refactoring CL https://chromium-review.googlesource.com/c/chromium/src/+/711614/4/ui/android/view_android.cc (thought having some conflict with https://crrev.com/666598) indicates that both will be necessary.

> physical backing size actually doesn't belong on view_android. It should just be stored at the root window instead.

Overlay content can have a different physical backing size while sharing WindowAndroid with other contents. So per-content physical size seems right to me.

Comment 33 by boliu@chromium.org, Oct 17 2017

> Yes... then view_android may have to store both shrunk/unshrunk sizes.

what needs the unshrunk size?

> Overlay content can have a different physical backing

how does that work? I hope it's not growing in size as it slides up or something..
> what needs the unshrunk size?

In the linked CL, I'm using the unshrunk size to adjust the content view size which was being done in Java layer. Maybe can be done in a different way but that helps dedup the operation, unless you strongly prefer view_android only holding the shrunk size.

> how does that work? I hope it's not growing in size as it slides up or something..

It is possible that the overlay panel has just different size(narrower width), not covering up the whole screen. Landscape view on Tablet is a good example.
Cc: -hush@chromium.org yus...@chromium.org
Sorry yusufo@ for removing you accidentally. Meant to add tedchoc@ and remove hush@ who already left the team.
From #31, the browser controls in Chrome are currently a static size, but there are several UI explorations around them being dynamic, so I wouldn't make any changes that remove the ability for their size to change over time.

As you found in Custom Tabs already, changing the height of the bottom controls is already supported based on the RemoteView passed from the calling application.

tl;dr - we should continue supporting dynamic control sizing
Thanks tedchoc@ for feedback. My understanding in summary:

- Physical backing size varies from content to content (overlay panel content can have smaller size than that of tab-based contents).

- Brower control size can change over time, more so for upcoming UI features. This justifies the dynamic update through ResizeParams. They may be stored in content (rwhva) or in ui (view_android).

- Shrink bool can be different from content to content at any time (overlay panel content has always the bool set to false), while the one for tab-based contents keeps changing. 

- Size (after being adjusted by browser controls and OSK) can live in view_android, with potential caveat mentioned in #c31. 

Since browser control size and shrink bool are grouped as the browser control info, I would prefer them living in the same place - either in ui/ or content/. 

Comment 38 by boliu@chromium.org, Oct 18 2017

> - Brower control size can change over time, more so for upcoming UI features.

Does that happen frame by frame during the scroll transition, or it's more like a "hey, your browser control has a new size now" kind of thing?

Basically, the question is does it need to be sent in every ResizeParams, or can it be sent out of band?

Comment 39 by boliu@chromium.org, Oct 18 2017

If control size needs to change frame by frame, then putting it along with everything else in view_android is fine. If not, I'd still like it to be some other out of band message.

The unshrunk size still should not exist in content or below. But looks like everything else needs to be in view_android. view_android is getting kinda bloaty...
The current behavior is that is happens very rarely and only in CustomTabs where the client gives a new view.  In theory, this could happen very often, but in practice, I don't see that being the case.

I believe even with the UX mocks I've seen, we could likely handle the size variation purely in the browser w/o telling blink.  If the size goes from 100dp -> 20dp, we could just tell blink the size is 80dp and translate the rendered content by 20dp and then we don't ever get different backing texture sizes.

Comment 41 by boliu@chromium.org, Oct 18 2017

That doesn't sound like frame by frame at least :)

> If the size goes from 100dp -> 20dp, we could just tell blink the size is 80dp and translate the rendered content by 20dp and then we don't ever get different backing texture sizes.

You mean backing texture for the browser controls right? If it's going from 100 to 20, then might want to reduce the texture size just in the name of saving memory...
You have to tell Blink to have both the top and bottom "position: fixed" elements not cut off.  (You need to tell renderer CC to have frame-by-frame positioning the transition, and you need to tell Blink to get correct input hit testing for new touches hitting the "position: fixed" element after scrolling has completed.)
Indeed, we need to tell blink the same information as we were before.  I meant to say that we wouldn't need to tell them any more often in a world where the toolbar collapses to a smaller size but doesn't hide entirely.
Project Member

Comment 44 by bugdroid1@chromium.org, Nov 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06eb4f4d92ff52a02e69ab935fbd2a5df4e23255

commit 06eb4f4d92ff52a02e69ab935fbd2a5df4e23255
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Mon Nov 06 00:19:08 2017

Do not use viewport size in content layer

Content layer is being refactored so that the only view size it knows of
has the effect of size shrinking by browser controls already taken into
account at embedder layer. See https://crrev.com/c/666598 for overall
changes. This CL makes it easy to apply the rest of the changes being made.

For this, all the references to ContentViewCore::GetViewportSize() which
returns the size of the viewport(unshrunken view, as set by onSizeChanged)
should be removed. It does the following to do that:

1) Moves the hit testing on viewport performed in
  RWHVA::OnShowUnhandledTapUIIfNeeded to
  ContextualSearchSelectionClinet.showUnhandledTapUIIfNeeded. Some events
  won't be able to early out, but it won't cause performance issues.

2) Replaces the check against viewport with shrunken view in
  RWHVA::SetContentViewCore. This is safe because the effect of browser
  controls size doesn't matter for checking the width/height against zero
  unless we have viewport as small as browser controls, which is not
  likely in real life.

Bug:  622847 
Change-Id: Ibe713a8dae4ff100bca5f84c3f40b386e5e4c46d
Reviewed-on: https://chromium-review.googlesource.com/746488
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Donn Denman <donnd@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514073}
[modify] https://crrev.com/06eb4f4d92ff52a02e69ab935fbd2a5df4e23255/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
[modify] https://crrev.com/06eb4f4d92ff52a02e69ab935fbd2a5df4e23255/content/browser/android/content_view_core.cc
[modify] https://crrev.com/06eb4f4d92ff52a02e69ab935fbd2a5df4e23255/content/browser/android/content_view_core.h
[modify] https://crrev.com/06eb4f4d92ff52a02e69ab935fbd2a5df4e23255/content/browser/renderer_host/render_widget_host_view_android.cc

Project Member

Comment 45 by bugdroid1@chromium.org, Nov 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29723f2880dae7be2c58f12927df98bf2381ecd8

commit 29723f2880dae7be2c58f12927df98bf2381ecd8
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Wed Nov 22 08:38:26 2017

Shrink content view size in Chrome

Chrome UI had top/bottom browser controls that may shrink Blink's view size.
Previously all the info (control height, flag indicating that shrinking should
happen) are passed down to generate the view size upon request in content layer.

This CL moves the responsibility to embedder(Chrome) so that all the shrinking
happens before reaching content layer. With this CL, the view size stored in
ViewAndroid already the shrinking affect taken into account. The browser control
info are still passed but they are for plumbing, to be used in Blink/cc only.
The new shrinking logic lives in CompositorViewHolder/CompositorView that
already knows about them.

Now browser control update info is fetched through WebContentsDelegate
which is overridden per content (i.e. tabbed content, overlay panel content)
each of which has its own way of managing the info. Other embedders that do not
need dynamic browser control update will use the default implementation.

Related discussion is in the bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=622847#c30

Follow-up CL's will remove size logic still left in ContentView/ContentViewCore,
and also pull the browser control size info out of ResizeParams since they
don't have to be in frame-by-frame struct.

Bug:  622847 
Change-Id: I8b7aa8ba0207b405b83a9c787ec2d8684f2b01fe
Reviewed-on: https://chromium-review.googlesource.com/752963
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518571}
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestRule.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/components/web_contents_delegate_android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/components/web_contents_delegate_android/web_contents_delegate_android.cc
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/components/web_contents_delegate_android/web_contents_delegate_android.h
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/browser/android/content_view_core.cc
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/browser/android/content_view_core.h
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/browser/renderer_host/render_view_host_delegate_view.cc
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/browser/renderer_host/render_view_host_delegate_view.h
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/ui/android/view_android.cc
[modify] https://crrev.com/29723f2880dae7be2c58f12927df98bf2381ecd8/ui/android/view_android.h

Project Member

Comment 46 by bugdroid1@chromium.org, Dec 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5415ae79ed40d50b0e048024e9f15313b79eb001

commit 5415ae79ed40d50b0e048024e9f15313b79eb001
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Thu Dec 07 05:34:37 2017

Remove |onSizeChanged| from ContentView/Core

The event flow was migrated to ViewAndroid. This CL removes
the remaining handler, and moves the post-event task in
ContentViewCore.onSizeChanged to ImeAdapterAndroid/PopupZoomer.

Bug:  622847 
Change-Id: I1814d435365a3aa0f7685b26c860cf56e2856a9a
Reviewed-on: https://chromium-review.googlesource.com/784731
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522351}
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/browser/android/ime_adapter_android.cc
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/browser/android/ime_adapter_android.h
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/public/android/java/src/org/chromium/content/browser/ContentView.java
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/public/android/java/src/org/chromium/content/browser/ContentViewCoreImpl.java
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
[modify] https://crrev.com/5415ae79ed40d50b0e048024e9f15313b79eb001/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewCore.java

Project Member

Comment 47 by bugdroid1@chromium.org, Apr 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b6a48e30b7a21b59c81418854361c5b23a625d71

commit b6a48e30b7a21b59c81418854361c5b23a625d71
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Thu Apr 19 06:19:53 2018

Android: Defines EventHandlerAndroid

ui::EventHandlerAndroid is modeled after ui::EventHandler used for
Aura, to make UI event processing flow similar to how other platforms
do it.

It's renamed from ViewClient as a starting point. Unlike ViewClient
that was required to be set at ViewAndroid constructor, it can be
added dynamically to each ViewAndroid to receive what the impl
of EventHandlerAndroid needs. It remains set while the view is
attached to window.

Bug:  622847 
Change-Id: I89d762b24f0257b8453b9fbd98be468f843c51a9
Reviewed-on: https://chromium-review.googlesource.com/1015448
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551956}
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/ui/android/BUILD.gn
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/ui/android/view_android.cc
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/ui/android/view_android.h
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/ui/android/view_android_unittests.cc
[delete] https://crrev.com/5a5c586e49b5916234677f2a2a51cff76b777016/ui/android/view_client.cc
[modify] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/ui/events/BUILD.gn
[add] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/ui/events/android/event_handler_android.cc
[rename] https://crrev.com/b6a48e30b7a21b59c81418854361c5b23a625d71/ui/events/android/event_handler_android.h

Project Member

Comment 48 by bugdroid1@chromium.org, Apr 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ef9b134fbc73cade58eaa31388c4cdf8574cd80

commit 4ef9b134fbc73cade58eaa31388c4cdf8574cd80
Author: Brian Sheedy <bsheedy@chromium.org>
Date: Sat Apr 21 02:19:09 2018

Revert "Android: Defines EventHandlerAndroid"

This reverts commit b6a48e30b7a21b59c81418854361c5b23a625d71.

Reason for revert: Breaks VR browser and VR browser tests ( https://crbug.com/835375 )

Original change's description:
> Android: Defines EventHandlerAndroid
> 
> ui::EventHandlerAndroid is modeled after ui::EventHandler used for
> Aura, to make UI event processing flow similar to how other platforms
> do it.
> 
> It's renamed from ViewClient as a starting point. Unlike ViewClient
> that was required to be set at ViewAndroid constructor, it can be
> added dynamically to each ViewAndroid to receive what the impl
> of EventHandlerAndroid needs. It remains set while the view is
> attached to window.
> 
> Bug:  622847 
> Change-Id: I89d762b24f0257b8453b9fbd98be468f843c51a9
> Reviewed-on: https://chromium-review.googlesource.com/1015448
> Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
> Reviewed-by: Bo <boliu@chromium.org>
> Reviewed-by: Timothy Dresser <tdresser@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551956}

TBR=boliu@chromium.org,tdresser@chromium.org,jinsukkim@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  622847 
Change-Id: I8bd8fb7ef03809518a07e0d315cbf8d306610cc6
Reviewed-on: https://chromium-review.googlesource.com/1022557
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552552}
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/ui/android/BUILD.gn
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/ui/android/view_android.cc
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/ui/android/view_android.h
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/ui/android/view_android_unittests.cc
[add] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/ui/android/view_client.cc
[rename] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/ui/android/view_client.h
[modify] https://crrev.com/4ef9b134fbc73cade58eaa31388c4cdf8574cd80/ui/events/BUILD.gn
[delete] https://crrev.com/3b6d0efbb12fa7bc621f5ec3285d67ae916650c4/ui/events/android/event_handler_android.cc

Project Member

Comment 49 by bugdroid1@chromium.org, Apr 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f8f47a39161e7dcb0705c7f8d667c8f6e984436

commit 1f8f47a39161e7dcb0705c7f8d667c8f6e984436
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Mon Apr 23 02:08:22 2018

Reland "Android: Defines EventHandlerAndroid"

EventHanderAndroid was accidentally unset when detached from Window.
Relanding after fixing it.

This reverts commit 4ef9b134fbc73cade58eaa31388c4cdf8574cd80.

TBR=tdresser@chromium.org,boliu@chromium.org

Bug:  622847 
Change-Id: I23db2587707271d2d64bfff988c36f02b2f07d5b
Reviewed-on: https://chromium-review.googlesource.com/1023610
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552618}
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/ui/android/BUILD.gn
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/ui/android/view_android.cc
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/ui/android/view_android.h
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/ui/android/view_android_unittests.cc
[delete] https://crrev.com/40e6a844f35346442f02f807425f36d49b467579/ui/android/view_client.cc
[modify] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/ui/events/BUILD.gn
[add] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/ui/events/android/event_handler_android.cc
[rename] https://crrev.com/1f8f47a39161e7dcb0705c7f8d667c8f6e984436/ui/events/android/event_handler_android.h

Project Member

Comment 50 by bugdroid1@chromium.org, Apr 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a3ae93c0440d323b9c393fbd94a4315868b5cc2

commit 5a3ae93c0440d323b9c393fbd94a4315868b5cc2
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Wed Apr 25 21:49:06 2018

Android: Forward key events to EventForwarder

This CL handles key events (onKeyUp, dispatchKeyEvent) by forwarding
them to native first like the others. They are consumed by content
components such as ImeAdapter, TapDisambiguator, etc.

Introduced a couple of new classes for this flow to work:

KeyEventAndroid: New object for Android key event, used to let it through
    the ViewAndroid tree and hit testing logic like other types of events.
    Hit testing is not necessary for key events since they don't have
    location information, but devised to process them in the same pattern.
    First EventHandlerAndroid (which is WCVA) is regarded to have
    the focus, and always expected to consume it.

ContentUiAndroid: Bridges native and Java for the new events.
    Used to pass them back up to Java layer from EventHandlerAndroid(WCVA).

Bug:  622847 
Change-Id: I991274f9ecb49b739ab0f798390d7918a7e4bb37
Reviewed-on: https://chromium-review.googlesource.com/1018682
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Yash Malik <ymalik@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553768}
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/android_webview/javatests/src/org/chromium/android_webview/test/WebKitHitTestTest.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/components/content_view/java/src/org/chromium/components/content_view/ContentView.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/browser/BUILD.gn
[add] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/browser/android/content_ui_event_handler.cc
[add] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/browser/android/content_ui_event_handler.h
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/public/android/BUILD.gn
[add] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/public/android/java/src/org/chromium/content/browser/ContentUiEventHandler.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/public/android/java/src/org/chromium/content/browser/ContentViewCoreImpl.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/public/android/java/src/org/chromium/content_public/browser/ContentViewCore.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewCore.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/android/event_forwarder.cc
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/android/event_forwarder.h
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/android/java/src/org/chromium/ui/base/EventForwarder.java
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/android/view_android.cc
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/android/view_android.h
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/events/BUILD.gn
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/events/android/event_handler_android.cc
[modify] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/events/android/event_handler_android.h
[add] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/events/android/key_event_android.cc
[add] https://crrev.com/5a3ae93c0440d323b9c393fbd94a4315868b5cc2/ui/events/android/key_event_android.h

Project Member

Comment 51 by bugdroid1@chromium.org, May 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ba8f240d065fdf6f91f90a008df3fad75216df5

commit 7ba8f240d065fdf6f91f90a008df3fad75216df5
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Fri May 04 20:44:36 2018

Android: Forward all UI events to native

This CL handles the remaining UI events(scroll/generic motion events)
by forwarding them to native first like the others. They are later
consumed by content components such as JoystickHandler or converted
to other events.

ContentUiEventHandler does the conversion when necessary, and sends
the events down to the associated RenderWidgetHostViewAndroid rather
than putting them back on the view android tree since we are already
at the point of having found the event handler that will consume them.

Bug:  622847 
Change-Id: I8aba396c0545cdc8ae3a6f566da54c2707a460c4
Reviewed-on: https://chromium-review.googlesource.com/999236
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556182}
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/components/content_view/java/src/org/chromium/components/content_view/ContentView.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/content/browser/android/content_ui_event_handler.cc
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/content/browser/android/content_ui_event_handler.h
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/content/public/android/java/src/org/chromium/content/browser/ContentUiEventHandler.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/content/public/android/java/src/org/chromium/content/browser/ContentViewCoreImpl.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/content/public/android/java/src/org/chromium/content_public/browser/ContentViewCore.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewCore.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/ui/android/event_forwarder.cc
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/ui/android/event_forwarder.h
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/ui/android/java/src/org/chromium/ui/base/EventForwarder.java
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/ui/android/view_android.cc
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/ui/android/view_android.h
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/ui/android/window_android.h
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/ui/events/android/event_handler_android.cc
[modify] https://crrev.com/7ba8f240d065fdf6f91f90a008df3fad75216df5/ui/events/android/event_handler_android.h

Status: Fixed (was: Assigned)

Sign in to add a comment