Separate SwipeRefreshHandler and ContentViewCore

content/browser/android/overscroll_controller_android.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/android/overscroll_controller_android.h"
 
 #include "base/android/build_info.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "cc/layers/layer.h"
 #include "cc/output/compositor_frame_metadata.h"
 #include "content/browser/android/content_view_core_impl.h"

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

don't need this include

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

It's needed for member access into blink::WebGestureEvent&

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

then forward declare that as well, or include the WebGestureEvent header

[rlanday, 2016/12/06 03:25:26]

Ok, I will include the WebGestureEvent header (a forward declaration won't work)

 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/common/content_switches.h"
 #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"
 #include "ui/base/l10n/l10n_util_android.h"
 #include "ui/events/blink/did_overscroll_params.h"
 
 using ui::DidOverscrollParams;
 using ui::EdgeEffect;
 using ui::EdgeEffectBase;
 using ui::EdgeEffectL;
 using ui::OverscrollGlow;
 using ui::OverscrollGlowClient;
 using ui::OverscrollRefresh;
-using ui::OverscrollRefreshHandler;
 
 namespace content {
 namespace {
 
 // Used for conditional creation of EdgeEffect types for the overscroll glow.
 const int kAndroidLSDKVersion = 21;
 
 // If the glow effect alpha is greater than this value, the refresh effect will
 // be suppressed. This value was experimentally determined to provide a
 // reasonable balance between avoiding accidental refresh activation and
@@ skipping 36 matching lines @@
                                                  float dpi_scale) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableOverscrollEdgeEffect)) {
     return nullptr;
   }
 
   return base::MakeUnique<OverscrollGlow>(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_);
 }
 
 OverscrollControllerAndroid::~OverscrollControllerAndroid() {
 }
 
 bool OverscrollControllerAndroid::WillHandleGestureEvent(
     const blink::WebGestureEvent& event) {
   if (!enabled_)
     return false;
@@ skipping 156 matching lines @@
 std::unique_ptr<EdgeEffectBase>
 OverscrollControllerAndroid::CreateEdgeEffect() {
   return CreateGlowEdgeEffect(&compositor_->GetResourceManager(), dpi_scale_);
 }
 
 void OverscrollControllerAndroid::SetNeedsAnimate() {
   compositor_->SetNeedsAnimate();
 }
 
 }  // namespace content