From ec2322e54319668369e5ae705db37e15391446a6 Mon Sep 17 00:00:00 2001 From: "mkosiba@chromium.org" Date: Thu, 15 May 2014 16:32:00 +0000 Subject: Make LayerScrollOffsetDelegate updates consistent. Looks like the inner+outer viewport changes had made the scroll size/range depend on the pageScale. BUG=340646 Review URL: https://codereview.chromium.org/256303006 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@270686 0039d316-1c4b-4281-b951-d872f2087c98 --- android_webview/browser/browser_view_renderer.cc | 125 +++++++++++++-------- android_webview/browser/browser_view_renderer.h | 20 ++-- .../browser/browser_view_renderer_client.h | 19 ++-- .../org/chromium/android_webview/AwContents.java | 57 +++++----- .../test/AndroidScrollIntegrationTest.java | 87 +++++++++++++- android_webview/native/aw_contents.cc | 43 +++---- android_webview/native/aw_contents.h | 12 +- 7 files changed, 231 insertions(+), 132 deletions(-) (limited to 'android_webview') diff --git a/android_webview/browser/browser_view_renderer.cc b/android_webview/browser/browser_view_renderer.cc index cad7c5b..b131ad9 100644 --- a/android_webview/browser/browser_view_renderer.cc +++ b/android_webview/browser/browser_view_renderer.cc @@ -11,6 +11,7 @@ #include "base/auto_reset.h" #include "base/command_line.h" #include "base/debug/trace_event.h" +#include "base/json/json_writer.h" #include "base/logging.h" #include "base/strings/string_number_conversions.h" #include "base/strings/stringprintf.h" @@ -73,6 +74,27 @@ class AutoResetWithLock { DISALLOW_COPY_AND_ASSIGN(AutoResetWithLock); }; +class TracedValue : public base::debug::ConvertableToTraceFormat { + public: + explicit TracedValue(base::Value* value) : value_(value) {} + static scoped_refptr FromValue( + base::Value* value) { + return scoped_refptr( + new TracedValue(value)); + } + virtual void AppendAsTraceFormat(std::string* out) const OVERRIDE { + std::string tmp; + base::JSONWriter::Write(value_.get(), &tmp); + *out += tmp; + } + + private: + virtual ~TracedValue() {} + scoped_ptr value_; + + DISALLOW_COPY_AND_ASSIGN(TracedValue); +}; + } // namespace // static @@ -485,6 +507,14 @@ void BrowserViewRenderer::ScrollTo(gfx::Vector2d scroll_offset) { scroll_offset_dip_ = scroll_offset_dip; } + TRACE_EVENT_INSTANT2("android_webview", + "BrowserViewRenderer::ScrollTo", + TRACE_EVENT_SCOPE_THREAD, + "x", + scroll_offset_dip.x(), + "y", + scroll_offset_dip.y()); + if (has_compositor_) shared_renderer_state_->GetCompositor()-> DidChangeRootLayerScrollOffset(); @@ -505,35 +535,8 @@ void BrowserViewRenderer::DidUpdateContent() { client_->OnNewPicture(); } -void BrowserViewRenderer::SetMaxRootLayerScrollOffset( - gfx::Vector2dF new_value_dip) { - if (!ui_task_runner_->BelongsToCurrentThread()) { - ui_task_runner_->PostTask( - FROM_HERE, - base::Bind(&BrowserViewRenderer::SetMaxRootLayerScrollOffset, - ui_thread_weak_ptr_, - new_value_dip)); - return; - } - DCHECK_GT(dip_scale_, 0); - - max_scroll_offset_dip_ = new_value_dip; - DCHECK_LE(0, max_scroll_offset_dip_.x()); - DCHECK_LE(0, max_scroll_offset_dip_.y()); - - client_->SetMaxContainerViewScrollOffset(max_scroll_offset()); -} - void BrowserViewRenderer::SetTotalRootLayerScrollOffset( gfx::Vector2dF scroll_offset_dip) { - if (!ui_task_runner_->BelongsToCurrentThread()) { - ui_task_runner_->PostTask( - FROM_HERE, - base::Bind(&BrowserViewRenderer::SetTotalRootLayerScrollOffset, - ui_thread_weak_ptr_, - scroll_offset_dip)); - return; - } { base::AutoLock lock(render_thread_lock_); @@ -561,10 +564,8 @@ void BrowserViewRenderer::SetTotalRootLayerScrollOffset( DCHECK(0 <= scroll_offset.x()); DCHECK(0 <= scroll_offset.y()); - // Disabled because the conditions are being violated while running - // AwZoomTest.testMagnification, see http://crbug.com/340648 - // DCHECK(scroll_offset.x() <= max_offset.x()); - // DCHECK(scroll_offset.y() <= max_offset.y()); + DCHECK(scroll_offset.x() <= max_offset.x()); + DCHECK(scroll_offset.y() <= max_offset.y()); client_->ScrollContainerViewTo(scroll_offset); } @@ -583,38 +584,68 @@ bool BrowserViewRenderer::IsExternalFlingActive() const { return client_->IsFlingActive(); } -void BrowserViewRenderer::SetRootLayerPageScaleFactorAndLimits( +void BrowserViewRenderer::UpdateRootLayerState( + const gfx::Vector2dF& total_scroll_offset_dip, + const gfx::Vector2dF& max_scroll_offset_dip, + const gfx::SizeF& scrollable_size_dip, float page_scale_factor, float min_page_scale_factor, float max_page_scale_factor) { if (!ui_task_runner_->BelongsToCurrentThread()) { ui_task_runner_->PostTask( FROM_HERE, - base::Bind(&BrowserViewRenderer::SetRootLayerPageScaleFactorAndLimits, + base::Bind(&BrowserViewRenderer::UpdateRootLayerState, ui_thread_weak_ptr_, + total_scroll_offset_dip, + max_scroll_offset_dip, + scrollable_size_dip, page_scale_factor, min_page_scale_factor, max_page_scale_factor)); return; } + TRACE_EVENT_INSTANT1( + "android_webview", + "BrowserViewRenderer::UpdateRootLayerState", + TRACE_EVENT_SCOPE_THREAD, + "state", + TracedValue::FromValue( + RootLayerStateAsValue(total_scroll_offset_dip, scrollable_size_dip) + .release())); + + DCHECK_GT(dip_scale_, 0); + + max_scroll_offset_dip_ = max_scroll_offset_dip; + DCHECK_LE(0, max_scroll_offset_dip_.x()); + DCHECK_LE(0, max_scroll_offset_dip_.y()); + page_scale_factor_ = page_scale_factor; DCHECK_GT(page_scale_factor_, 0); - client_->SetPageScaleFactorAndLimits( - page_scale_factor, min_page_scale_factor, max_page_scale_factor); - client_->SetMaxContainerViewScrollOffset(max_scroll_offset()); + + client_->UpdateScrollState(max_scroll_offset(), + scrollable_size_dip, + page_scale_factor, + min_page_scale_factor, + max_page_scale_factor); + SetTotalRootLayerScrollOffset(total_scroll_offset_dip); } -void BrowserViewRenderer::SetRootLayerScrollableSize( - gfx::SizeF scrollable_size) { - if (!ui_task_runner_->BelongsToCurrentThread()) { - ui_task_runner_->PostTask( - FROM_HERE, - base::Bind(&BrowserViewRenderer::SetRootLayerScrollableSize, - ui_thread_weak_ptr_, - scrollable_size)); - return; - } - client_->SetContentsSize(scrollable_size); +scoped_ptr BrowserViewRenderer::RootLayerStateAsValue( + const gfx::Vector2dF& total_scroll_offset_dip, + const gfx::SizeF& scrollable_size_dip) { + scoped_ptr state(new base::DictionaryValue); + + state->SetDouble("total_scroll_offset_dip.x", total_scroll_offset_dip.x()); + state->SetDouble("total_scroll_offset_dip.y", total_scroll_offset_dip.y()); + + state->SetDouble("max_scroll_offset_dip.x", max_scroll_offset_dip_.x()); + state->SetDouble("max_scroll_offset_dip.y", max_scroll_offset_dip_.y()); + + state->SetDouble("scrollable_size_dip.width", scrollable_size_dip.width()); + state->SetDouble("scrollable_size_dip.height", scrollable_size_dip.height()); + + state->SetDouble("page_scale_factor", page_scale_factor_); + return state.PassAs(); } void BrowserViewRenderer::DidOverscroll(gfx::Vector2dF accumulated_overscroll, diff --git a/android_webview/browser/browser_view_renderer.h b/android_webview/browser/browser_view_renderer.h index 991075f..9487a3e 100644 --- a/android_webview/browser/browser_view_renderer.h +++ b/android_webview/browser/browser_view_renderer.h @@ -11,6 +11,7 @@ #include "base/android/scoped_java_ref.h" #include "base/callback.h" #include "base/cancelable_callback.h" +#include "base/values.h" #include "content/public/browser/android/synchronous_compositor_client.h" #include "skia/ext/refptr.h" #include "ui/gfx/rect.h" @@ -120,17 +121,16 @@ class BrowserViewRenderer : public content::SynchronousCompositorClient, virtual void DidDestroyCompositor(content::SynchronousCompositor* compositor) OVERRIDE; virtual void SetContinuousInvalidate(bool invalidate) OVERRIDE; - virtual void SetMaxRootLayerScrollOffset(gfx::Vector2dF new_value) OVERRIDE; - virtual void SetTotalRootLayerScrollOffset(gfx::Vector2dF new_value_css) - OVERRIDE; virtual void DidUpdateContent() OVERRIDE; virtual gfx::Vector2dF GetTotalRootLayerScrollOffset() OVERRIDE; + virtual void UpdateRootLayerState( + const gfx::Vector2dF& total_scroll_offset_dip, + const gfx::Vector2dF& max_scroll_offset_dip, + const gfx::SizeF& scrollable_size_dip, + float page_scale_factor, + float min_page_scale_factor, + float max_page_scale_factor) OVERRIDE; virtual bool IsExternalFlingActive() const OVERRIDE; - virtual void SetRootLayerPageScaleFactorAndLimits(float page_scale_factor, - float min_page_scale_factor, - float max_page_scale_factor) - OVERRIDE; - virtual void SetRootLayerScrollableSize(gfx::SizeF scrollable_size) OVERRIDE; virtual void DidOverscroll(gfx::Vector2dF accumulated_overscroll, gfx::Vector2dF latest_overscroll_delta, gfx::Vector2dF current_fling_velocity) OVERRIDE; @@ -141,6 +141,7 @@ class BrowserViewRenderer : public content::SynchronousCompositorClient, bool effective_immediately) OVERRIDE; private: + void SetTotalRootLayerScrollOffset(gfx::Vector2dF new_value_dip); // Checks the continuous invalidate and block invalidate state, and schedule // invalidates appropriately. If |force_invalidate| is true, then send a view // invalidate regardless of compositor expectation. @@ -148,6 +149,9 @@ class BrowserViewRenderer : public content::SynchronousCompositorClient, bool DrawSWInternal(jobject java_canvas, const gfx::Rect& clip_bounds); bool CompositeSW(SkCanvas* canvas); void DidComposite(bool force_invalidate); + scoped_ptr RootLayerStateAsValue( + const gfx::Vector2dF& total_scroll_offset_dip, + const gfx::SizeF& scrollable_size_dip); // If we call up view invalidate and OnDraw is not called before a deadline, // then we keep ticking the SynchronousCompositor so it can make progress. diff --git a/android_webview/browser/browser_view_renderer_client.h b/android_webview/browser/browser_view_renderer_client.h index fce1c80..41fbf5a 100644 --- a/android_webview/browser/browser_view_renderer_client.h +++ b/android_webview/browser/browser_view_renderer_client.h @@ -34,20 +34,19 @@ class BrowserViewRendererClient { // Try to set the view's scroll offset to |new_value|. virtual void ScrollContainerViewTo(gfx::Vector2d new_value) = 0; - // Set the view's scroll offset cap to |new_value|. - virtual void SetMaxContainerViewScrollOffset(gfx::Vector2d new_value) = 0; - // Is a Android view system managed fling in progress? virtual bool IsFlingActive() const = 0; - // Set the current page scale to |page_scale_factor| and page scale limits + // Sets the following: + // view's scroll offset cap to |max_scroll_offset|, + // current contents_size to |contents_size_dip|, + // the current page scale to |page_scale_factor| and page scale limits // to |min_page_scale_factor|..|max_page_scale_factor|. - virtual void SetPageScaleFactorAndLimits(float page_scale_factor, - float min_page_scale_factor, - float max_page_scale_factor) = 0; - - // Set the current contents_size to |contents_size_dip|. - virtual void SetContentsSize(gfx::SizeF contents_size_dip) = 0; + virtual void UpdateScrollState(gfx::Vector2d max_scroll_offset, + gfx::SizeF contents_size_dip, + float page_scale_factor, + float min_page_scale_factor, + float max_page_scale_factor) = 0; // Handle overscroll. virtual void DidOverscroll(gfx::Vector2d overscroll_delta) = 0; diff --git a/android_webview/java/src/org/chromium/android_webview/AwContents.java b/android_webview/java/src/org/chromium/android_webview/AwContents.java index 4f14219..86e6fa9 100644 --- a/android_webview/java/src/org/chromium/android_webview/AwContents.java +++ b/android_webview/java/src/org/chromium/android_webview/AwContents.java @@ -1949,11 +1949,6 @@ public class AwContents { } @CalledByNative - private void setMaxContainerViewScrollOffset(int maxX, int maxY) { - mScrollOffsetManager.setMaxScrollOffset(maxX, maxY); - } - - @CalledByNative private void scrollContainerViewTo(int x, int y) { mScrollOffsetManager.scrollContainerViewTo(x, y); } @@ -1964,31 +1959,14 @@ public class AwContents { } @CalledByNative - private void setContentsSize(int widthDip, int heightDip) { - mContentWidthDip = widthDip; - mContentHeightDip = heightDip; - } - - @CalledByNative - private void setPageScaleFactorAndLimits( + private void updateScrollState(int maxContainerViewScrollOffsetX, + int maxContainerViewScrollOffsetY, int contentWidthDip, int contentHeightDip, float pageScaleFactor, float minPageScaleFactor, float maxPageScaleFactor) { - if (mPageScaleFactor == pageScaleFactor && - mMinPageScaleFactor == minPageScaleFactor && - mMaxPageScaleFactor == maxPageScaleFactor) { - return; - } - mMinPageScaleFactor = minPageScaleFactor; - mMaxPageScaleFactor = maxPageScaleFactor; - if (mPageScaleFactor != pageScaleFactor) { - float oldPageScaleFactor = mPageScaleFactor; - mPageScaleFactor = pageScaleFactor; - // NOTE: if this ever needs to become synchronous then we need to make sure the scroll - // bounds are correctly updated before calling the method, otherwise embedder code that - // attempts to scroll on scale change might cause weird results. - mContentsClient.getCallbackHelper().postOnScaleChangedScaled( - (float)(oldPageScaleFactor * mDIPScale), - (float)(mPageScaleFactor * mDIPScale)); - } + mContentWidthDip = contentWidthDip; + mContentHeightDip = contentHeightDip; + mScrollOffsetManager.setMaxScrollOffset(maxContainerViewScrollOffsetX, + maxContainerViewScrollOffsetY); + setPageScaleFactorAndLimits(pageScaleFactor, minPageScaleFactor, maxPageScaleFactor); } @CalledByNative @@ -2014,6 +1992,27 @@ public class AwContents { // Helper methods // ------------------------------------------------------------------------------------------- + private void setPageScaleFactorAndLimits( + float pageScaleFactor, float minPageScaleFactor, float maxPageScaleFactor) { + if (mPageScaleFactor == pageScaleFactor && + mMinPageScaleFactor == minPageScaleFactor && + mMaxPageScaleFactor == maxPageScaleFactor) { + return; + } + mMinPageScaleFactor = minPageScaleFactor; + mMaxPageScaleFactor = maxPageScaleFactor; + if (mPageScaleFactor != pageScaleFactor) { + float oldPageScaleFactor = mPageScaleFactor; + mPageScaleFactor = pageScaleFactor; + // NOTE: if this ever needs to become synchronous then we need to make sure the scroll + // bounds are correctly updated before calling the method, otherwise embedder code that + // attempts to scroll on scale change might cause weird results. + mContentsClient.getCallbackHelper().postOnScaleChangedScaled( + (float)(oldPageScaleFactor * mDIPScale), + (float)(mPageScaleFactor * mDIPScale)); + } + } + private void saveWebArchiveInternal(String path, final ValueCallback callback) { if (path == null || mNativeAwContents == 0) { ThreadUtils.runOnUiThread(new Runnable() { diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java index a222023..9c522a5 100644 --- a/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java +++ b/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java @@ -30,6 +30,7 @@ public class AndroidScrollIntegrationTest extends AwTestBase { private static class OverScrollByCallbackHelper extends CallbackHelper { int mDeltaX; int mDeltaY; + int mScrollRangeY; public int getDeltaX() { assert getCallCount() > 0; @@ -41,9 +42,15 @@ public class AndroidScrollIntegrationTest extends AwTestBase { return mDeltaY; } - public void notifyCalled(int deltaX, int deltaY) { + public int getScrollRangeY() { + assert getCallCount() > 0; + return mScrollRangeY; + } + + public void notifyCalled(int deltaX, int deltaY, int scrollRangeY) { mDeltaX = deltaX; mDeltaY = deltaY; + mScrollRangeY = scrollRangeY; notifyCalled(); } } @@ -80,7 +87,7 @@ public class AndroidScrollIntegrationTest extends AwTestBase { protected boolean overScrollBy(int deltaX, int deltaY, int scrollX, int scrollY, int scrollRangeX, int scrollRangeY, int maxOverScrollX, int maxOverScrollY, boolean isTouchEvent) { - mOverScrollByCallbackHelper.notifyCalled(deltaX, deltaY); + mOverScrollByCallbackHelper.notifyCalled(deltaX, deltaY, scrollRangeY); return super.overScrollBy(deltaX, deltaY, scrollX, scrollY, scrollRangeX, scrollRangeY, maxOverScrollX, maxOverScrollY, isTouchEvent); } @@ -122,6 +129,9 @@ public class AndroidScrollIntegrationTest extends AwTestBase { private static final String TEST_PAGE_COMMON_HEADERS = " " + "