Separate SwipeRefreshHandler and ContentViewCore

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index fc2b4b5b07e11c1a7bb982c0ad5ebc85e4a2eff5..a9e794a99b211547078b130ebc09e44dd317091a 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -55,6 +55,7 @@
 #include "content/browser/renderer_host/input/synthetic_gesture_target_android.h"
 #include "content/browser/renderer_host/input/web_input_event_builders_android.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/browser/renderer_host/render_view_host_delegate_view.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/common/gpu_host_messages.h"
@@ -336,13 +337,6 @@ std::unique_ptr<ui::TouchSelectionController> CreateSelectionController(
   return base::MakeUnique<ui::TouchSelectionController>(client, config);
 }
 
-std::unique_ptr<OverscrollControllerAndroid> CreateOverscrollController(
-    ContentViewCoreImpl* content_view_core,
-    float dpi_scale) {
-  return base::MakeUnique<OverscrollControllerAndroid>(content_view_core,
-                                                       dpi_scale);
-}
-
 gfx::RectF GetSelectionRect(const ui::TouchSelectionController& controller) {
   gfx::RectF rect = controller.GetRectBetweenBounds();
   if (rect.IsEmpty())
@@ -470,6 +464,8 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
   host_->SetView(this);
   SetContentViewCore(content_view_core);
 
+  CreateOverscrollControllerIfPossible();
+
   if (GetTextInputManager())
     GetTextInputManager()->AddObserver(this);
 }
@@ -1221,6 +1217,11 @@ void RenderWidgetHostViewAndroid::SetSynchronousCompositorClient(
   }
 }
 
+void RenderWidgetHostViewAndroid::OnOverscrollRefreshHandlerSet() {

[Ted C, 2016/12/13 21:09:48]

I would probably use Available instead of Set, but that probably makes this far
too long.

[rlanday, 2016/12/14 01:11:15]

I think that fits

+  DCHECK(!overscroll_controller_);
+  CreateOverscrollControllerIfPossible();
+}
+
 bool RenderWidgetHostViewAndroid::SupportsAnimation() const {
   // The synchronous (WebView) compositor does not have a proper browser
   // compositor with which to drive animations.
@@ -1773,7 +1774,6 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
 
   bool resize = false;
   if (content_view_core != content_view_core_) {
-    overscroll_controller_.reset();
     selection_controller_.reset();
     ReleaseLocksOnSurface();
     // TODO(yusufo) : Get rid of the below conditions and have a better handling
@@ -1821,12 +1821,8 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
   if (!selection_controller_)
     selection_controller_ = CreateSelectionController(this, content_view_core_);
 
-  if (!overscroll_controller_ &&
-      view_.GetWindowAndroid()->GetCompositor()) {
-    overscroll_controller_ = CreateOverscrollController(
-        content_view_core_, ui::GetScaleFactorForNativeView(GetNativeView()));
-    is_showing_overscroll_glow_ = true;
-  }
+  if (content_view_core_)
+    CreateOverscrollControllerIfPossible();
 }
 
 void RenderWidgetHostViewAndroid::RunAckCallbacks() {
@@ -1853,6 +1849,7 @@ void RenderWidgetHostViewAndroid::OnGestureEvent(
 
 void RenderWidgetHostViewAndroid::OnContentViewCoreDestroyed() {
   SetContentViewCore(NULL);
+  overscroll_controller_.reset();
 }
 
 void RenderWidgetHostViewAndroid::OnCompositingDidCommit() {
@@ -1890,9 +1887,7 @@ void RenderWidgetHostViewAndroid::OnDetachedFromWindow() {
 
 void RenderWidgetHostViewAndroid::OnAttachCompositor() {
   DCHECK(content_view_core_);
-  if (!overscroll_controller_)
-    overscroll_controller_ = CreateOverscrollController(
-        content_view_core_, ui::GetScaleFactorForNativeView(GetNativeView()));
+  CreateOverscrollControllerIfPossible();
   ui::WindowAndroidCompositor* compositor =
       view_.GetWindowAndroid()->GetCompositor();
   delegated_frame_host_->RegisterFrameSinkHierarchy(
@@ -2006,4 +2001,47 @@ void RenderWidgetHostViewAndroid::ComputeEventLatencyOSTouchHistograms(
   }
 }
 
+void RenderWidgetHostViewAndroid::CreateOverscrollControllerIfPossible() {
+  // an OverscrollController is already set
+  if (overscroll_controller_)
+    return;
+
+  RenderWidgetHostDelegate* delegate = host_->delegate();
+  if (!delegate)
+    return;
+
+  RenderViewHostDelegateView* delegate_view = delegate->GetDelegateView();
+  // render_widget_host_unittest.cc uses an object called
+  // MockRenderWidgetHostDelegate that does not have a DelegateView
+  if (!delegate_view)
+    return;
+
+  ui::OverscrollRefreshHandler* overscroll_refresh_handler =
+      delegate_view->GetOverscrollRefreshHandler();
+  if (!overscroll_refresh_handler)
+    return;
+
+  if (!content_view_core_)
+    return;
+
+  // If window_android is null here, this is bad because we don't listen for it
+  // being set, so we won't be able to construct the OverscrollController at the
+  // proper time.
+  // TODO(rlanday): once we get WindowAndroid from ViewAndroid instead of
+  // ContentViewCore, listen for WindowAndroid being set and create the
+  // OverscrollController.
+  ui::WindowAndroid* window_android = content_view_core_->GetWindowAndroid();
+  if (!window_android)
+    return;
+
+  ui::WindowAndroidCompositor* compositor = window_android->GetCompositor();
+  if (!compositor)
+    return;
+
+  overscroll_controller_ = base::MakeUnique<OverscrollControllerAndroid>(

[Ted C, 2016/12/13 21:09:48]

I guess we could also have the RenderViewHostDelegateView expose a
CreateOverscrollControllerAndroid or something that could encapsulate all of
this compositor gathering and stuff if needed.

If we wanted to get CVC out of this file entirely, that might be a way.

Not needed now though.

[rlanday, 2016/12/14 01:11:15]

I added a note here about how we hope to someday get WindowAndroid from
ViewAndroid instead of CVC, I think that would also eliminate the dependency?

+      overscroll_refresh_handler, compositor,
+      ui::GetScaleFactorForNativeView(GetNativeView()));
+  is_showing_overscroll_glow_ = true;

[rlanday, 2016/12/08 19:29:14]

This is the new thing that was added in
https://codereview.chromium.org/2524423002, for some reason they're only doing
this in SetContentViewCore() and not OnAttachCompositor(), I think it's a bug
but I asked there to be sure

[boliu, 2016/12/08 19:32:29]

lgtm

+}
+
 }  // namespace content