Refactor fixed layout mode

Refactor fixed layout mode

Previously, the desktop browser would use FrameView's rect
as the layout size while the mobile browser would enable
"fixed layout mode" which then used a layoutSize variable
that it could set instead.

This CL removes "fixed layout mode" as a FrameView concept
and makes the layout size always an explicitly set property
of the main FrameView. Another way to think about it is that
we're now always in a fixed-layout, only we keep
the layout size pegged to the window size (if the
--enable-viewport flag is set, the layout size is set from
@viewport/viewport meta).

Note: WebViewImpl still has a setFixedLayoutSize method 
that's used by Android WebView.

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

[bokan, 2013-09-04 23:53:06]

I've split this into a separate CL from enabling viewport on desktop. There's
still a few changes on the Chrome side that need to be made before I can remove
some of these methods. 

This also breaks the assumption that resizing the FrameView in "fixed layout
mode" (now having layoutFallbackWidth != 0) doesn't cause a layout. This is due
to the way updateScrollbars calls visibleContentsResized. I tried moving the
layout() call from visibleContentsResized into the setLayoutSize but I think
updateScrollbars needs the relayout to calculate scrollbar visibility. Does this
even affect Clank since it uses overlay scrollbars? Those extra layout calls
shouldn't actually change anything.

[kenneth.r.christiansen, 2013-09-05 07:54:23]

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

https://codereview.chromium.org/23819019/diff/5001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1665: if (page() &&
!page()->settings().layoutFallbackWidth())
With the move to using @viewport rules internally, layoutFallbackWidth will go
away at some point

[rune, 2013-09-05 07:56:39]

Haven't looked at the details yet, but ... yay!

[kenneth.r.christiansen, 2013-09-05 08:03:07]

https://codereview.chromium.org/23819019/diff/5001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23819019/diff/5001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:2591: if (!isMainFrame())
Why don't you make this -> isMainFrame() as a separate patch. It should be quite
easy to review

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

https://codereview.chromium.org/23819019/diff/5001/Source/core/page/FrameView...
Source/core/page/FrameView.h:113: IntSize
layoutSize(VisibleContentRectIncludesScrollbars = ExcludeScrollbars) const;
Seems like the enum could be renamed now you also use it here

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

https://codereview.chromium.org/23819019/diff/5001/Source/core/page/Settings....
Source/core/page/Settings.in:24: layoutFallbackWidth type=int, initial=0
Are you making sure that Android will set this to 980?

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

https://codereview.chromium.org/23819019/diff/5001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:2886:
page()->settings().setLayoutFallbackWidth(enable ? 980 : 0);
Hmm... I hope noone are overriding it then. This is pretty much a hack and no
FIXME or explanation.

https://codereview.chromium.org/23819019/diff/5001/Source/web/tests/WebFrameT...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/23819019/diff/5001/Source/web/tests/WebFrameT...
Source/web/tests/WebFrameTest.cpp:317: // FIXME: This test currently fails
because we assumed layout() wouldn't be called
So how will that affect say chrome for android on orientation change?

https://codereview.chromium.org/23819019/diff/5001/Source/web/tests/WebFrameT...
Source/web/tests/WebFrameTest.cpp:731:
m_webView->setFixedLayoutSize(WebSize(viewportWidth, viewportHeight));
I thought you were getting rid of fixed layout

[bokan, 2013-09-05 20:03:35]

https://codereview.chromium.org/23819019/diff/5001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23819019/diff/5001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:2591: if (!isMainFrame())
On 2013/09/05 08:03:07, kenneth.r.christiansen wrote:
> Why don't you make this -> isMainFrame() as a separate patch. It should be
quite
> easy to review
Done in https://codereview.chromium.org/23654013/

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

https://codereview.chromium.org/23819019/diff/5001/Source/core/page/FrameView...
Source/core/page/FrameView.h:113: IntSize
layoutSize(VisibleContentRectIncludesScrollbars = ExcludeScrollbars) const;
On 2013/09/05 08:03:07, kenneth.r.christiansen wrote:
> Seems like the enum could be renamed now you also use it here
Done. Renamed to IncludeScrollbarsInRect

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

https://codereview.chromium.org/23819019/diff/5001/Source/core/page/Settings....
Source/core/page/Settings.in:24: layoutFallbackWidth type=int, initial=0
On 2013/09/05 08:03:07, kenneth.r.christiansen wrote:
> Are you making sure that Android will set this to 980?
So I'm actually unsure of how we typically deal with the situation where we need
to have different settings between the platforms. What's the best way to do
this?

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

https://codereview.chromium.org/23819019/diff/5001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:2886:
page()->settings().setLayoutFallbackWidth(enable ? 980 : 0);
On 2013/09/05 08:03:07, kenneth.r.christiansen wrote:
> Hmm... I hope noone are overriding it then. This is pretty much a hack and no
> FIXME or explanation.
Yes, this is a hack to keep the tests working. I'll add a note about this,
however, on the comment above about layoutFallbackWidth eventually going away,
we need some way to signal to the WebView how we want the FrameView resizing to
behave. On desktop we want to resize the layoutSize on each WebView::resize()
while on mobile we don't. aelias originally suggested tying this to
layoutFallbackWidth == 0 but it seems we need something else. Is there already
something we can use or should we add a new flag/setting (something like
'fixLayoutSizeToWindowSize')

https://codereview.chromium.org/23819019/diff/5001/Source/web/tests/WebFrameT...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/23819019/diff/5001/Source/web/tests/WebFrameT...
Source/web/tests/WebFrameTest.cpp:317: // FIXME: This test currently fails
because we assumed layout() wouldn't be called
On 2013/09/05 08:03:07, kenneth.r.christiansen wrote:
> So how will that affect say chrome for android on orientation change?
It shouldn't affect behavior since the orientation change will resize the
WebView but not the layout size. The extra layout call shouldn't actually change
anything since none of the sizes it cares about have changed. I've tested that
this works locally - as long as we're ok having a spurious layout call, but I'm
not familiar enough to know if this may have other consequences.

https://codereview.chromium.org/23819019/diff/5001/Source/web/tests/WebFrameT...
Source/web/tests/WebFrameTest.cpp:731:
m_webView->setFixedLayoutSize(WebSize(viewportWidth, viewportHeight));
On 2013/09/05 08:03:07, kenneth.r.christiansen wrote:
> I thought you were getting rid of fixed layout
Yes, sorry, this should be setLayoutSize. Fixed.

[kenneth.r.christiansen, 2013-09-05 20:35:22]

> So I'm actually unsure of how we typically deal with the situation where we
need
> to have different settings between the platforms. What's the best way to do
> this?

When the @viewport supports work correctly in the UASheets, it should be able to
fallback to what is defined there. This depends on first converting legacy meta
tags into @viewport rules, which is work in progress.

> Yes, this is a hack to keep the tests working. I'll add a note about this,
> however, on the comment above about layoutFallbackWidth eventually going away,
> we need some way to signal to the WebView how we want the FrameView resizing
to
> behave. On desktop we want to resize the layoutSize on each WebView::resize()
> while on mobile we don't. 

I am not sure that is a correct assumption, especially not when @viewport
becomes wild spread.

Think about this

<meta name='viewport' content='width=device-width, maximum-scale=1'> 

^ this actually requires resize on mobile devices. The iPhone will layout with
320 (= device-width) in portrait and 480 in landscape.

On desktop you can expect that some resizes will just result in scale change and
not actually layout size changes.

[bokan, 2013-09-05 21:09:06]

On 2013/09/05 20:35:22, kenneth.r.christiansen wrote:
> > So I'm actually unsure of how we typically deal with the situation where we
> need
> > to have different settings between the platforms. What's the best way to do
> > this?
> 
> When the @viewport supports work correctly in the UASheets, it should be able
to
> fallback to what is defined there. This depends on first converting legacy
meta
> tags into @viewport rules, which is work in progress.
> 
> > Yes, this is a hack to keep the tests working. I'll add a note about this,
> > however, on the comment above about layoutFallbackWidth eventually going
away,
> > we need some way to signal to the WebView how we want the FrameView resizing
> to
> > behave. On desktop we want to resize the layoutSize on each
WebView::resize()
> > while on mobile we don't. 
> 
> I am not sure that is a correct assumption, especially not when @viewport
> becomes wild spread.
> 
> Think about this
> 
> <meta name='viewport' content='width=device-width, maximum-scale=1'> 
> 
> ^ this actually requires resize on mobile devices. The iPhone will layout with
> 320 (= device-width) in portrait and 480 in landscape.
> 
> On desktop you can expect that some resizes will just result in scale change
and
> not actually layout size changes.

Sorry, I should have been more specific. We do still resize the layoutSize in
mobile, only not necessarily to WebView's size. The case you gave will still
work since in WebViewImpl::resize() we call updatePageDefinedScaleConstraints()
which will calculate the correct layout size from viewport arguments and set the
layout size in there. Here's the logic summarized:

WebViewImpl::resize() {
    if (isDesktop)
        mainFrame->setLayoutSize(m_size);

    updatePageDefinedScaleConstraints(); //Exits early on desktop since we don't
support viewport on desktop yet
}

We have plans to experimentally enable viewport on desktop (behind a flag) but
I'll be implementing that in a separate CL. I suppose what we'll eventually want
is both platforms to go through the same viewport logic but if there's no
viewport specified, mobile falls back to layoutFallbackWidth (or its
replacement), desktop falls back to WebViewImpl::m_size. Does this sound right?

[kenneth.r.christiansen, 2013-09-05 23:44:47]

> Sorry, I should have been more specific. We do still resize the layoutSize in
> mobile, only not necessarily to WebView's size. The case you gave will still
> work since in WebViewImpl::resize() we call
updatePageDefinedScaleConstraints()
> which will calculate the correct layout size from viewport arguments and set
the
> layout size in there. Here's the logic summarized:
> 
> WebViewImpl::resize() {
>     if (isDesktop)
>         mainFrame->setLayoutSize(m_size);

I really hope we can get rid of this isDesktop everywhere, and just have the
default viewport set as part of the default UA style sheet.

Currently it is set to the following in ./Source/core/css/html.css

1135 @viewport {
1136     min-width: 980px;
1137 }

When enabling it everywhere it should be something like

@viewport {
   width: 100%;
}

by default, and Android should use min-width: 980px;

> 
>     updatePageDefinedScaleConstraints(); //Exits early on desktop since we
don't
> support viewport on desktop yet
> }
> 
> We have plans to experimentally enable viewport on desktop (behind a flag) but
> I'll be implementing that in a separate CL. I suppose what we'll eventually
want
> is both platforms to go through the same viewport logic but if there's no
> viewport specified, mobile falls back to layoutFallbackWidth (or its
> replacement), desktop falls back to WebViewImpl::m_size. Does this sound
right?

We had that for Qt WebKit "back in the days" instead of falling back to the
actual viewport size, we did a

layoutWidth = max(fallbackWidth, viewportWidth), with fallbackWidth == 0 as
default, but set to 980 on the Nokia N9 browser.

[bokan, 2013-09-13 04:50:38]

I've cleaned up the unit and layout tests.

I've also changed the WebViewImpl::resize method to use the WebViewImpl's size
if the --enable-viewport flag is not set, otherwise it will always set size
based on the viewport arguments from the page/UA. 

The layout() call in visibleContentsResized only gets made if the
--enable-viewport flag is not set. i.e. the call gets made on desktop, not on
clank - as before. 

PTAL

[aelias_OOO_until_Jul13, 2013-09-13 05:10:47]

FYI, Android WebView turns out to still need WebView::setFixedLayoutSize(). 
When this is called from Chromium, the desired behavior from now on is to lock
the layout size to the value specified there and never change it (i.e. neither
viewport tag nor WebView::resize() will affect it).  So it will correspond more
to the literal meaning of "fixed layout".  The change is at
https://codereview.chromium.org/23441020/ and should land soon, please make this
patch compatible with it.  Maybe you could change this patch name to "Refactor
FrameView layout size."

[aelias_OOO_until_Jul13, 2013-09-13 05:29:52]

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

https://codereview.chromium.org/23819019/diff/41001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1732: layout();
The logical thing to do would be to delete this line entirely and do "if
(needsLayout()) layout();" in FrameView::setLayoutSize() instead.

[bokan, 2013-09-16 22:13:04]

I've rebased over mkosiba@'s CL and the setFixedLayoutSize method on
WebViewImpl.

Small note: I also renamed updatePageDefinedPageScaleConstraints to
updatePageDefinedViewportConstraints since it does more than just page scale.

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

https://codereview.chromium.org/23819019/diff/41001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1732: layout();
On 2013/09/13 05:29:52, aelias wrote:
> The logical thing to do would be to delete this line entirely and do "if
> (needsLayout()) layout();" in FrameView::setLayoutSize() instead.

Done. There was a unit test that I had to remove,
"FrameViewNeedsLayoutOnFixedLayoutResize" that implied there was an expectation
that setting the layout size should make needsLayout true, but now that we
layout immediately after that's no longer needed correct?

[aelias_OOO_until_Jul13, 2013-09-17 00:27:11]

https://codereview.chromium.org/23819019/diff/5002/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23819019/diff/5002/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:1738: void FrameView::visibleContentsResized()
Hmm it looks like in today's Blink, this is called exclusively from
updateScrollbars().  So I propose bringing back if (needsLayout()) layout(); in
this and rename the method to FrameView::scrollbarExistenceChanged();

https://codereview.chromium.org/23819019/diff/5002/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:3431: && doc && (doc->isHTMLDocument() ||
doc->isXHTMLDocument());
Why is it necessary to check isHTMLDocument/isXHTMLDocument?  Isn't just being
the main frame enough?

https://codereview.chromium.org/23819019/diff/5002/Source/core/platform/Scrol...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/23819019/diff/5002/Source/core/platform/Scrol...
Source/core/platform/ScrollView.cpp:340: visibleContentsResized();
This codepath relies on this to relayout.

https://codereview.chromium.org/23819019/diff/5002/Source/core/platform/Scrol...
Source/core/platform/ScrollView.cpp:415: visibleContentsResized();
This codepath relies on this to relayout.

https://codereview.chromium.org/23819019/diff/5002/Source/core/platform/Scrol...
Source/core/platform/ScrollView.cpp:420: visibleContentsResized();
This codepath relies on this to relayout.

https://codereview.chromium.org/23819019/diff/5002/Source/core/platform/Scrol...
Source/core/platform/ScrollView.cpp:716: contentsResized();
Can you remove this line?  It seems redundant with the contentsResized call in
setLayoutSize().

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

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1663: // Update mainframe size before layout size.
This prevents the case where the layout size shrinks causing the
Looks like you need to move this comment?

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1680: // Relayout immediately to recalculate the
minimum scale limit.
I think this layout() call can be deleted now.  Please do so and merge the two
nested if() statements.

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:2875: // TODO: Remove once chromium-side changes land
Just put {} in WebView.h and you can delete the WebViewImpl method now.

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

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.h#n...
Source/web/WebViewImpl.h:247: // Locks the main frame's layout size to a fixed
value and causes
Please move this comment to WebView.h

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.h#n...
Source/web/WebViewImpl.h:249: // or height to 0 will remove the lock and renable
viewport/resize.
width *and* height to 0.  0-height layout is actually used in WebView.

[kenneth.r.christiansen, 2013-09-18 21:09:48]

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

https://codereview.chromium.org/23819019/diff/53001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:3433: && doc && (doc->isHTMLDocument() ||
doc->isXHTMLDocument());
Why is this? because it cannot affect SVG documents?

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

https://codereview.chromium.org/23819019/diff/53001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3099: void WebViewImpl::setFixedLayoutSize(const
WebSize& layoutSize)
I think it would be more clear if it was called

setLayoutSizeOverride()

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

https://codereview.chromium.org/23819019/diff/53001/public/web/WebView.h#newc...
public/web/WebView.h:311: virtual void setFixedLayoutSize(const WebSize&) = 0;
Are these public methods? or could we call it setLayoutSizeOverride(..) ?

[bokan, 2013-09-18 21:17:42]

I noticed that some layout tests were broken in the last patch so I've found
some of the layout/updateScrollbars calls needed to be moved around, see inline
comments. Everything should be passing now.

A few tests that use viewport/fixed-layout-size were causing problems with
scrollbars popping up unexpectedly and sizes being off by 15 since viewport
doesn't ineract well with scrollbars on viewport. I've changed those tests to
always hide scrollbars for now. I'll come back to these when enabling viewport
on desktop.

Another issue with tests now is in WebFrameTest there's tests with FixedLayout
in the name that are actually testing  viewport and others that are actually
testing FixedLayout for WebView. I'll have to clean these up but it'll require
going through many tests one by one and this patch is already big enough.

Finally, I've been rebaselining as I go and I just noticed this breaks the
patch-to-patch comparison in the review tool. If it makes life easier, I can
send a diff patch with changes from the last one. 

Thanks!

https://codereview.chromium.org/23819019/diff/5002/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23819019/diff/5002/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:1738: void FrameView::visibleContentsResized()
On 2013/09/17 00:27:12, aelias wrote:
> Hmm it looks like in today's Blink, this is called exclusively from
> updateScrollbars().  So I propose bringing back if (needsLayout()) layout();
in
> this and rename the method to FrameView::scrollbarExistenceChanged();
Renamed to scrollbarExistenceDidChange() due to name conflict in
updateScrollbars()

This call would only get made in non-fixed-layout. I changed it to
if(!viewportEnabled() && needsLayout()) layout(); so it should effectively work
as before. The reason being some of the tests get broken by scrollbars changing
layout size (not a problem in Clank and viewport isn't supported on desktop
yet). Removing the !viewportEnabled check will be part of the work to make
viewport work on desktop. Am I correct in assuming for now that viewportEnabled
roughly means useFixedLayout?

https://codereview.chromium.org/23819019/diff/5002/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:3431: && doc && (doc->isHTMLDocument() ||
doc->isXHTMLDocument());
On 2013/09/17 00:27:12, aelias wrote:
> Why is it necessary to check isHTMLDocument/isXHTMLDocument?  Isn't just being
> the main frame enough?
This was causing breaks related to plugins and SVG - I assume they have cases
where their main frame doesn't live in a WebViewImpl. Originally I added the
appropriate setLayoutSize calls, but there were numerous places this had to be
done and it seemed brittle and prone to error. Doing this makes it so that all
non-HTML documents layout in the frame rect size, seems appropriate - is it ok?

https://codereview.chromium.org/23819019/diff/5002/Source/core/platform/Scrol...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/23819019/diff/5002/Source/core/platform/Scrol...
Source/core/platform/ScrollView.cpp:415: visibleContentsResized();
On 2013/09/17 00:27:12, aelias wrote:
> This codepath relies on this to relayout.
Does it? If scrollbars are overlay they shouldn't need a relayout, no?

https://codereview.chromium.org/23819019/diff/5002/Source/core/platform/Scrol...
Source/core/platform/ScrollView.cpp:716: contentsResized();
On 2013/09/17 00:27:12, aelias wrote:
> Can you remove this line?  It seems redundant with the contentsResized call in
> setLayoutSize().

Done.

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

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1663: // Update mainframe size before layout size.
This prevents the case where the layout size shrinks causing the
On 2013/09/17 00:27:12, aelias wrote:
> Looks like you need to move this comment?
I moved the updateLayoutSize call back up here, having it after setting the
frameView size cause some other problems I found. The case the comment was
describing went away as well, I think I've fixed up the ordering of
layout/needsLayout calls between setLayoutSize and ScrollView::setFrameRect.

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:1680: // Relayout immediately to recalculate the
minimum scale limit.
On 2013/09/17 00:27:12, aelias wrote:
> I think this layout() call can be deleted now.  Please do so and merge the two
> nested if() statements.
Related to the above comment - I removed the layout call in setLayoutSize since
it would cause a layout before the frameRect had a chance to resize. Calling
setLayoutSize now calls setNeedsLayout (via contentsResized). I think this
layout call needs to stay now.

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:2875: // TODO: Remove once chromium-side changes land
On 2013/09/17 00:27:12, aelias wrote:
> Just put {} in WebView.h and you can delete the WebViewImpl method now.

Done.

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

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.h#n...
Source/web/WebViewImpl.h:247: // Locks the main frame's layout size to a fixed
value and causes
On 2013/09/17 00:27:12, aelias wrote:
> Please move this comment to WebView.h

Done.

https://codereview.chromium.org/23819019/diff/5002/Source/web/WebViewImpl.h#n...
Source/web/WebViewImpl.h:249: // or height to 0 will remove the lock and renable
viewport/resize.
On 2013/09/17 00:27:12, aelias wrote:
> width *and* height to 0.  0-height layout is actually used in WebView.
I talked with mkosiba after sumitting this and realized I misread the patch.
Fixed.

[bokan, 2013-09-18 21:23:21]

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

https://codereview.chromium.org/23819019/diff/53001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:3433: && doc && (doc->isHTMLDocument() ||
doc->isXHTMLDocument());
On 2013/09/18 21:09:49, kenneth.r.christiansen wrote:
> Why is this? because it cannot affect SVG documents?

Yes, I had problems with SVG documents and plugins. As far as I know, these
don't need any kind of layout size manipulation so I figured leaving non HTML
documents as layoutSize == frameRect was the simplest solution.

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

https://codereview.chromium.org/23819019/diff/53001/public/web/WebView.h#newc...
public/web/WebView.h:311: virtual void setFixedLayoutSize(const WebSize&) = 0;
On 2013/09/18 21:09:49, kenneth.r.christiansen wrote:
> Are these public methods? or could we call it setLayoutSizeOverride(..) ? 
These are public, the original intent was to remove them but WebView needs to be
able to fix layout size. I agree LayoutSizeOverride is a better name but it's a
non-trivial change. I can do that in a separate CL.

[bokan, 2013-09-30 19:05:30]

aelias@, I've rebased and fixed to keep everything building and passing, PTAL
when you have a chance.

[aelias_OOO_until_Jul13, 2013-10-01 05:09:30]

https://chromiumcodereview.appspot.com/23819019/diff/65001/LayoutTests/TestEx...
File LayoutTests/TestExpectations (right):

https://chromiumcodereview.appspot.com/23819019/diff/65001/LayoutTests/TestEx...
LayoutTests/TestExpectations:1298: #Below test was broken but fixed with patch
for crbug.com/285397 so now it needs a new baseline
Please remove these comments, I'm not sure if the auto-rebaseliner will clean
them up for you.

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/core/page/F...
File Source/core/page/FrameView.cpp (right):

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/core/page/F...
Source/core/page/FrameView.cpp:1749: if
(!m_frame->page()->settings().viewportEnabled() && needsLayout())
Checking viewportEnabled here doesn't seem very principled.  How about checking
if they're overlay scrollbars instead?

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/core/page/F...
Source/core/page/FrameView.cpp:3437: bool layoutSizeIsExplicitlySet =
m_frame->page() && m_frame->page()->mainFrame() == m_frame
Instead of this, how about a public method
FrameView::setLayoutSizeMatchesFrameSize(bool), make the boolean default to true
and only set it false from WebViewImpl?

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/web/WebView...
File Source/web/WebViewImpl.cpp (right):

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/web/WebView...
Source/web/WebViewImpl.cpp:1721: updateMainFrameLayoutSize();
Please call:

updatePageDefinedViewportConstraints();
updateMainFrameLayoutSize();

They are logically distinct in my mind (viewport size is an input to viewport
tag calculation, then layout can happen after that).

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/web/WebView...
Source/web/WebViewImpl.cpp:1738: view->layout();
Could you delete this layout call?  It should no longer be necessary.

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/web/WebView...
Source/web/WebViewImpl.cpp:3038:
page()->mainFrame()->document()->textAutosizer()->recalculateMultipliers();
Please move this line into updateMainFrameLayoutSize.

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/web/WebView...
Source/web/WebViewImpl.cpp:3041:
page()->mainFrame()->view()->setLayoutSize(layoutSize);
Just call updateMainFrameLayoutSize from here instead.

https://chromiumcodereview.appspot.com/23819019/diff/65001/Source/web/WebView...
Source/web/WebViewImpl.cpp:3053:
updatePageDefinedViewportConstraints(mainFrameImpl()->frame()->document()->viewportArguments());
I don't like going into complex viewport tag processing code from this method. 
It should just apply the value that the viewport code calculated previously
instead.  Please restructure like:

if (m_fixedLayoutSizeLock)
  return;

if (settings()->viewportEnabled())
 
view->setLayoutSize(flooredIntSize(m_pageScaleConstraintsSet.pageDefinedConstraints().layoutSize));
else
  view->setLayoutSize(m_size);

[bokan, 2013-10-02 17:44:00]

https://codereview.chromium.org/23819019/diff/65001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/23819019/diff/65001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1298: #Below test was broken but fixed with patch
for crbug.com/285397 so now it needs a new baseline
On 2013/10/01 05:09:30, aelias wrote:
> Please remove these comments, I'm not sure if the auto-rebaseliner will clean
> them up for you.
Done. The bottom test is currently expected to Crash, but it's text output also
needs to be rebaselined, should I just leave it commented until it's fixed? It
doesn't crash locally for me, should I remove the crash expectation try
rebaselineing it?

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

https://codereview.chromium.org/23819019/diff/65001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1749: if
(!m_frame->page()->settings().viewportEnabled() && needsLayout())
On 2013/10/01 05:09:30, aelias wrote:
> Checking viewportEnabled here doesn't seem very principled.  How about
checking
> if they're overlay scrollbars instead?
A few unit tests in WebFrameTest fail when I check for that instead. I'm
assuming they're checking Clank/WebView specific behaviour but the tests don't
have the lverlay scrollbars theme enabled. Does WebView use overlay scrollbars
as well? If it does I can fix the tests and change this.

https://codereview.chromium.org/23819019/diff/65001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:3437: bool layoutSizeIsExplicitlySet =
m_frame->page() && m_frame->page()->mainFrame() == m_frame
On 2013/10/01 05:09:30, aelias wrote:
> Instead of this, how about a public method
> FrameView::setLayoutSizeMatchesFrameSize(bool), make the boolean default to
true
> and only set it false from WebViewImpl?
Done, only I set it false from Frame::createView

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

https://codereview.chromium.org/23819019/diff/65001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1721: updateMainFrameLayoutSize();
On 2013/10/01 05:09:30, aelias wrote:
> Please call:
> 
> updatePageDefinedViewportConstraints();
> updateMainFrameLayoutSize();
> 
> They are logically distinct in my mind (viewport size is an input to viewport
> tag calculation, then layout can happen after that).

Done.

https://codereview.chromium.org/23819019/diff/65001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1738: view->layout();
On 2013/10/01 05:09:30, aelias wrote:
> Could you delete this layout call?  It should no longer be necessary.
Why is that so? None of the calls I added above will cause a layout. Removing
this here breaks page scale related unit tests.

https://codereview.chromium.org/23819019/diff/65001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3038:
page()->mainFrame()->document()->textAutosizer()->recalculateMultipliers();
On 2013/10/01 05:09:30, aelias wrote:
> Please move this line into updateMainFrameLayoutSize.

Done.

https://codereview.chromium.org/23819019/diff/65001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3041:
page()->mainFrame()->view()->setLayoutSize(layoutSize);
On 2013/10/01 05:09:30, aelias wrote:
> Just call updateMainFrameLayoutSize from here instead.

Done.

https://codereview.chromium.org/23819019/diff/65001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3053:
updatePageDefinedViewportConstraints(mainFrameImpl()->frame()->document()->viewportArguments());
On 2013/10/01 05:09:30, aelias wrote:
> I don't like going into complex viewport tag processing code from this method.

> It should just apply the value that the viewport code calculated previously
> instead.  Please restructure like:
> 
> if (m_fixedLayoutSizeLock)
>   return;
> 
> if (settings()->viewportEnabled())
>  
>
view->setLayoutSize(flooredIntSize(m_pageScaleConstraintsSet.pageDefinedConstraints().layoutSize));
> else
>   view->setLayoutSize(m_size);

Done.

[aelias_OOO_until_Jul13, 2013-10-02 18:59:58]

https://codereview.chromium.org/23819019/diff/65001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/23819019/diff/65001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1298: #Below test was broken but fixed with patch
for crbug.com/285397 so now it needs a new baseline
If it doesn't crash anymore, feel free to try to reactivate it and rebaseline
it.  If it still crashes on one of the trybots then forget about it.

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

https://codereview.chromium.org/23819019/diff/65001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1749: if
(!m_frame->page()->settings().viewportEnabled() && needsLayout())
On 2013/10/02 17:44:01, bokan wrote:
> On 2013/10/01 05:09:30, aelias wrote:
> > Checking viewportEnabled here doesn't seem very principled.  How about
> checking
> > if they're overlay scrollbars instead?
> A few unit tests in WebFrameTest fail when I check for that instead. I'm
> assuming they're checking Clank/WebView specific behaviour but the tests don't
> have the lverlay scrollbars theme enabled. Does WebView use overlay scrollbars
> as well? If it does I can fix the tests and change this. 

Yes, both Clank and WebView always use overlay scrollbars.  You can go ahead and
update all the WebFrameTests that enable the viewport tag to also activate the
overlay scrollbar theme.

[bokan, 2013-10-02 21:27:26]

Done. If you're ok with this, please loop in an OWNER.

https://codereview.chromium.org/23819019/diff/65001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/23819019/diff/65001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1298: #Below test was broken but fixed with patch
for crbug.com/285397 so now it needs a new baseline
On 2013/10/02 18:59:59, aelias wrote:
> If it doesn't crash anymore, feel free to try to reactivate it and rebaseline
> it.  If it still crashes on one of the trybots then forget about it.

Done.

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

https://codereview.chromium.org/23819019/diff/65001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1749: if
(!m_frame->page()->settings().viewportEnabled() && needsLayout())
On 2013/10/02 18:59:59, aelias wrote:
> On 2013/10/02 17:44:01, bokan wrote:
> > On 2013/10/01 05:09:30, aelias wrote:
> > > Checking viewportEnabled here doesn't seem very principled.  How about
> > checking
> > > if they're overlay scrollbars instead?
> > A few unit tests in WebFrameTest fail when I check for that instead. I'm
> > assuming they're checking Clank/WebView specific behaviour but the tests
don't
> > have the lverlay scrollbars theme enabled. Does WebView use overlay
scrollbars
> > as well? If it does I can fix the tests and change this. 
> 
> Yes, both Clank and WebView always use overlay scrollbars.  You can go ahead
and
> update all the WebFrameTests that enable the viewport tag to also activate the
> overlay scrollbar theme.

Done.

[aelias_OOO_until_Jul13, 2013-10-03 00:35:16]

lgtm modulo comments below.  Adding leviw@ for OWNERS.  Please also edit your
patch description not to go over 80 columns.

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

https://codereview.chromium.org/23819019/diff/86001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1616: void FrameView::setLayoutSize(const
IntSize& size)
Could you ASSERT that no one calls the public setLayoutSize() when
m_layoutSizeFixedToFrameSize is true?

https://codereview.chromium.org/23819019/diff/86001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1749: if
(!ScrollbarTheme::theme()->usesOverlayScrollbars() && needsLayout())
Please do:

if (ScrollbarTheme::theme()->usesOverlayScrollbars())
    return;

since we don't need the frameViewDidChangeSize call below either.

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

https://codereview.chromium.org/23819019/diff/86001/Source/core/page/FrameVie...
Source/core/page/FrameView.h:345: void setLayoutSizeFixedToFrameSize(bool
isFixed) { m_layoutSizeFixedToFrameSize = isFixed; }
Please move this up next to layoutSize()/setLayoutSisze().

[leviw_travelin_and_unemployed, 2013-10-03 00:46:09]

LGTM. Great work!

[bokan, 2013-10-03 19:52:39]

I added a frameViewScrollbarsExistenceDidChange method in RenderLayerCompositor
that now gets called in the FrameView method to ensure a scrollbar layer gets
created.

Could you (or someone familiar with the compositor) have a quick look? I imagine
the addition is fairly trivial but I have no experience inside the compositor.
The changes are in RenderLayerCompositor and the scrollbarExistenceDidChange
method in FrameView.cpp

Other than that, once I get the patch to apply and run on the try bots this is
ready to land.

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

https://codereview.chromium.org/23819019/diff/86001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1616: void FrameView::setLayoutSize(const
IntSize& size)
On 2013/10/03 00:35:16, aelias wrote:
> Could you ASSERT that no one calls the public setLayoutSize() when
> m_layoutSizeFixedToFrameSize is true?

Done.

https://codereview.chromium.org/23819019/diff/86001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:1749: if
(!ScrollbarTheme::theme()->usesOverlayScrollbars() && needsLayout())
On 2013/10/03 00:35:16, aelias wrote:
> Please do:
> 
> if (ScrollbarTheme::theme()->usesOverlayScrollbars())
>     return;
> 
> since we don't need the frameViewDidChangeSize call below either.

We needed the frameViewDidChangeSize below so that a scrollbar layer would be
created. I've added a frameViewScrollbarsExistenceDidChange to
RenderLayerCompositor and called it from this method.

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

https://codereview.chromium.org/23819019/diff/86001/Source/core/page/FrameVie...
Source/core/page/FrameView.h:345: void setLayoutSizeFixedToFrameSize(bool
isFixed) { m_layoutSizeFixedToFrameSize = isFixed; }
On 2013/10/03 00:35:16, aelias wrote:
> Please move this up next to layoutSize()/setLayoutSisze().

Done.

[aelias_OOO_until_Jul13, 2013-10-03 20:23:46]

RenderLayerCompositor change looks good modulo one comment.

https://codereview.chromium.org/23819019/diff/100001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23819019/diff/100001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1297:
updateOverflowControlsLayers();
Please bracket with "if (m_containerLayer)", since updateOverflowControlsLayers
does use it.

[commit-bot: I haz the power, 2013-10-07 15:52:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/23819019/116001

[commit-bot: I haz the power, 2013-10-07 19:26:03]

Change committed as 159039