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"
@@ skipping 69 matching lines @@
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableOverscrollEdgeEffect)) {
     return nullptr;
   }
 
   return base::MakeUnique<OverscrollGlow>(client);
 }
 
 std::unique_ptr<OverscrollRefresh> CreateRefreshEffect(
     OverscrollRefreshHandler* handler) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(

[Jinsuk Kim, 2016/11/25 01:49:18]

Now that OverscrollRefreshHandler is not CVC but a separate object, this flag
checking can be moved to native SwipeRefreshHandler Init() to avoid creating the
instance in the first place rather than passing it all the way here and doing
nothing in case the feature is disabled.

[rlanday, 2016/11/29 00:46:25]

Sorry, I missed this comment before---I suppose we can do this; is it weird
though to always construct the Java SwipeRefreshHandler but sometimes not
construct the C++ one?

[rlanday, 2016/11/30 00:18:33]

I think this discussion is now moot with the latest version of the patch because
there is no longer a native SwipeRefreshHandler.

           switches::kDisablePullToRefreshEffect)) {
     return nullptr;
   }
 
   return base::MakeUnique<OverscrollRefresh>(handler);
 }
 
 }  // namespace
 
 OverscrollControllerAndroid::OverscrollControllerAndroid(
     ContentViewCoreImpl* content_view_core,
+    OverscrollRefreshHandler* overscroll_refresh_handler,
     float dpi_scale)
     : compositor_(content_view_core->GetWindowAndroid()->GetCompositor()),

[Jinsuk Kim, 2016/11/25 01:49:18]

Now you can pass the compositor object directly - then you can get rid of the
dependency on CVC.

[rlanday, 2016/11/29 00:46:25]

Done

       dpi_scale_(dpi_scale),
       enabled_(true),
-      glow_effect_(CreateGlowEffect(this, dpi_scale_)),
-      refresh_effect_(CreateRefreshEffect(content_view_core)) {
+      glow_effect_(CreateGlowEffect(this, dpi_scale_)) {
+  if (overscroll_refresh_handler) {
+    refresh_effect_ = CreateRefreshEffect(overscroll_refresh_handler);

[Jinsuk Kim, 2016/11/25 01:49:18]

How about keep this intact but update CreateRefreshEffect to handle null
pointer?

[rlanday, 2016/11/29 00:46:25]

Done

+  }
   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