[DevTools] Use device metrics emulation implemented in content.

[DevTools] Use device metrics emulation implemented in content.

Device emulation is now split: while metrics emulation is simply forwarded
to embedder, a number of internal blink settings are set to emulate device
behavior. These settings include overlay scrollbars, viewport support, text
autosizing and page scaling.

To support fit-to-window mode, WebView now sets a scaling transform
to the root graphics layer. Also, device scale factor is overriden to produce
sharp image even when emulating.

This also removes old device metrics emulation support through blink code.

See also Chromium-side patch: https://codereview.chromium.org/23364004/

BUG=288959
R=aelias@chromium.org, pfeldman@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159110

[dgozman, 2013-09-19 12:27:35]

Pavel, take a look please.

[pfeldman, 2013-09-19 12:46:05]

https://codereview.chromium.org/23187005/diff/15001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/23187005/diff/15001/Source/core/inspector/Ins...
Source/core/inspector/InspectorPageAgent.cpp:970: // FIXME: cleanup all
references to page agent overrides.
Do it right away!

https://codereview.chromium.org/23187005/diff/15001/Source/core/inspector/Ins...
Source/core/inspector/InspectorPageAgent.cpp:982: // FIXME: cleanup all
references to page agent overrides.
ditto

https://codereview.chromium.org/23187005/diff/15001/Source/core/platform/Scro...
File Source/core/platform/ScrollbarThemeMacCommon.mm (right):

https://codereview.chromium.org/23187005/diff/15001/Source/core/platform/Scro...
Source/core/platform/ScrollbarThemeMacCommon.mm:132: if
(RuntimeEnabledFeatures::overlayScrollbarsEnabled()) {
Lets land this separately.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
File Source/web/WebDevToolsAgentImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:193: // FIXME: remove DeviceMetricsSupport.
Do it right away.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:365: , m_deviceMetricsEnabled(false)
You need to initialize all booleans here.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:456: // FIXME: cleanup usages of this
function and remove it.
ditto

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:399: , m_emulatingDeviceScales(false)
Initialize floats too.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3829: updateRootLayerTransform();
Will this work?:

if (m_emulatingDeviceScales)
    updateRootLayerTransform();

So make sure we don't set identity transform when it is not needed?

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:4025: if (!page() || !m_layerTreeView)
Assert these exist instead.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:730: bool m_emulatingDeviceScales;
Lets assume m_emulatingDeviceScales is m_emulatedDeviceScaleFactor != 0.

https://codereview.chromium.org/23187005/diff/15001/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/23187005/diff/15001/public/web/WebViewClient....
public/web/WebViewClient.h:155: // Called to emulate device. Window should
occupy the whole device screen,
// Called to emulate device dimensions and scale factor. Window should ... Also
explain what fitToView means.

[apavlov, 2013-09-19 12:47:57]

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
File Source/web/WebDevToolsAgentImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:510:
m_webViewImpl->setPageScaleFactorLimits(1.f, 1.f);
Is the float literal notation really required here and below? I've seen it
disproved whenever implicit casts work...

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:525: // FIXME: save and restore next two
values.
Guess this comment should note that this should be done when enableViewport is
switched on.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:528:
m_webViewImpl->page()->settings().setTextAutosizingEnabled(true);
This one is dubious. Text autosizing should be enabled only when emulating
Chrome. Blackberry, Android 2.3, and some other devices do not have Chrome.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:4029: if (m_emulatingDeviceScales)
float deviceScaleFactor = m_emulatingDeviceScales ? m_emulatedDeviceScaleFactor
: page()->deviceScaleFactor();

https://codereview.chromium.org/23187005/diff/15001/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/23187005/diff/15001/public/web/WebViewClient....
public/web/WebViewClient.h:158: bool enabled, const WebSize& deviceSize, const
WebRect& viewRect,
perhaps, deviceSize -> screenSize?

[dgozman, 2013-09-19 13:59:37]

PTAL

https://codereview.chromium.org/23187005/diff/15001/Source/core/platform/Scro...
File Source/core/platform/ScrollbarThemeMacCommon.mm (right):

https://codereview.chromium.org/23187005/diff/15001/Source/core/platform/Scro...
Source/core/platform/ScrollbarThemeMacCommon.mm:132: if
(RuntimeEnabledFeatures::overlayScrollbarsEnabled()) {
On 2013/09/19 12:46:05, pfeldman wrote:
> Lets land this separately.

Done.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
File Source/web/WebDevToolsAgentImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:365: , m_deviceMetricsEnabled(false)
On 2013/09/19 12:46:05, pfeldman wrote:
> You need to initialize all booleans here.

Done.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:510:
m_webViewImpl->setPageScaleFactorLimits(1.f, 1.f);
On 2013/09/19 12:47:57, apavlov wrote:
> Is the float literal notation really required here and below? I've seen it
> disproved whenever implicit casts work...

Done.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:525: // FIXME: save and restore next two
values.
On 2013/09/19 12:47:57, apavlov wrote:
> Guess this comment should note that this should be done when enableViewport is
> switched on.

Nice catch! Thanks.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:528:
m_webViewImpl->page()->settings().setTextAutosizingEnabled(true);
On 2013/09/19 12:47:57, apavlov wrote:
> This one is dubious. Text autosizing should be enabled only when emulating
> Chrome. Blackberry, Android 2.3, and some other devices do not have Chrome.

Safari, Firefox and IE has this functionality. Added a TODO to sort things out
when enabling viewport.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:399: , m_emulatingDeviceScales(false)
On 2013/09/19 12:46:05, pfeldman wrote:
> Initialize floats too.

Done.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3829: updateRootLayerTransform();
On 2013/09/19 12:46:05, pfeldman wrote:
> Will this work?:
> 
> if (m_emulatingDeviceScales)
>     updateRootLayerTransform();
> 
> So make sure we don't set identity transform when it is not needed?

What if root layer has transform left from previous emulation? We need to
restore to identity.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:4025: if (!page() || !m_layerTreeView)
On 2013/09/19 12:46:05, pfeldman wrote:
> Assert these exist instead.

Done.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:4029: if (m_emulatingDeviceScales)
On 2013/09/19 12:47:57, apavlov wrote:
> float deviceScaleFactor = m_emulatingDeviceScales ?
m_emulatedDeviceScaleFactor
> : page()->deviceScaleFactor();

Done.

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/23187005/diff/15001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:730: bool m_emulatingDeviceScales;
On 2013/09/19 12:46:05, pfeldman wrote:
> Lets assume m_emulatingDeviceScales is m_emulatedDeviceScaleFactor != 0.

Done.

https://codereview.chromium.org/23187005/diff/15001/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/23187005/diff/15001/public/web/WebViewClient....
public/web/WebViewClient.h:155: // Called to emulate device. Window should
occupy the whole device screen,
On 2013/09/19 12:46:05, pfeldman wrote:
> // Called to emulate device dimensions and scale factor. Window should ...
Also
> explain what fitToView means.

Done.

https://codereview.chromium.org/23187005/diff/15001/public/web/WebViewClient....
public/web/WebViewClient.h:158: bool enabled, const WebSize& deviceSize, const
WebRect& viewRect,
On 2013/09/19 12:47:57, apavlov wrote:
> perhaps, deviceSize -> screenSize?

Done.

[dgozman, 2013-09-27 19:10:11]

Please take another look. Moved input events scaling from Content to Blink.

[pfeldman, 2013-09-30 12:42:38]

lgtm with a naming nit.

https://codereview.chromium.org/23187005/diff/73001/Source/core/page/FrameView.h
File Source/core/page/FrameView.h (right):

https://codereview.chromium.org/23187005/diff/73001/Source/core/page/FrameVie...
Source/core/page/FrameView.h:158: virtual float inputEventsScaleFactor() const
OVERRIDE { return visibleContentScaleFactor() *
m_inputEventsScaleFactorForEmulation; }
Move definition into .cpp

https://codereview.chromium.org/23187005/diff/73001/Source/web/WebInputEventC...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/23187005/diff/73001/Source/web/WebInputEventC...
Source/web/WebInputEventConversion.cpp:66: return
rootView->inputEventsScaleFactor();
widgetScaleFactor migrating from visibleContentScaleFactor to
inputEventsScaleFactor sounds misleading. Which of the names needs to be
changed?

https://codereview.chromium.org/23187005/diff/73001/public/web/WebDevToolsAge...
File public/web/WebDevToolsAgentClient.h (right):

https://codereview.chromium.org/23187005/diff/73001/public/web/WebDevToolsAge...
public/web/WebDevToolsAgentClient.h:88: // Called to emulate device dimensions
and scale factor. Window should
Called to emulate device dimensions, scale and input. Even though input is not
there yet, it will be (and hence using "device" is justified).

[aelias_OOO_until_Jul13, 2013-10-01 07:14:15]

Sorry, could you reupload your last patch?  Rietveld is refusing to show the
latest changes.

[dgozman, 2013-10-01 07:20:09]

On 2013/10/01 07:14:15, aelias wrote:
> Sorry, could you reupload your last patch?  Rietveld is refusing to show the
> latest changes.

Done.

[aelias_OOO_until_Jul13, 2013-10-01 08:08:51]

lgtm modulo comments below.

https://codereview.chromium.org/23187005/diff/87001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23187005/diff/87001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:2564: updateScrollbars(scrollOffset());
Are you sure you need this updateScrollbars()?

https://codereview.chromium.org/23187005/diff/87001/Source/core/page/Settings.in
File Source/core/page/Settings.in (right):

https://codereview.chromium.org/23187005/diff/87001/Source/core/page/Settings...
Source/core/page/Settings.in:154: rootLayerClipsContent initial=true
If your Blink patch lands first, it will break Android until the Chromium side
lands (Android requires the clip to be disabled, other platforms don't care one
way or the other).  To avoid that situation I recommend setting the default to
false, and set it to true on non-Android on the Chromium side.

I also recommend setting the layout test expectations I touched in
https://chromiumcodereview.appspot.com/19019004 as NeedsManualRebaseline, as
they'll break again when the setting goes to true on desktop platforms.

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebDevToolsAge...
File Source/web/WebDevToolsAgentImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:319: // FIXME: enable viewport after UI
problems with pageScaleFactor will be fixed.
I don't think this should land with dead code, it could be a maintenance burden.
 Please delete the code and bring it back when you're ready to enable.

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:403: , m_rootLayerScale(0)
Let's make rootLayerScale default to 1 instead.

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3765: void
WebViewImpl::setDeviceEmulationParameters(bool enabled, float deviceScaleFactor,
float rootLayerScale)
The other args are ignored when enabled = false which seems a bit confusing. 
How about just removing the bool and let the caller pass "0, 1" values to
disable?

https://codereview.chromium.org/23187005/diff/87001/public/web/WebDevToolsAge...
File public/web/WebDevToolsAgentClient.h (right):

https://codereview.chromium.org/23187005/diff/87001/public/web/WebDevToolsAge...
public/web/WebDevToolsAgentClient.h:93: virtual void emulateDevice(
Please split into two methods:

virtual void enableScreenMetricsEmulation(const WebSize&...) { }
virtual void disableScreenMetricsEmulation() { }

https://codereview.chromium.org/23187005/diff/87001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/23187005/diff/87001/public/web/WebView.h#newc...
public/web/WebView.h:403: virtual void setDeviceEmulationParameters(
You're introducing a second Blink API to bounce back some information that Blink
could've computed in the first place.  Let's avoid this criss-crossing
dependency.

The new information you're getting back through here is the rootLayerScale which
is calculated by fit_to_view.  But you have enough information to do the
fit_to_view calculation on the Blink side.  Please move that calculation to
Blink and then tell both RenderView and WebViewImpl about the scale you
calculated, and delete this WebView.h change.

[aelias_OOO_until_Jul13, 2013-10-01 08:24:22]

https://codereview.chromium.org/23187005/diff/87001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/23187005/diff/87001/public/web/WebView.h#newc...
public/web/WebView.h:403: virtual void setDeviceEmulationParameters(
On 2013/10/01 08:08:51, aelias wrote:
> You're introducing a second Blink API to bounce back some information that
Blink
> could've computed in the first place.  Let's avoid this criss-crossing
> dependency.

As discussed offline, it can't be eliminated, so let's just split into two
narrowly named methods for clarity:

setCompositorDeviceScaleFactorOverride()
setRootLayerScaleTransform()

[dgozman, 2013-10-01 15:26:34]

https://codereview.chromium.org/23187005/diff/73001/Source/core/page/FrameView.h
File Source/core/page/FrameView.h (right):

https://codereview.chromium.org/23187005/diff/73001/Source/core/page/FrameVie...
Source/core/page/FrameView.h:158: virtual float inputEventsScaleFactor() const
OVERRIDE { return visibleContentScaleFactor() *
m_inputEventsScaleFactorForEmulation; }
On 2013/09/30 12:42:38, pfeldman wrote:
> Move definition into .cpp

Done.

https://codereview.chromium.org/23187005/diff/73001/Source/web/WebInputEventC...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/23187005/diff/73001/Source/web/WebInputEventC...
Source/web/WebInputEventConversion.cpp:66: return
rootView->inputEventsScaleFactor();
On 2013/09/30 12:42:38, pfeldman wrote:
> widgetScaleFactor migrating from visibleContentScaleFactor to
> inputEventsScaleFactor sounds misleading. Which of the names needs to be
> changed?

Renamed this method to |widgetInputEventsScaleFactor|.

https://codereview.chromium.org/23187005/diff/73001/public/web/WebDevToolsAge...
File public/web/WebDevToolsAgentClient.h (right):

https://codereview.chromium.org/23187005/diff/73001/public/web/WebDevToolsAge...
public/web/WebDevToolsAgentClient.h:88: // Called to emulate device dimensions
and scale factor. Window should
On 2013/09/30 12:42:38, pfeldman wrote:
> Called to emulate device dimensions, scale and input. Even though input is not
> there yet, it will be (and hence using "device" is justified).

Done.

https://codereview.chromium.org/23187005/diff/87001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23187005/diff/87001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:2564: updateScrollbars(scrollOffset());
On 2013/10/01 08:08:51, aelias wrote:
> Are you sure you need this updateScrollbars()?

Not really. Removed.

https://codereview.chromium.org/23187005/diff/87001/Source/core/page/Settings.in
File Source/core/page/Settings.in (right):

https://codereview.chromium.org/23187005/diff/87001/Source/core/page/Settings...
Source/core/page/Settings.in:154: rootLayerClipsContent initial=true
On 2013/10/01 08:08:51, aelias wrote:
> If your Blink patch lands first, it will break Android until the Chromium side
> lands (Android requires the clip to be disabled, other platforms don't care
one
> way or the other).  To avoid that situation I recommend setting the default to
> false, and set it to true on non-Android on the Chromium side.
> 
> I also recommend setting the layout test expectations I touched in
> https://chromiumcodereview.appspot.com/19019004 as NeedsManualRebaseline, as
> they'll break again when the setting goes to true on desktop platforms.

I will first land API change (https://codereview.chromium.org/24272003/), with
setting introduced there.
Then chromium-side change, which will set it to false on Android. And only after
that this change, which uses it.

Thank for layout test expectations tip!

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebDevToolsAge...
File Source/web/WebDevToolsAgentImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebDevToolsAge...
Source/web/WebDevToolsAgentImpl.cpp:319: // FIXME: enable viewport after UI
problems with pageScaleFactor will be fixed.
On 2013/10/01 08:08:51, aelias wrote:
> I don't think this should land with dead code, it could be a maintenance
burden.
>  Please delete the code and bring it back when you're ready to enable.

Done.

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23187005/diff/87001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:403: , m_rootLayerScale(0)
On 2013/10/01 08:08:51, aelias wrote:
> Let's make rootLayerScale default to 1 instead.

Done.

https://codereview.chromium.org/23187005/diff/87001/public/web/WebDevToolsAge...
File public/web/WebDevToolsAgentClient.h (right):

https://codereview.chromium.org/23187005/diff/87001/public/web/WebDevToolsAge...
public/web/WebDevToolsAgentClient.h:93: virtual void emulateDevice(
On 2013/10/01 08:08:51, aelias wrote:
> Please split into two methods:
> 
> virtual void enableScreenMetricsEmulation(const WebSize&...) { }
> virtual void disableScreenMetricsEmulation() { }

This method will also emulate device input in the near future. So, using
"device" instead of "screen metrics" here seems to be the right choice. I'd
split this into:
virtual void enableDeviceEmulation(const WebSize&...) { }
virtual void disableDeviceEmulation() { }

Comment updated to mention the device-like input.

https://codereview.chromium.org/23187005/diff/87001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/23187005/diff/87001/public/web/WebView.h#newc...
public/web/WebView.h:403: virtual void setDeviceEmulationParameters(
On 2013/10/01 08:24:22, aelias wrote:
> On 2013/10/01 08:08:51, aelias wrote:
> > You're introducing a second Blink API to bounce back some information that
> Blink
> > could've computed in the first place.  Let's avoid this criss-crossing
> > dependency.
> 
> As discussed offline, it can't be eliminated, so let's just split into two
> narrowly named methods for clarity:
> 
> setCompositorDeviceScaleFactorOverride()
> setRootLayerScaleTransform()

Done.

[dgozman, 2013-10-03 15:32:28]

Pavel, please take a look. I've changed layout tests and moved reload to
frontend.

[pfeldman, 2013-10-03 15:38:45]

Changes to test look good!

[commit-bot: I haz the power, 2013-10-08 06:33:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/23187005/145001

[commit-bot: I haz the power, 2013-10-08 06:50:48]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[dgozman, 2013-10-08 09:30:34]

Committed patchset #21 manually as r159110 (presubmit successful).