content/browser/renderer_host/render_widget_host_view_android.cc - Issue 2528823002: Separate SwipeRefreshHandler and ContentViewCore

Unified Diff: content/browser/renderer_host/render_widget_host_view_android.cc

Issue 2528823002: Separate SwipeRefreshHandler and ContentViewCore (Closed)

Patch Set: Respond to boliu's comments, refactor CreateOverscrollController() function Created 4 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« no previous file with comments | « content/browser/renderer_host/render_widget_host_view_android.h ('k') | content/browser/web_contents/web_contents_android.h » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

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 c0daedb3742ad67bd53a166e75877f14c7c4db38..713f18fb83a449343b96f6f4681b2ce1674d65f0 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())

@@ -469,6 +463,8 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(

host_->SetView(this);

SetContentViewCore(content_view_core);

+ CreateOverscrollControllerIfPossible();

if (GetTextInputManager())

GetTextInputManager()->AddObserver(this);

}

@@ -1220,6 +1216,11 @@ void RenderWidgetHostViewAndroid::SetSynchronousCompositorClient(

}

+void RenderWidgetHostViewAndroid::OnOverscrollRefreshHandlerSet() {

+ overscroll_controller_.reset();

boliu 2016/12/06 23:24:16 this can be set multiple times? did this fix a uni

rlanday 2016/12/07 00:07:48 We were discussing how in the current version of t

boliu 2016/12/07 00:33:15 Wording.. it's not CVC can be set multiple times,

+ CreateOverscrollControllerIfPossible();

bool RenderWidgetHostViewAndroid::SupportsAnimation() const {

// The synchronous (WebView) compositor does not have a proper browser

// compositor with which to drive animations.

@@ -1768,7 +1769,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

@@ -1815,12 +1815,6 @@ 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()));

- }

}

void RenderWidgetHostViewAndroid::RunAckCallbacks() {

@@ -1884,9 +1878,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(

@@ -2000,4 +1992,32 @@ void RenderWidgetHostViewAndroid::ComputeEventLatencyOSTouchHistograms(

}

+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

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 alway

rlanday 2016/12/07 00:07:48 This is actually what the test case was failing on

boliu 2016/12/07 00:33:15 Ok.. Do you have the full stack of when that happe

rlanday 2016/12/07 00:46:00 I 8.385s Main signal 11 (SIGSEGV), code 1 (SEG

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 D

+ 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

rlanday 2016/12/07 00:07:48 Hmm, this check doesn't already exist in the curre

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 theo

+ ui::GetScaleFactorForNativeView(GetNativeView()));

} // namespace content