Enabled using viewport on desktop browsers behind experimental flag

Enabled using viewport on desktop browsers behind experimental flag
'enable-desktop-viewport'. The changes shouldn't affect anything if the 
flag isn't set. 

This is the Blink side of a two part patch

BUG=232102

[Rick Byers, 2013-06-04 00:25:30]

Nice, thanks David - I'm happy that you got the viewport tag working on desktop
with just these few tweaks!

Here's some thoughts on how we might be able to clean this up a bit so we can
land it.  Obviously we'll also need to add/enable some tests too, but we might
want to run the code by aelias first to try to uncover any major issues (since
we're still not 100% sure we can land something here, and even less sure we'll
ever enable the flag by default).

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/public...
File Source/WebKit/chromium/public/WebSettings.h (right):

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/public...
Source/WebKit/chromium/public/WebSettings.h:55: virtual bool
desktopViewportEnabled() const = 0;
how does the semantics of this differ from viewportEnabled?  I.e. would you
expect ChromeOS to set all 3 of these flag (viewportEnabled,
desktopViewportEnabled AND deviceSupportsRotation) or some subset of them?

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1603: &&
settings()->deviceSupportsRotation() && oldSize.width && oldContentsWidth;
Note that ChromeOS does support rotation (Ctrl-Shift-F3).  From aelias's comment
in the bug I gather this code assumes that with viewport enabled, resizes ONLY
come from rotation.  Perhaps you want a different 'has resizable windows'
setting?  Also please add a comment describing what's going on here - I was
totally lost before reading the bug comments.

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1615: setFixedLayoutSize(newSize);
Shouldn't we also be updating the size on resize under width=device-width (since
device-width means window width on desktop)?

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1615: setFixedLayoutSize(newSize);
Shouldn't we also do this in the ValueDeviceWidth case (since width=device-width
is the same as width=auto on desktop)?

Do we really need a separate desktopViewPortEnabled setting, or could we
accomplish all the same things with 'viewportEnabled() &&
supportsResizableWindows()'?  The overall goal of this CL is to reduce the ways
the mobile web platform and desktop web platform have diverged since the line is
just becoming blurrier :-)

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:3010:
m_pageScaleConstraintsSet.updatePageDefinedConstraints(arguments, m_size,
settings()->desktopViewportEnabled() ? 0 :
page()->settings()->layoutFallbackWidth());
Rather than change the code here, can't we change the implementation of
layoutFallbackWidth?  Again it could be keyed off of whether we have resizable
windows or not...

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:3597: if ((*isNewNavigation &&
!isNavigationWithinPage) || settings()->desktopViewportEnabled())
Why is this different for android/desktop?

[bokan, 2013-06-04 18:52:16]

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/public...
File Source/WebKit/chromium/public/WebSettings.h (right):

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/public...
Source/WebKit/chromium/public/WebSettings.h:55: virtual bool
desktopViewportEnabled() const = 0;
On 2013/06/04 00:25:30, Rick Byers wrote:
> how does the semantics of this differ from viewportEnabled?  I.e. would you
> expect ChromeOS to set all 3 of these flag (viewportEnabled,
> desktopViewportEnabled AND deviceSupportsRotation) or some subset of them?
desktopViewportEnabled turns on viewportEnabled but all the layout resizing I
added is behind desktopViewportEnabled so that none of the Android behaviour
changes. I didn't realize that rotation happened on anything but Android. I'm
not sure what we want to do with rotation on CrOS but if we want to support
rotation behaviour like Android (so that the viewport gets appropriately
rescaled) we'll need some additional signal that a resize came from a rotation.
Enabling rotation on CrOS right now means resizing a window causes inappropriate
scaling rather than creating scrollbars.

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1603: &&
settings()->deviceSupportsRotation() && oldSize.width && oldContentsWidth;
On 2013/06/04 00:25:30, Rick Byers wrote:
> Note that ChromeOS does support rotation (Ctrl-Shift-F3).  From aelias's
comment
> in the bug I gather this code assumes that with viewport enabled, resizes ONLY
> come from rotation.  Perhaps you want a different 'has resizable windows'
> setting?  Also please add a comment describing what's going on here - I was
> totally lost before reading the bug comments. 
Ah, resizableWindow makes more sense then - I'll change it. I assume in that
case we don't want to have Android-like behaviour when the screen's rotated on
CrOS? (i.e. rescale the viewport).

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1615: setFixedLayoutSize(newSize);
On 2013/06/04 00:25:30, Rick Byers wrote:
> Shouldn't we also be updating the size on resize under width=device-width
(since
> device-width means window width on desktop)?
That happens in the method call above, along with other handling of the viewport
tag. If there was no viewport width set (which is ValueAuto) then we want to set
the layout size to the window so that we emulate the non-viewport case.

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1615: setFixedLayoutSize(newSize);
On 2013/06/04 00:25:30, Rick Byers wrote:
> Shouldn't we also do this in the ValueDeviceWidth case (since
width=device-width
> is the same as width=auto on desktop)?
> 
> Do we really need a separate desktopViewPortEnabled setting, or could we
> accomplish all the same things with 'viewportEnabled() &&
> supportsResizableWindows()'?  The overall goal of this CL is to reduce the
ways
> the mobile web platform and desktop web platform have diverged since the line
is
> just becoming blurrier :-)

Agreed, and I originally had everything behind viewportEnabled, but I don't know
how Android behaves in cases like these (e.g What would happen here when we set
layout size?) and I currently have no way to test it. How difficult/practical is
it to setup an Android build and device?

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:3010:
m_pageScaleConstraintsSet.updatePageDefinedConstraints(arguments, m_size,
settings()->desktopViewportEnabled() ? 0 :
page()->settings()->layoutFallbackWidth());
On 2013/06/04 00:25:30, Rick Byers wrote:
> Rather than change the code here, can't we change the implementation of
> layoutFallbackWidth?  Again it could be keyed off of whether we have resizable
> windows or not...
Actually, I already set fallback width to 0 if the setting is on so this is
redundant. Removed.

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:3597: if ((*isNewNavigation &&
!isNavigationWithinPage) || settings()->desktopViewportEnabled())
On 2013/06/04 00:25:30, Rick Byers wrote:
> Why is this different for android/desktop?
So I'm not sure why, but sometimes when I'd reload a page it's scale factor
would get reset to default, even though the page scale variable was still set so
the viewport would fill the window. Adding this makes it reload that scale. I'll
have to ask aelias@ about what's the appropriate thing to do here.

[Rick Byers, 2013-06-05 15:04:08]

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/public...
File Source/WebKit/chromium/public/WebSettings.h (right):

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/public...
Source/WebKit/chromium/public/WebSettings.h:55: virtual bool
desktopViewportEnabled() const = 0;
On 2013/06/04 18:52:16, bokan wrote:
> On 2013/06/04 00:25:30, Rick Byers wrote:
> > how does the semantics of this differ from viewportEnabled?  I.e. would you
> > expect ChromeOS to set all 3 of these flag (viewportEnabled,
> > desktopViewportEnabled AND deviceSupportsRotation) or some subset of them?
> desktopViewportEnabled turns on viewportEnabled but all the layout resizing I
> added is behind desktopViewportEnabled so that none of the Android behaviour
> changes. I didn't realize that rotation happened on anything but Android. I'm
> not sure what we want to do with rotation on CrOS but if we want to support
> rotation behaviour like Android (so that the viewport gets appropriately
> rescaled) we'll need some additional signal that a resize came from a
rotation.

I think it's fine to treat rotation the same as normal window resize on CrOS. 
So I think this is mostly about picking better names for the setting.  Try to
focus the settings on the behavior that's affected, as opposed to properties of
the embedder.  Maybe it's 'resizePreservesViewportWidth' or something like that?

> Enabling rotation on CrOS right now means resizing a window causes
inappropriate
> scaling rather than creating scrollbars.

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1603: &&
settings()->deviceSupportsRotation() && oldSize.width && oldContentsWidth;
On 2013/06/04 18:52:16, bokan wrote:
> On 2013/06/04 00:25:30, Rick Byers wrote:
> > Note that ChromeOS does support rotation (Ctrl-Shift-F3).  From aelias's
> comment
> > in the bug I gather this code assumes that with viewport enabled, resizes
ONLY
> > come from rotation.  Perhaps you want a different 'has resizable windows'
> > setting?  Also please add a comment describing what's going on here - I was
> > totally lost before reading the bug comments. 
> Ah, resizableWindow makes more sense then - I'll change it. I assume in that
> case we don't want to have Android-like behaviour when the screen's rotated on
> CrOS? (i.e. rescale the viewport).

Right.

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1615: setFixedLayoutSize(newSize);
On 2013/06/04 18:52:16, bokan wrote:
> On 2013/06/04 00:25:30, Rick Byers wrote:
> > Shouldn't we also be updating the size on resize under width=device-width
> (since
> > device-width means window width on desktop)?
> That happens in the method call above, along with other handling of the
viewport
> tag. If there was no viewport width set (which is ValueAuto) then we want to
set
> the layout size to the window so that we emulate the non-viewport case. 

I think this logic belongs inside of updatePageDefinedPageScaleConstraints then.
 See crbug.com/229151 with some debate on exactly how this should happen, but
for now I think it's right to rely on a setting (just one named more precisely -
eg. layoutFallbackWidthIsWindowWidth or something).

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1615: setFixedLayoutSize(newSize);
On 2013/06/04 18:52:16, bokan wrote:
> On 2013/06/04 00:25:30, Rick Byers wrote:
> > Shouldn't we also do this in the ValueDeviceWidth case (since
> width=device-width
> > is the same as width=auto on desktop)?
> > 
> > Do we really need a separate desktopViewPortEnabled setting, or could we
> > accomplish all the same things with 'viewportEnabled() &&
> > supportsResizableWindows()'?  The overall goal of this CL is to reduce the
> ways
> > the mobile web platform and desktop web platform have diverged since the
line
> is
> > just becoming blurrier :-)
> 
> Agreed, and I originally had everything behind viewportEnabled, but I don't
know
> how Android behaves in cases like these (e.g What would happen here when we
set
> layout size?) and I currently have no way to test it. How difficult/practical
is
> it to setup an Android build and device? 

You should be able to build content shell easily enough (which should be
sufficient here), see
https://code.google.com/p/chromium/wiki/AndroidBuildInstructions.

But being extra careful not to modify android behavior is a good idea.  Again
this is probably just about defining the settings to clearly indicate possible
differences between desktop and mobile (what the fallback width is and how
resize is handled).  In follow up work we can try to unify behavior and remove
the setting.

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:3597: if ((*isNewNavigation &&
!isNavigationWithinPage) || settings()->desktopViewportEnabled())
On 2013/06/04 18:52:16, bokan wrote:
> On 2013/06/04 00:25:30, Rick Byers wrote:
> > Why is this different for android/desktop?
> So I'm not sure why, but sometimes when I'd reload a page it's scale factor
> would get reset to default, even though the page scale variable was still set
so
> the viewport would fill the window. Adding this makes it reload that scale.
I'll
> have to ask aelias@ about what's the appropriate thing to do here.

Ok.  I'm OK with a temporary hack (eg. relying on a setting or perhaps even a
platform ifdef) in order to get this code landed since we don't even know yet if
it'll last.  Just be sure to call out the issue in a FIXME comment and reference
a bug to track resolving it.  

But yes, better yet, hopefully aelias can just help you figure out the right
thing to do.

[bokan, 2013-06-06 19:43:47]

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/public...
File Source/WebKit/chromium/public/WebSettings.h (right):

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/public...
Source/WebKit/chromium/public/WebSettings.h:55: virtual bool
desktopViewportEnabled() const = 0;
On 2013/06/05 15:04:08, Rick Byers wrote:
> On 2013/06/04 18:52:16, bokan wrote:
> > On 2013/06/04 00:25:30, Rick Byers wrote:
> > > how does the semantics of this differ from viewportEnabled?  I.e. would
you
> > > expect ChromeOS to set all 3 of these flag (viewportEnabled,
> > > desktopViewportEnabled AND deviceSupportsRotation) or some subset of them?
> > desktopViewportEnabled turns on viewportEnabled but all the layout resizing
I
> > added is behind desktopViewportEnabled so that none of the Android behaviour
> > changes. I didn't realize that rotation happened on anything but Android.
I'm
> > not sure what we want to do with rotation on CrOS but if we want to support
> > rotation behaviour like Android (so that the viewport gets appropriately
> > rescaled) we'll need some additional signal that a resize came from a
> rotation.
> 
> I think it's fine to treat rotation the same as normal window resize on CrOS. 
> So I think this is mostly about picking better names for the setting.  Try to
> focus the settings on the behavior that's affected, as opposed to properties
of
> the embedder.  Maybe it's 'resizePreservesViewportWidth' or something like
that?
> 
> > Enabling rotation on CrOS right now means resizing a window causes
> inappropriate
> > scaling rather than creating scrollbars.
> 
Changed it to resizeAffectsLayoutWidth

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/15927026/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebViewImpl.cpp:1615: setFixedLayoutSize(newSize);
On 2013/06/05 15:04:08, Rick Byers wrote:
> On 2013/06/04 18:52:16, bokan wrote:
> > On 2013/06/04 00:25:30, Rick Byers wrote:
> > > Shouldn't we also be updating the size on resize under width=device-width
> > (since
> > > device-width means window width on desktop)?
> > That happens in the method call above, along with other handling of the
> viewport
> > tag. If there was no viewport width set (which is ValueAuto) then we want to
> set
> > the layout size to the window so that we emulate the non-viewport case. 
> 
> I think this logic belongs inside of updatePageDefinedPageScaleConstraints
then.
>  See crbug.com/229151 with some debate on exactly how this should happen, but
> for now I think it's right to rely on a setting (just one named more precisely
-
> eg. layoutFallbackWidthIsWindowWidth or something). 
Done.

[aelias_OOO_until_Jul13, 2013-06-18 01:24:18]

This is on the right track, but let's take it a step further to make the
settings less messy:

1. Let's abolish the concept of "fixed layout mode" and delete everything in
Blink that refers to it (not that many things).  Replace it with a method
"FrameView::setLayoutSize" that is called on every WebViewImpl::resize on
desktop.

2. Instead of "layoutFallbackWidthIsWindowWidth", replace layoutFallbackWidth
with an integer "defaultLayoutWidth" set from Chromium-side.  It would be 980 on
mobile and 0 on desktop.  Then tie your desktop-specific behavior to the value
0.

https://codereview.chromium.org/15927026/diff/9001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebSettingsImpl.h (right):

https://codereview.chromium.org/15927026/diff/9001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebSettingsImpl.h:89: virtual void
setResizeAffectsLayoutWidth(bool);
This is poorly named, since resize affects layout width in desktop browsers as
well.  Maybe call it "anchorScrollPositionOnResize"?

https://codereview.chromium.org/15927026/diff/9001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/15927026/diff/9001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:1609: // adjust the viewport size so
that we don't cause a relayout
This comment doesn't accurately describe what this is doing: it isn't adjusting
the viewport size or avoiding any relayout.  We're adjusting the scroll position
and page scale to keep content onscreen.

https://codereview.chromium.org/15927026/diff/9001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:3621: // this is a hack to fix some
page scaling issues when reloading a page. Why is this needed?
Instead of doing this, try calling setInitialPageScaleOverride(1) from the
Chromium side and see if that fixes your problem.

[aelias_OOO_until_Jul13, 2013-06-18 01:30:19]

https://codereview.chromium.org/15927026/diff/9001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/15927026/diff/9001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:3621: // this is a hack to fix some
page scaling issues when reloading a page. Why is this needed?
On 2013/06/18 01:24:18, aelias wrote:
> Instead of doing this, try calling setInitialPageScaleOverride(1) from the
> Chromium side and see if that fixes your problem.

On second thought, that probably won't work.  I don't think this change is the
right fix, whatever the problem is.  You probably should add debug logging
locally to diagnose the issue further: sometimes page scale initialization gets
messed up due to subtle timing issues in when different sizes get initialized.

[aelias_OOO_until_Jul13, 2013-07-31 22:31:21]

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

https://codereview.chromium.org/15927026/diff/16001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:480: return
ScrollView::layoutSize(scrollbarInclusion);
I don't think we should keep using the ScrollView::layoutSize logic on
FrameViews.  Just always return m_layoutSize.  And make sure that it's always
set correctly (for example, call it on every WebViewImpl::resize()).

https://codereview.chromium.org/15927026/diff/16001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:485: void FrameView::setLayoutSize(const IntSize&
newSize)
Could you just layout() when this is called and remove all other layout calls
from FrameView?

https://codereview.chromium.org/15927026/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderView.cpp (left):

https://codereview.chromium.org/15927026/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderView.cpp:980: height = m_frameView->useFixedLayout()
? ceilf(style()->effectiveZoom() * float(height)) : height;
This is a legacy of one of the other WebKit ports, since Chrome for Android
never used effectiveZoom().  So I suggest trying to just delete these lines.

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

https://codereview.chromium.org/15927026/diff/16001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3015: enterForceCompositingMode(true);
This doesn't feel like the right place to put this.  Maybe do it when the new
FrameView is constructed?