Skip to content

Commit

Permalink
Surface synchronization: Pass original ScreenInfo to OOPIF/BrowserPlugin
Browse files Browse the repository at this point in the history
In device emulation mode, the top level renderer devtools lies to Blink
about the size and scale factor by constructing an emulated ScreenInfo object.
Prior to this patch, we propagated that ScreenInfo down to OOPIFs and
BrowserPlugins. This caused double scaling of the compositor viewport size
in OOPIFs because the top level renderer had already scaled down the
surface embedded.

This CL instead plumbs down the original ScreenInfo which fixes the device
emulation problem.

Bug: 819903, 672962
Change-Id: Ie32c2798edcbe8738c463edf86d45c86f33d0d5b
Reviewed-on: https://chromium-review.googlesource.com/956554
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542130}
  • Loading branch information
Fady Samuel authored and Commit Bot committed Mar 9, 2018
1 parent afb450f commit a863f15
Show file tree
Hide file tree
Showing 9 changed files with 133 additions and 62 deletions.
8 changes: 5 additions & 3 deletions content/renderer/browser_plugin/browser_plugin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ gfx::Rect BrowserPlugin::FrameRectInPixels() const {
}

float BrowserPlugin::GetDeviceScaleFactor() const {
return embedding_render_widget_->GetOriginalDeviceScaleFactor();
return embedding_render_widget_->GetOriginalScreenInfo().device_scale_factor;
}

void BrowserPlugin::UpdateInternalInstanceId() {
Expand Down Expand Up @@ -461,7 +461,8 @@ bool BrowserPlugin::Initialize(WebPluginContainer* container) {
embedding_render_widget_ =
RenderFrameImpl::FromWebFrame(container_->GetDocument().GetFrame())
->GetRenderWidget();
pending_resize_params_.screen_info = embedding_render_widget_->screen_info();
pending_resize_params_.screen_info =
embedding_render_widget_->GetOriginalScreenInfo();
embedding_render_widget_->RegisterBrowserPlugin(this);

return true;
Expand Down Expand Up @@ -540,7 +541,8 @@ void BrowserPlugin::UpdateGeometry(const WebRect& plugin_rect_in_viewport,
}

pending_resize_params_.frame_rect = frame_rect;
pending_resize_params_.screen_info = embedding_render_widget_->screen_info();
pending_resize_params_.screen_info =
embedding_render_widget_->GetOriginalScreenInfo();
if (guest_crashed_) {
// Update the sad page to match the current ScreenInfo.
compositing_helper_->ChildFrameGone(frame_rect.size(),
Expand Down
2 changes: 1 addition & 1 deletion content/renderer/input/render_widget_input_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ viz::FrameSinkId RenderWidgetInputHandler::GetFrameSinkIdAtPoint(
gfx::PointF point_in_pixel(point);
if (IsUseZoomForDSFEnabled()) {
point_in_pixel = gfx::ConvertPointToPixel(
widget_->GetOriginalDeviceScaleFactor(), point_in_pixel);
widget_->GetOriginalScreenInfo().device_scale_factor, point_in_pixel);
}
blink::WebNode result_node = widget_->GetWebWidget()
->HitTestResultAt(blink::WebPoint(
Expand Down
9 changes: 7 additions & 2 deletions content/renderer/render_frame_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1202,9 +1202,12 @@ void RenderFrameImpl::CreateFrame(
CHECK(parent_routing_id != MSG_ROUTING_NONE || !web_frame->Parent());

if (widget_params.routing_id != MSG_ROUTING_NONE) {
// TODO(fsamuel): It's not clear if we should be passing in the
// web ScreenInfo or the original ScreenInfo here.
render_frame->render_widget_ = RenderWidget::CreateForFrame(
widget_params.routing_id, widget_params.hidden,
render_frame->render_view_->screen_info(), compositor_deps, web_frame);
render_frame->render_view_->GetWebScreenInfo(), compositor_deps,
web_frame);
}

if (has_committed_real_load)
Expand Down Expand Up @@ -1585,10 +1588,12 @@ RenderWidgetFullscreenPepper* RenderFrameImpl::CreatePepperFullscreenContainer(
base::Bind(&RenderViewImpl::ShowCreatedFullscreenWidget,
render_view()->GetWeakPtr());

// TODO(fsamuel): It's not clear if we should be passing in the
// web ScreenInfo or the original ScreenInfo here.
RenderWidgetFullscreenPepper* widget = RenderWidgetFullscreenPepper::Create(
fullscreen_widget_routing_id, show_callback,
GetRenderWidget()->compositor_deps(), plugin, active_url,
GetRenderWidget()->screen_info(), std::move(widget_channel_request));
GetRenderWidget()->GetWebScreenInfo(), std::move(widget_channel_request));
// TODO(nick): The show() handshake seems like unnecessary complexity here,
// since there's no real delay between CreateFullscreenWidget and
// ShowCreatedFullscreenWidget. Would it be simpler to have the
Expand Down
4 changes: 2 additions & 2 deletions content/renderer/render_frame_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ void RenderFrameProxy::Init(blink::WebRemoteFrame* web_frame,

compositing_helper_ = std::make_unique<ChildFrameCompositingHelper>(this);

pending_resize_params_.screen_info = render_widget_->screen_info();
pending_resize_params_.screen_info = render_widget_->GetOriginalScreenInfo();

#if defined(USE_AURA)
if (features::IsMusEnabled()) {
Expand Down Expand Up @@ -716,7 +716,7 @@ void RenderFrameProxy::FrameRectsChanged(
pending_resize_params_.screen_space_rect = gfx::Rect(screen_space_rect);
pending_resize_params_.local_frame_size =
gfx::Size(local_frame_rect.width, local_frame_rect.height);
pending_resize_params_.screen_info = render_widget_->screen_info();
pending_resize_params_.screen_info = render_widget_->GetOriginalScreenInfo();
if (crashed_) {
// Update the sad page to match the current size.
compositing_helper_->ChildFrameGone(local_frame_size(),
Expand Down
59 changes: 57 additions & 2 deletions content/renderer/render_view_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,8 @@ class RenderViewImplScaleFactorTest : public RenderViewImplBlinkSettingsTest {
params.needs_resize_ack = false;
params.content_source_id = view()->GetContentSourceId();
view()->OnResize(params);
ASSERT_EQ(dsf, view()->GetWebDeviceScaleFactor());
ASSERT_EQ(dsf, view()->GetOriginalDeviceScaleFactor());
ASSERT_EQ(dsf, view()->GetWebScreenInfo().device_scale_factor);
ASSERT_EQ(dsf, view()->GetOriginalScreenInfo().device_scale_factor);
}

void TestEmulatedSizeDprDsf(int width, int height, float dpr,
Expand Down Expand Up @@ -802,6 +802,61 @@ TEST_F(RenderViewImplTest, DecideNavigationPolicyForWebUI) {
CloseRenderView(new_view);
}

// This test verifies that when device emulation is enabled, RenderFrameProxy
// continues to receive the original ScreenInfo and not the emualted
// ScreenInfo.
TEST_F(RenderViewImplScaleFactorTest, DeviceEmulationWithOOPIF) {
DoSetUp();

// This test should only run with --site-per-process.
if (!AreAllSitesIsolatedForTesting())
return;

const float device_scale = 2.0f;
float compositor_dsf = IsUseZoomForDSFEnabled() ? 1.f : device_scale;
SetDeviceScaleFactor(device_scale);

LoadHTML(
"<body style='min-height:1000px;'>"
" <iframe src='data:text/html,frame 1'></iframe>"
"</body>");

WebFrame* web_frame = frame()->GetWebFrame();
ASSERT_TRUE(web_frame->FirstChild()->IsWebLocalFrame());
TestRenderFrame* child_frame = static_cast<TestRenderFrame*>(
RenderFrame::FromWebFrame(web_frame->FirstChild()->ToWebLocalFrame()));
ASSERT_TRUE(child_frame);

child_frame->SwapOut(kProxyRoutingId + 1, true,
ReconstructReplicationStateForTesting(child_frame));
EXPECT_TRUE(web_frame->FirstChild()->IsWebRemoteFrame());
RenderFrameProxy* child_proxy = RenderFrameProxy::FromWebFrame(
web_frame->FirstChild()->ToWebRemoteFrame());
ASSERT_TRUE(child_proxy);

// Verify that the system device scale factor has propagated into the
// RenderFrameProxy.
EXPECT_EQ(device_scale,
view()->GetWidget()->GetWebScreenInfo().device_scale_factor);
EXPECT_EQ(device_scale,
view()->GetWidget()->GetOriginalScreenInfo().device_scale_factor);
EXPECT_EQ(device_scale, child_proxy->screen_info().device_scale_factor);

TestEmulatedSizeDprDsf(640, 480, 3.f, compositor_dsf);

// Verify that the RenderFrameProxy device scale factor is still the same.
EXPECT_EQ(3.f, view()->GetWidget()->GetWebScreenInfo().device_scale_factor);
EXPECT_EQ(device_scale,
view()->GetWidget()->GetOriginalScreenInfo().device_scale_factor);
EXPECT_EQ(device_scale, child_proxy->screen_info().device_scale_factor);

view()->OnDisableDeviceEmulation();

blink::WebDeviceEmulationParams params;
view()->OnEnableDeviceEmulation(params);
// Don't disable here to test that emulation is being shutdown properly.
}

// Verify that security origins are replicated properly to RenderFrameProxies
// when swapping out.
TEST_F(RenderViewImplTest, OriginReplicationForSwapOut) {
Expand Down
26 changes: 16 additions & 10 deletions content/renderer/render_view_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -580,9 +580,10 @@ void RenderViewImpl::Initialize(
main_render_frame_ = RenderFrameImpl::CreateMainFrame(
this, params->main_frame_routing_id,
std::move(main_frame_interface_provider),
params->main_frame_widget_routing_id, params->hidden, screen_info(),
compositor_deps_, opener_frame, params->devtools_main_frame_token,
params->replicated_frame_state, params->has_committed_real_load);
params->main_frame_widget_routing_id, params->hidden,
GetWebScreenInfo(), compositor_deps_, opener_frame,
params->devtools_main_frame_token, params->replicated_frame_state,
params->has_committed_real_load);
}

// TODO(dcheng): Shouldn't these be mutually exclusive at this point? See
Expand Down Expand Up @@ -1824,7 +1825,7 @@ gfx::Size RenderViewImpl::GetSize() const {
}

float RenderViewImpl::GetDeviceScaleFactor() const {
return GetWebDeviceScaleFactor();
return GetWebScreenInfo().device_scale_factor;
}

float RenderViewImpl::GetZoomLevel() const {
Expand Down Expand Up @@ -1914,8 +1915,10 @@ void RenderViewImpl::OnEnableAutoResize(const gfx::Size& min_size,

if (IsUseZoomForDSFEnabled()) {
webview()->EnableAutoResizeMode(
gfx::ScaleToCeiledSize(min_size, GetWebDeviceScaleFactor()),
gfx::ScaleToCeiledSize(max_size, GetWebDeviceScaleFactor()));
gfx::ScaleToCeiledSize(min_size,
GetWebScreenInfo().device_scale_factor),
gfx::ScaleToCeiledSize(max_size,
GetWebScreenInfo().device_scale_factor));
} else {
webview()->EnableAutoResizeMode(min_size, max_size);
}
Expand Down Expand Up @@ -1964,8 +1967,10 @@ void RenderViewImpl::OnSetLocalSurfaceIdForAutoResize(

if (IsUseZoomForDSFEnabled()) {
webview()->EnableAutoResizeMode(
gfx::ScaleToCeiledSize(min_size, GetWebDeviceScaleFactor()),
gfx::ScaleToCeiledSize(max_size, GetWebDeviceScaleFactor()));
gfx::ScaleToCeiledSize(min_size,
GetWebScreenInfo().device_scale_factor),
gfx::ScaleToCeiledSize(max_size,
GetWebScreenInfo().device_scale_factor));
} else {
webview()->EnableAutoResizeMode(min_size, max_size);
}
Expand Down Expand Up @@ -2295,7 +2300,8 @@ bool RenderViewImpl::DidTapMultipleTargets(

// The touch_rect, target_rects and zoom_rect are in the outer viewport
// reference frame.
float to_pix = IsUseZoomForDSFEnabled() ? 1 : GetWebDeviceScaleFactor();
float to_pix =
IsUseZoomForDSFEnabled() ? 1 : GetWebScreenInfo().device_scale_factor;
gfx::Rect zoom_rect;
float new_total_scale =
DisambiguationPopupHelper::ComputeZoomAreaAndScaleFactor(
Expand Down Expand Up @@ -2338,7 +2344,7 @@ bool RenderViewImpl::DidTapMultipleTargets(
// device scale will be applied in WebKit
// --> zoom_rect doesn't include device scale,
// but WebKit will still draw on zoom_rect *
// GetWebDeviceScaleFactor()
// GetWebScreenInfo().device_scale_factor
canvas.scale(new_total_scale / to_pix, new_total_scale / to_pix);
canvas.translate(-zoom_rect.x() * to_pix, -zoom_rect.y() * to_pix);

Expand Down
2 changes: 2 additions & 0 deletions content/renderer/render_view_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,8 @@ class CONTENT_EXPORT RenderViewImpl : public RenderWidget,
ScreenMetricsEmulationWithOriginalDSF1);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplScaleFactorTest,
ScreenMetricsEmulationWithOriginalDSF2);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplScaleFactorTest,
DeviceEmulationWithOOPIF);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest,
DecideNavigationPolicyHandlesAllTopLevel);
#if defined(OS_MACOSX)
Expand Down
Loading

0 comments on commit a863f15

Please sign in to comment.