Separate SwipeRefreshHandler and ContentViewCore

content/browser/renderer_host/render_widget_host_view_android.cc

 // Copyright (c) 2012 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/renderer_host/render_widget_host_view_android.h"
 
 #include <android/bitmap.h>
 
 #include <utility>
 
@@ skipping 37 matching lines @@
 #include "content/browser/gpu/gpu_data_manager_impl.h"
 #include "content/browser/gpu/gpu_process_host_ui_shim.h"
 #include "content/browser/media/android/media_web_contents_observer_android.h"
 #include "content/browser/renderer_host/compositor_impl_android.h"
 #include "content/browser/renderer_host/dip_util.h"
 #include "content/browser/renderer_host/frame_metadata_util.h"
 #include "content/browser/renderer_host/input/input_router_impl.h"
 #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"
 #include "content/common/input_messages.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/android/compositor.h"
 #include "content/public/browser/android/synchronous_compositor_client.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/render_view_host.h"
@@ skipping 261 matching lines @@
   config.tap_slop = gfx::ViewConfiguration::GetTouchSlopInDips();
   config.enable_adaptive_handle_orientation =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableAdaptiveSelectionHandleOrientation);
   config.enable_longpress_drag_selection =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableLongpressDragSelection);
   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())
     return rect;
 
   rect.Union(controller.GetStartHandleRect());
   rect.Union(controller.GetEndHandleRect());
   return rect;
 }
 
@@ skipping 106 matching lines @@
   if (using_browser_compositor_) {
     delegated_frame_host_.reset(new ui::DelegatedFrameHostAndroid(
         &view_, cached_background_color_,
         base::Bind(&RenderWidgetHostViewAndroid::ReturnResources,
                    weak_ptr_factory_.GetWeakPtr())));
   }
 
   host_->SetView(this);
   SetContentViewCore(content_view_core);
 
+  CreateOverscrollControllerIfPossible();
+
   if (GetTextInputManager())
     GetTextInputManager()->AddObserver(this);
 }
 
 RenderWidgetHostViewAndroid::~RenderWidgetHostViewAndroid() {
   if (content_view_core_)
     content_view_core_->RemoveObserver(this);
   SetContentViewCore(NULL);
   DCHECK(ack_callbacks_.empty());
   DCHECK(!delegated_frame_host_);
@@ skipping 731 matching lines @@
 }
 
 void RenderWidgetHostViewAndroid::SetSynchronousCompositorClient(
       SynchronousCompositorClient* client) {
   synchronous_compositor_client_ = client;
   if (!sync_compositor_ && synchronous_compositor_client_) {
     sync_compositor_ = SynchronousCompositorHost::Create(this);
   }
 }
 
+void RenderWidgetHostViewAndroid::OnOverscrollRefreshHandlerSet() {
+  overscroll_controller_.reset();

[boliu, 2016/12/06 23:24:16]

this can be set multiple times? did this fix a unit test or something?

[rlanday, 2016/12/07 00:07:48]

We were discussing how in the current version of the code, ContentViewCore can
be set multiple times. Since OverscrollRefreshHandler is currently part of
ContentViewCore, that means I think the effect is being able to reset
OverscrollRefreshHandler (it makes a new OverscrollController). I wasn't sure
it's safe to prohibit something that I think is currently allowed.

[boliu, 2016/12/07 00:33:15]

Wording.. it's not CVC can be set multiple times, it's that CVC can be set only
once, but that "one time" can be set at different points in the lifetime of
RWHVA. (It can be set to null multiple times though, apparently)

I think same is true for WCVA

+  CreateOverscrollControllerIfPossible();
+}
+
 bool RenderWidgetHostViewAndroid::SupportsAnimation() const {
   // The synchronous (WebView) compositor does not have a proper browser
   // compositor with which to drive animations.
   return using_browser_compositor_;
 }
 
 void RenderWidgetHostViewAndroid::SetNeedsAnimate() {
   DCHECK(view_.GetWindowAndroid());
   DCHECK(using_browser_compositor_);
   view_.GetWindowAndroid()->SetNeedsAnimate();
@@ skipping 528 matching lines @@
 }
 
 void RenderWidgetHostViewAndroid::SetContentViewCore(
     ContentViewCoreImpl* content_view_core) {
   DCHECK(!content_view_core || !content_view_core_ ||
          (content_view_core_ == content_view_core));
   StopObservingRootWindow();
 
   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
     // for resizing after crbug.com/628302 is handled.
     bool is_size_initialized = !content_view_core
         || content_view_core->GetViewportSizeDip().width() != 0
         || content_view_core->GetViewportSizeDip().height() != 0;
     if (content_view_core_ || is_size_initialized)
       resize = true;
     if (content_view_core_) {
@@ skipping 26 matching lines @@
   }
 
   if (is_showing_)
     StartObservingRootWindow();
 
   if (resize)
     WasResized();
 
   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()));
-  }
 }
 
 void RenderWidgetHostViewAndroid::RunAckCallbacks() {
   while (!ack_callbacks_.empty()) {
     ack_callbacks_.front().Run();
     ack_callbacks_.pop();
   }
 }
 
 void RenderWidgetHostViewAndroid::OnGestureEvent(
@@ skipping 43 matching lines @@
     OnAttachCompositor();
 }
 
 void RenderWidgetHostViewAndroid::OnDetachedFromWindow() {
   StopObservingRootWindow();
   OnDetachCompositor();
 }
 
 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(
       compositor->GetFrameSinkId());
 }
 
 void RenderWidgetHostViewAndroid::OnDetachCompositor() {
   DCHECK(content_view_core_);
   DCHECK(using_browser_compositor_);
   RunAckCallbacks();
@@ skipping 93 matching lines @@
       return;
     case ui::MotionEvent::ACTION_UP:
     case ui::MotionEvent::ACTION_POINTER_UP:
       UMA_HISTOGRAM_CUSTOM_COUNTS("Event.Latency.OS.TOUCH_RELEASED",
                                   delta.InMicroseconds(), 1, 1000000, 50);
     default:
       return;
   }
 }
 
+void RenderWidgetHostViewAndroid::CreateOverscrollControllerIfPossible() {
+  // an OverscrollController is already set
+  if (overscroll_controller_) {

[boliu, 2016/12/06 23:24:16]

style: for if blocks where the both the condition and the body are single line,
don't need braces, but do keep them on separate lines

I guess that would make this method a bit less wordy..

(note for java, style rule is different, yay...)

[rlanday, 2016/12/07 00:07:48]

Ok

+    return;
+  }
+
+  RenderWidgetHostDelegate* delegate = host_->delegate();
+  if (!delegate) {
+    return;
+  }
+
+  RenderViewHostDelegateView* delegate_view = delegate->GetDelegateView();
+  if (!delegate_view) {

[boliu, 2016/12/06 23:24:16]

can this happen? Pretty sure WebContentsImpl always has a DelegateView..

[rlanday, 2016/12/07 00:07:48]

This is actually what the test case was failing on:
[INFO:render_widget_host_view_android.cc(475)] host_: 0xb3fe8880
[INFO:render_widget_host_view_android.cc(476)] host_->delegate()0xb3fc4340
[INFO:render_widget_host_view_android.cc(477)]
host_->delegate()->GetDelegateView()0

It's possible there's always a delegate, is that check necessary?

[boliu, 2016/12/07 00:33:15]

Ok.. Do you have the full stack of when that happens. I'm interested.

[rlanday, 2016/12/07 00:46:00]

I    8.385s Main  signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0
I    8.385s Main  
I    8.385s Main  Stack Trace:
I    8.385s Main    RELADDR   FUNCTION                                          
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                       FILE:LINE
I    8.385s Main    00db80d1  RenderWidgetHostViewAndroid                       
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc:476
I    8.385s Main    00b69e4a 
content::RenderWidgetHostTest_Background_Test::TestBody()                       
                                                                                
                                                                                
                                                                                
                                                                                
                                                                         
/usr/local/google/home/rlanday/chromium/src/content/browser/renderer_host/render_widget_host_unittest.cc:878
I    8.385s Main    014dd742  void
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*)                   
                                                                                
                                                                                
                                                                                
                                                                               
/usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest-internal-inl.h:803
I    8.385s Main    014dd699  testing::Test::Run()                              
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:2474
I    8.386s Main    014de028  testing::TestInfo::Run()                          
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:2656
I    8.386s Main    014de63f  testing::TestCase::Run()                          
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:2774
I    8.386s Main    014e4184  testing::internal::UnitTestImpl::RunAllTests()    
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:4647
I    8.386s Main    014e3e97  bool
testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool>(testing::internal::UnitTestImpl*, bool
(testing::internal::UnitTestImpl::*)(), char const*)                            
                                                                                
                                                                                
                                                                                
                
/usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest-internal-inl.h:803
I    8.386s Main    014e3dd3  testing::UnitTest::Run()                          
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/testing/gtest/src/gtest.cc:4255
I    8.386s Main    v------>  RUN_ALL_TESTS()                                   
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/testing/gtest/include/gtest/gtest.h:2237
I    8.386s Main    012a17bd  base::TestSuite::Run()                            
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/base/test/test_suite.cc:271
I    8.386s Main    0126620c  content::UnitTestTestSuite::Run()                 
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/content/public/test/unittest_test_suite.cc:45
I    8.386s Main    v------>  int base::internal::FunctorTraits<int
(content::UnitTestTestSuite::*)(),
void>::Invoke<content::UnitTestTestSuite*>(int
(content::UnitTestTestSuite::*)(), content::UnitTestTestSuite*&&)               
                                                                                
                                                                                
                                                                                
                                  
/usr/local/google/home/rlanday/chromium/src/base/bind_internal.h:214
I    8.386s Main    v------>  int base::internal::InvokeHelper<false,
int>::MakeItSo<int (content::UnitTestTestSuite::* const&)(),
content::UnitTestTestSuite*>(int (content::UnitTestTestSuite::* const&)(),
content::UnitTestTestSuite*&&)                                                  
                                                                                
                                                                                
                                                           
/usr/local/google/home/rlanday/chromium/src/base/bind_internal.h:285
I    8.386s Main    v------>  int
base::internal::Invoker<base::internal::BindState<int
(content::UnitTestTestSuite::*)(),
base::internal::UnretainedWrapper<content::UnitTestTestSuite> >, int
()>::RunImpl<int (content::UnitTestTestSuite::* const&)(),
std::__ndk1::tuple<base::internal::UnretainedWrapper<content::UnitTestTestSuite>
> const&, 0u>(int (content::UnitTestTestSuite::* const&)(),
std::__ndk1::tuple<base::internal::UnretainedWrapper<content::UnitTestTestSuite>
> const&, base::IndexSequence<0u>) 
/usr/local/google/home/rlanday/chromium/src/base/bind_internal.h:361
I    8.386s Main    00f3e7de 
base::internal::Invoker<base::internal::BindState<int
(content::UnitTestTestSuite::*)(),
base::internal::UnretainedWrapper<content::UnitTestTestSuite> >, int
()>::Run(base::internal::BindStateBase*)                                        
                                                                                
                                                                                
                                                                             
/usr/local/google/home/rlanday/chromium/src/base/bind_internal.h:339
I    8.386s Main    v------>  base::internal::RunMixin<base::Callback<int (),
(base::internal::CopyMode)1, (base::internal::RepeatMode)1> >::Run() const      
                                                                                
                                                                                
                                                                                
                                                                                
                         
/usr/local/google/home/rlanday/chromium/src/base/callback.h:85
I    8.387s Main    v------>  base::(anonymous
namespace)::LaunchUnitTestsInternal(base::Callback<int (),
(base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, int, int,
bool, base::Callback<void (), (base::internal::CopyMode)1,
(base::internal::RepeatMode)1> const&)                                          
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/base/test/launcher/unit_test_launcher.cc:187
I    8.387s Main    012b7e1a  base::LaunchUnitTests(int, char**,
base::Callback<int (), (base::internal::CopyMode)1,
(base::internal::RepeatMode)1> const&)                                          
                                                                                
                                                                                
                                                                                
                                                                   
/usr/local/google/home/rlanday/chromium/src/base/test/launcher/unit_test_launcher.cc:453
I    8.387s Main    00f3e775  main                                              
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                      
/usr/local/google/home/rlanday/chromium/src/content/test/run_all_unittests.cc:22
I    8.387s Main    v------>  testing::android::RunTests(_JNIEnv*,
base::android::JavaParamRef<_jobject*> const&,
base::android::JavaParamRef<_jstring*> const&,
base::android::JavaParamRef<_jstring*> const&,
base::android::JavaParamRef<_jstring*> const&,
base::android::JavaParamRef<_jobject*> const&,
base::android::JavaParamRef<_jstring*> const&)                                  
                                                                                
                                            
/usr/local/google/home/rlanday/chromium/src/testing/android/native_test/native_test_launcher.cc:127
I    8.387s Main    01200705 
Java_org_chromium_native_1test_NativeTest_nativeRunTests                        
                                                                                
                                                                                
                                                                                
                                                                                
                                                                         
/usr/local/google/home/rlanday/chromium/src/out/Android86/gen/testing/android/native_test/native_test_jni_headers/testing/jni/NativeTest_jni.h:49
I    8.387s Main    00784a88  offset 0x4e1000) (void
org.chromium.native_test.NativeTest.nativeRunTests(java.lang.String,
java.lang.String, java.lang.String, android.content.Context,
java.lang.String)+252                                                           
                                                                                
                                                                                
                                                                                
  /data/app/org.chromium.native_test-1/oat/x86/base.odex
I    8.387s Main    007858fd  offset 0x4e1000) (void
org.chromium.native_test.NativeTest.runTests(android.app.Activity)+257          
                                                                                
                                                                                
                                                                                
                                                                                
                                                  
/data/app/org.chromium.native_test-1/oat/x86/base.odex
I    8.387s Main    00784960  offset 0x4e1000) (void
org.chromium.native_test.NativeTest.access$000(org.chromium.native_test.NativeTest,
android.app.Activity)+68                                                        
                                                                                
                                                                                
                                                                                
                                               
/data/app/org.chromium.native_test-1/oat/x86/base.odex
I    8.387s Main    007845cd  offset 0x4e1000) (void
org.chromium.native_test.NativeTest$2.run()+113                                 
                                                                                
                                                                                
                                                                                
                                                                                
                                                  
/data/app/org.chromium.native_test-1/oat/x86/base.odex
I    8.387s Main    7423bc3b  offset 0x1eb2000                                  
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                       /data/dalvik-cache/x86/system@framework@boot.oat
I    8.387s Main  

[boliu, 2016/12/07 00:50:19]

meh, MockRenderWidgetHostDelegate doesn't have a DelegateView:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

Can you add a comment that this is for unit tests?

+    return;
+  }
+
+  ui::OverscrollRefreshHandler* overscroll_refresh_handler =
+      delegate_view->GetOverscrollRefreshHandler();
+  if (!overscroll_refresh_handler) {
+    return;
+  }
+
+  overscroll_controller_ = base::MakeUnique<OverscrollControllerAndroid>(
+      overscroll_refresh_handler,
+      content_view_core_->GetWindowAndroid()->GetCompositor(),

[boliu, 2016/12/06 23:24:16]

also need to null check GetCompositor

[boliu, 2016/12/06 23:26:57]

actually, need to null check content_view_core_ as well, probably

[rlanday, 2016/12/07 00:07:48]

Hmm, this check doesn't already exist in the current version of the code, does
it? Are you saying we need it in case the OverscrollRefreshHandler is set before
the ContentViewCore? In that case, are there any other changes we need to make?

[boliu, 2016/12/07 00:33:15]

Line 1820 on the left

[boliu, 2016/12/07 00:35:46]

Oh wait you mean null check for CVC? Well, in theory,
OnOverscrollRefreshHandlerSet can come before or after SetCVC(non-null CVC),
right? I don't think we should assume which order they happen

+      ui::GetScaleFactorForNativeView(GetNativeView()));
+}
+
 }  // namespace content