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

Issue 627943 link

Starred by 3 users
Status: Fixed
Owner:
█ rlanday@chromium.org
Last visit > 30 days ago
Closed: Dec 2016
Cc:
tedc...@chromium.org
jinsuk...@chromium.org
█ rlanday@chromium.org
amaralp@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 598880



Sign in to add a comment

Android: Disentangle SwipeRefreshHandler and ContentViewCore/Tab/TabWebContentsObserver

Project Member Reported by siev...@chromium.org, Jul 13 2016 
This can probably be simply driven from the RWHV's OverscrollController in native instead of being routed through ContentViewCore and then burdening the embedder with creating this and hiding it when navigating and what not.

There is a content-interface that's implemented in chrome/ but then uses the Android SwipeRefreshLayout from third_party.

(+amaralp and jinsuk since in some ways it's a bit similar to popups and some of the questions encountered with the selection logic/action mode)

One simple flow I could imagine is that when the RWHV's OverscrollController is created it calls up to get an interface for this from the embedder (for example through WebContentsViewDelegate).

The browser can then instantiate this and naturally associate it with the web contents (for resources and to trigger the reload, add/remove it to/from the layout). But the visibility (including for navigation, see TabWebContentsObserver.didNavigateMainFrame's call to mTab.stopSwipeRefreshHandler()) could be driven from the renderer view's visibility.

We could even consider dealing with the visibility in a consistent way for all the widgets that are associated with a render widget (popups and action mode and this) so that they all automatically hide more naturally when a RWHV/WebContents is hidden. Right now the logic to hide popups and what not and also handle navigation and other events is a bit scattered.

For example, it could trickle through ViewAndroid (and there could be a child ViewAndroid to the WebContentsView or RenderWidgetHostView's ViewAndroid that represents the state of the popup or swipe refresh handler).

Comment 1 by siev...@chromium.org, Jul 13 2016

Blocking: 598880

Comment 2 by durga.behera@chromium.org, Aug 5 2016

Components: Content>Core
Labels: OS-Android

Comment 3 by jinsuk...@chromium.org, Nov 10 2016

Cc: rlanday@chromium.org

Comment 4 by aelias@chromium.org, Nov 11 2016

Owner: rlanday@chromium.org
Status: Assigned (was: Available)

Comment 5 by jinsuk...@chromium.org, Nov 14 2016 

WebContentsViewDelegate may not be easy to use since AFAICT RWHVA wouldn't have access to it without adding a new delegate to connect them.

One possible approach would be add native SwipeRefreshController to replace the forwarding through ContentViewCore. And it will be handy to use ViewAndroid to truly lazily create entire OverscrollRefreshHandler instance necessary to create the OverscrollController.

Not sure how this can deal with visibility in this way though.

Comment 6 by jinsuk...@chromium.org, Nov 17 2016 

The idea #c5 won't work since ViewAndroidDelegate cannot have dependency on  OverscrollRefreshHandler in content.... This is another motivation for us to get rid of the Java side interface and only use the native one :)

Setting aside lazy creation part a bit... we can take a different approach such as introducing SwipeRefreshHandler native instance inheriting ui::OverscrolLRefreshHandler, letting WebContentsViewAndroid store it, and pass it to RWVHA ctr to create OverscrollController. In this way SwipeRefreshHandler doesn't have to inherit ORH, so it can go away.
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 14 2016 

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

commit 7f2ec7f88816aa89f872f068881d117047049cfe
Author: rlanday <rlanday@chromium.org>
Date: Wed Dec 14 02:28:34 2016

Separate SwipeRefreshHandler and ContentViewCore

Currently Android's native ContentViewCore class implements
ui::OverscrollRefreshHandler and handles swipe to refresh events. This
change refactors this functionality out of ContentViewCore into a new
native SwipeRefreshHandler class as part of a broader effort to
eliminate ContentViewCore.

BUG= 627943 

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

[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/BUILD.gn
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/android/overscroll_controller_android.cc
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/android/overscroll_controller_android.h
[add] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/renderer_host/render_view_host_delegate_view.cc
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/renderer_host/render_view_host_delegate_view.h
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/web_contents/web_contents_android.cc
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/web_contents/web_contents_android.h
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/public/android/BUILD.gn
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/ui/android/BUILD.gn
[rename] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/ui/android/java/src/org/chromium/ui/OverscrollRefreshHandler.java
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/ui/android/overscroll_refresh.cc
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/ui/android/overscroll_refresh.h
[add] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/ui/android/overscroll_refresh_handler.cc
[add] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/ui/android/overscroll_refresh_handler.h
[modify] https://crrev.com/7f2ec7f88816aa89f872f068881d117047049cfe/ui/android/overscroll_refresh_unittest.cc

Comment 8 by rlanday@chromium.org, Dec 14 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment