Separate SwipeRefreshHandler and ContentViewCore

content/browser/android/overscroll_controller_android.cc

Index: content/browser/android/overscroll_controller_android.cc
diff --git a/content/browser/android/overscroll_controller_android.cc b/content/browser/android/overscroll_controller_android.cc
index 6e92d182c8aa540c173f9709895f13117d0527cc..2c1491a4dfbeafd4dbb16ee6f32cd5157bc07256 100644
--- a/content/browser/android/overscroll_controller_android.cc
+++ b/content/browser/android/overscroll_controller_android.cc
@@ -16,6 +16,7 @@
 #include "third_party/WebKit/public/platform/WebInputEvent.h"
 #include "ui/android/edge_effect.h"
 #include "ui/android/edge_effect_l.h"
+#include "ui/android/overscroll_refresh_handler.h"
 #include "ui/android/resources/resource_manager.h"
 #include "ui/android/window_android.h"
 #include "ui/android/window_android_compositor.h"
@@ -29,7 +30,6 @@ using ui::EdgeEffectL;
 using ui::OverscrollGlow;
 using ui::OverscrollGlowClient;
 using ui::OverscrollRefresh;
-using ui::OverscrollRefreshHandler;
 
 namespace content {
 namespace {
@@ -86,25 +86,29 @@ std::unique_ptr<OverscrollGlow> CreateGlowEffect(OverscrollGlowClient* client,
 }
 
 std::unique_ptr<OverscrollRefresh> CreateRefreshEffect(
-    OverscrollRefreshHandler* handler) {
+    ui::OverscrollRefreshHandler* overscroll_refresh_handler) {
+  if (overscroll_refresh_handler == nullptr)

[boliu, 2016/12/03 01:05:28]

should write RWHVA/WCVA such that this is never null here

so I think instead of relying on RenderWidgetHostViewAndroid::SetContentViewCore
to create OverscrollController, maybe should be an explicit signal when WCVA
gets the OverscrollRefreshHandler?

[rlanday, 2016/12/05 19:53:56]

I'm not quite sure what you mean by "explicit signal", are you saying that
WebContentsViewAndroid should construct the OverscrollController instead of
constructing it here?

[rlanday, 2016/12/05 23:01:33]

Ok, so I took the call to CreateOverscrollController() out of
RenderWidgetHostViewAndroid::SetContentViewCore(). There are now three places
where I have calls to CreateOverscrollAndroid():

1. The RenderWidgetHostViewAndroid constructor. The constructor calls
SetContentViewCore(), which before my change was creating the
OverscrollController in the case where the handler is already set in
WebContentsViewAndroid before RenderWidgetHostViewAndroid is constructed.

2. I'm adding a new method,
RenderWidgetHostViewAndroid::OnSetOverscrollRefreshHandler(), which is called by
WebContentsViewAndroid::SetOverscrollRefreshHandler() if the RWHVA has already
been constructed.

3. RenderWidgetHostViewAndroid::OnAttachCompositor(). I don't 100% understand
under what circumstances we need this one, but I assume it's necessary since the
code is already doing it here.

Does this sound reasonable? Should I add a helper method, either for
host_->delegate()->GetDelegateView()->GetOverscrollRefreshHandler()

or to actually create the OverscrollController, to reduce code duplication? Note
that in OnSetOverscrollRefreshHandler(), we know the handler is not null (I
think), but in the other cases we need to do the check. Could we have one
function that just does the check whether or not it's needed, or would the
unnecessary check be potentially confusing?

[boliu, 2016/12/06 00:46:33]

Overall pretty reasonable to me. 

Probably fine either way whether you want to refactor calling
CreateOverscrollController into a separate function. I think in this case, once
the Handler is set, it's never unset, right? So either is is fine with me :)

+    return nullptr;
+
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisablePullToRefreshEffect)) {
     return nullptr;
   }
 
-  return base::MakeUnique<OverscrollRefresh>(handler);
+  return base::MakeUnique<OverscrollRefresh>(overscroll_refresh_handler);
 }
 
 }  // namespace
 
 OverscrollControllerAndroid::OverscrollControllerAndroid(
-    ContentViewCoreImpl* content_view_core,
+    ui::OverscrollRefreshHandler* overscroll_refresh_handler,
+    ui::WindowAndroidCompositor* compositor,
     float dpi_scale)
-    : compositor_(content_view_core->GetWindowAndroid()->GetCompositor()),
+    : compositor_(compositor),
       dpi_scale_(dpi_scale),
       enabled_(true),
       glow_effect_(CreateGlowEffect(this, dpi_scale_)),
-      refresh_effect_(CreateRefreshEffect(content_view_core)) {
+      refresh_effect_(CreateRefreshEffect(overscroll_refresh_handler)) {
   DCHECK(compositor_);
 }