Skip to content

Commit

Permalink
make scroll_offset not stored in dip scale in BrowserViewRenderer
Browse files Browse the repository at this point in the history 
The scroll amount passed into BrowserViewRenderer::ScrollTo is
physical pixel with page_scale_factor applied.
Before --use-zoom-for-dsf enabled, the scroll amount was divided by
both dip_scale_ and page_scale_factor_ to convert to scroll amount
in dip. With the flag enabled, it still need to divide by the
page_scale_factor, but it incorrectly passed the original offset
without take page_scale_factor into account, causes scroll amount
larger than the max_scroll_offset.

When --use-zoom-for-dsf is enabled, scroll_offset_dip_ and
max_scroll_offset_dip_ are convert from/to physical pixel in
every it get used. We shouldn't stored them in dip.

So, this cl changes scroll_offset_dip_ to scroll_offset_unscaled_
and max_scroll_offset_dip_ to max_scroll_offset_unscaled_,
as they were in physical pixel when zoom-for-dsf enabled, and
dip otherwise. Instead of having dip_scale applied every time it
was set.

TEST: webview_ui_test.test.DropDownListTest#testDropDownScaledViewPortUseWideViewPort
Bug: 869995, 873617
Change-Id: I694ca54438620e7afdf8db210704f40fb2c1ba54
Reviewed-on: https://chromium-review.googlesource.com/1162519
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Ella Ge <eirage@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582692}
  • Loading branch information
@EiraGe
EiraGe authored and Commit Bot committed Aug 13, 2018
1 parent 62dd4bf commit 76a886b
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 76 deletions.
131 changes: 61 additions & 70 deletions android_webview/browser/browser_view_renderer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,21 +350,17 @@ sk_sp<SkPicture> BrowserViewRenderer::CapturePicture(int width,
SkPictureRecorder recorder;
SkCanvas* rec_canvas = recorder.beginRecording(width, height, NULL, 0);
if (compositor_) {
gfx::Vector2dF scroll_offset =
content::IsUseZoomForDSFEnabled()
? gfx::ScaleVector2d(scroll_offset_dip_, dip_scale_)
: scroll_offset_dip_;
{
// Reset scroll back to the origin, will go back to the old
// value when scroll_reset is out of scope.
base::AutoReset<gfx::Vector2dF> scroll_reset(&scroll_offset_dip_,
base::AutoReset<gfx::Vector2dF> scroll_reset(&scroll_offset_unscaled_,
gfx::Vector2dF());
compositor_->DidChangeRootLayerScrollOffset(
gfx::ScrollOffset(scroll_offset));
gfx::ScrollOffset(scroll_offset_unscaled_));
CompositeSW(rec_canvas);
}
compositor_->DidChangeRootLayerScrollOffset(
gfx::ScrollOffset(scroll_offset));
gfx::ScrollOffset(scroll_offset_unscaled_));
}
return recorder.finishRecordingAsPicture();
}
Expand Down Expand Up @@ -562,53 +558,53 @@ void BrowserViewRenderer::SetDipScale(float dip_scale) {

gfx::Vector2d BrowserViewRenderer::max_scroll_offset() const {
DCHECK_GT(dip_scale_, 0.f);
return gfx::ToCeiledVector2d(gfx::ScaleVector2d(
max_scroll_offset_dip_, dip_scale_ * page_scale_factor_));
float scale = content::IsUseZoomForDSFEnabled()
? page_scale_factor_
: dip_scale_ * page_scale_factor_;
return gfx::ToCeiledVector2d(
gfx::ScaleVector2d(max_scroll_offset_unscaled_, scale));
}

void BrowserViewRenderer::ScrollTo(const gfx::Vector2d& scroll_offset) {
gfx::Vector2d max_offset = max_scroll_offset();
gfx::Vector2dF scroll_offset_dip;
gfx::Vector2dF scroll_offset_unscaled;
// To preserve the invariant that scrolling to the maximum physical pixel
// value also scrolls to the maximum dip pixel value we transform the physical
// offset into the dip offset by using a proportion (instead of dividing by
// dip_scale * page_scale_factor).
if (max_offset.x()) {
scroll_offset_dip.set_x((scroll_offset.x() * max_scroll_offset_dip_.x()) /
max_offset.x());
scroll_offset_unscaled.set_x(
(scroll_offset.x() * max_scroll_offset_unscaled_.x()) / max_offset.x());
}
if (max_offset.y()) {
scroll_offset_dip.set_y((scroll_offset.y() * max_scroll_offset_dip_.y()) /
max_offset.y());
scroll_offset_unscaled.set_y(
(scroll_offset.y() * max_scroll_offset_unscaled_.y()) / max_offset.y());
}

DCHECK_LE(0.f, scroll_offset_dip.x());
DCHECK_LE(0.f, scroll_offset_dip.y());
DCHECK(scroll_offset_dip.x() < max_scroll_offset_dip_.x() ||
scroll_offset_dip.x() - max_scroll_offset_dip_.x() < kEpsilon)
<< scroll_offset_dip.x() << " " << max_scroll_offset_dip_.x();
DCHECK(scroll_offset_dip.y() < max_scroll_offset_dip_.y() ||
scroll_offset_dip.y() - max_scroll_offset_dip_.y() < kEpsilon)
<< scroll_offset_dip.y() << " " << max_scroll_offset_dip_.y();

if (scroll_offset_dip_ == scroll_offset_dip)
DCHECK_LE(0.f, scroll_offset_unscaled.x());
DCHECK_LE(0.f, scroll_offset_unscaled.y());
DCHECK(scroll_offset_unscaled.x() < max_scroll_offset_unscaled_.x() ||
scroll_offset_unscaled.x() - max_scroll_offset_unscaled_.x() <
kEpsilon)
<< scroll_offset_unscaled.x() << " " << max_scroll_offset_unscaled_.x();
DCHECK(scroll_offset_unscaled.y() < max_scroll_offset_unscaled_.y() ||
scroll_offset_unscaled.y() - max_scroll_offset_unscaled_.y() <
kEpsilon)
<< scroll_offset_unscaled.y() << " " << max_scroll_offset_unscaled_.y();

if (scroll_offset_unscaled_ == scroll_offset_unscaled)
return;

scroll_offset_dip_ = scroll_offset_dip;
scroll_offset_unscaled_ = scroll_offset_unscaled;

TRACE_EVENT_INSTANT2("android_webview",
"BrowserViewRenderer::ScrollTo",
TRACE_EVENT_SCOPE_THREAD,
"x",
scroll_offset_dip.x(),
"y",
scroll_offset_dip.y());
TRACE_EVENT_INSTANT2("android_webview", "BrowserViewRenderer::ScrollTo",
TRACE_EVENT_SCOPE_THREAD, "x",
scroll_offset_unscaled.x(), "y",
scroll_offset_unscaled.y());

if (compositor_) {
compositor_->DidChangeRootLayerScrollOffset(gfx::ScrollOffset(
content::IsUseZoomForDSFEnabled() ? scroll_offset
: scroll_offset_dip_));
}
if (compositor_)
compositor_->DidChangeRootLayerScrollOffset(
gfx::ScrollOffset(scroll_offset_unscaled));
}

void BrowserViewRenderer::DidUpdateContent(
Expand All @@ -625,23 +621,23 @@ void BrowserViewRenderer::DidUpdateContent(
}

void BrowserViewRenderer::SetTotalRootLayerScrollOffset(
const gfx::Vector2dF& scroll_offset_dip) {
if (scroll_offset_dip_ == scroll_offset_dip)
const gfx::Vector2dF& scroll_offset_unscaled) {
if (scroll_offset_unscaled_ == scroll_offset_unscaled)
return;
scroll_offset_dip_ = scroll_offset_dip;
scroll_offset_unscaled_ = scroll_offset_unscaled;

gfx::Vector2d max_offset = max_scroll_offset();
gfx::Vector2d scroll_offset;
// For an explanation as to why this is done this way see the comment in
// BrowserViewRenderer::ScrollTo.
if (max_scroll_offset_dip_.x()) {
scroll_offset.set_x((scroll_offset_dip.x() * max_offset.x()) /
max_scroll_offset_dip_.x());
if (max_scroll_offset_unscaled_.x()) {
scroll_offset.set_x((scroll_offset_unscaled.x() * max_offset.x()) /
max_scroll_offset_unscaled_.x());
}

if (max_scroll_offset_dip_.y()) {
scroll_offset.set_y((scroll_offset_dip.y() * max_offset.y()) /
max_scroll_offset_dip_.y());
if (max_scroll_offset_unscaled_.y()) {
scroll_offset.set_y((scroll_offset_unscaled.y() * max_offset.y()) /
max_scroll_offset_unscaled_.y());
}

DCHECK_LE(0, scroll_offset.x());
Expand All @@ -663,34 +659,27 @@ void BrowserViewRenderer::UpdateRootLayerState(
if (compositor != compositor_)
return;

gfx::Vector2dF total_scroll_offset_dip = total_scroll_offset;
gfx::Vector2dF max_scroll_offset_dip = total_max_scroll_offset;
gfx::SizeF scrollable_size_dip = scrollable_size;
if (content::IsUseZoomForDSFEnabled()) {
total_scroll_offset_dip.Scale(1 / dip_scale_);
max_scroll_offset_dip.Scale(1 / dip_scale_);
if (content::IsUseZoomForDSFEnabled())
scrollable_size_dip.Scale(1 / dip_scale_);
}

TRACE_EVENT_INSTANT1(
"android_webview",
"BrowserViewRenderer::UpdateRootLayerState",
TRACE_EVENT_SCOPE_THREAD,
"state",
RootLayerStateAsValue(total_scroll_offset_dip, scrollable_size_dip));

DCHECK_GE(max_scroll_offset_dip.x(), 0.f);
DCHECK_GE(max_scroll_offset_dip.y(), 0.f);
"android_webview", "BrowserViewRenderer::UpdateRootLayerState",
TRACE_EVENT_SCOPE_THREAD, "state",
RootLayerStateAsValue(total_scroll_offset, scrollable_size_dip));

DCHECK_GE(total_max_scroll_offset.x(), 0.f);
DCHECK_GE(total_max_scroll_offset.y(), 0.f);
DCHECK_GT(page_scale_factor, 0.f);
// SetDipScale should have been called at least once before this is called.
DCHECK_GT(dip_scale_, 0.f);

if (max_scroll_offset_dip_ != max_scroll_offset_dip ||
if (max_scroll_offset_unscaled_ != total_max_scroll_offset ||
scrollable_size_dip_ != scrollable_size_dip ||
page_scale_factor_ != page_scale_factor ||
min_page_scale_factor_ != min_page_scale_factor ||
max_page_scale_factor_ != max_page_scale_factor) {
max_scroll_offset_dip_ = max_scroll_offset_dip;
max_scroll_offset_unscaled_ = total_max_scroll_offset;
scrollable_size_dip_ = scrollable_size_dip;
page_scale_factor_ = page_scale_factor;
min_page_scale_factor_ = min_page_scale_factor;
Expand All @@ -700,21 +689,23 @@ void BrowserViewRenderer::UpdateRootLayerState(
page_scale_factor, min_page_scale_factor,
max_page_scale_factor);
}
SetTotalRootLayerScrollOffset(total_scroll_offset_dip);
SetTotalRootLayerScrollOffset(total_scroll_offset);
}

std::unique_ptr<base::trace_event::ConvertableToTraceFormat>
BrowserViewRenderer::RootLayerStateAsValue(
const gfx::Vector2dF& total_scroll_offset_dip,
const gfx::Vector2dF& total_scroll_offset,
const gfx::SizeF& scrollable_size_dip) {
std::unique_ptr<base::trace_event::TracedValue> state(
new base::trace_event::TracedValue());

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("total_scroll_offset.x", total_scroll_offset.x());
state->SetDouble("total_scroll_offset.y", total_scroll_offset.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("max_scroll_offset_unscaled.x",
max_scroll_offset_unscaled_.x());
state->SetDouble("max_scroll_offset_unscaled.y",
max_scroll_offset_unscaled_.y());

state->SetDouble("scrollable_size_dip.width", scrollable_size_dip.width());
state->SetDouble("scrollable_size_dip.height", scrollable_size_dip.height());
Expand Down Expand Up @@ -777,8 +768,8 @@ std::string BrowserViewRenderer::ToString() const {
base::StringAppendF(&str,
"global visible rect: %s ",
last_on_draw_global_visible_rect_.ToString().c_str());
base::StringAppendF(
&str, "scroll_offset_dip: %s ", scroll_offset_dip_.ToString().c_str());
base::StringAppendF(&str, "scroll_offset_unscaled: %s ",
scroll_offset_unscaled_.ToString().c_str());
base::StringAppendF(&str,
"overscroll_rounding_error_: %s ",
overscroll_rounding_error_.ToString().c_str());
Expand Down
13 changes: 8 additions & 5 deletions android_webview/browser/browser_view_renderer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ class BrowserViewRenderer : public content::SynchronousCompositorClient,
float dip_scale() const { return dip_scale_; }
float page_scale_factor() const { return page_scale_factor_; }

// Set the root layer scroll offset to |new_value|.
// Set the root layer scroll offset to |new_value|. The |new_value| here is in
// physical pixel.
void ScrollTo(const gfx::Vector2d& new_value);

// Android views hierarchy gluing.
Expand Down Expand Up @@ -224,11 +225,13 @@ class BrowserViewRenderer : public content::SynchronousCompositorClient,

gfx::SizeF scrollable_size_dip_;

// TODO(miletus): Make scroll_offset_dip_ a gfx::ScrollOffset.
gfx::Vector2dF scroll_offset_dip_;
// When zoom-for-dsf enabled |max_scroll_offset_unscaled_| and
// |scroll_offset_unscaled_| is in physical pixel; otherwise, they are in dip
// TODO(miletus): Make scroll_offset_unscaled_ a gfx::ScrollOffset.
gfx::Vector2dF scroll_offset_unscaled_;

// TODO(miletus): Make max_scroll_offset_dip_ a gfx::ScrollOffset.
gfx::Vector2dF max_scroll_offset_dip_;
// TODO(miletus): Make max_scroll_offset_unscaled_ a gfx::ScrollOffset.
gfx::Vector2dF max_scroll_offset_unscaled_;

// Used to prevent rounding errors from accumulating enough to generate
// visible skew (especially noticeable when scrolling up and down in the same
Expand Down
4 changes: 3 additions & 1 deletion content/browser/android/synchronous_compositor_host.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ class SynchronousCompositorHost : public SynchronousCompositor,
// Indicates begin frames are paused from the browser.
bool begin_frame_paused_ = false;

// Updated by both renderer and browser.
// Updated by both renderer and browser. This is in physical pixel when
// use-zoom-for-dsf is enabled, otherwise in dip.
gfx::ScrollOffset root_scroll_offset_;

// Indicates that whether OnComputeScroll is called or overridden. The
Expand All @@ -146,6 +147,7 @@ class SynchronousCompositorHost : public SynchronousCompositor,
bool invalidate_needs_draw_;
uint32_t did_activate_pending_tree_count_;
uint32_t frame_metadata_version_ = 0u;
// Physical pixel when use-zoom-for-dsf is enabled, otherwise in dip.
gfx::ScrollOffset max_scroll_offset_;
gfx::SizeF scrollable_size_;
float page_scale_factor_ = 0.f;
Expand Down
2 changes: 2 additions & 0 deletions content/renderer/android/synchronous_compositor_proxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ class SynchronousCompositorProxy : public ui::SynchronousInputHandler,

// To browser.
uint32_t version_ = 0;
// |total_scroll_offset_| and |max_scroll_offset_| are in physical pixel when
// use-zoom-for-dsf is enabled, otherwise in dip.
gfx::ScrollOffset total_scroll_offset_; // Modified by both.
gfx::ScrollOffset max_scroll_offset_;
gfx::SizeF scrollable_size_;
Expand Down

0 comments on commit 76a886b

Please sign in to comment.