Make it possibe to lock the fixedLayoutSize

This makes it possible to specify and lock the fixedLayoutSize.

The reason for this change is android_webview/ wrap_content mode where the size of
the WebView depends on the size of its contents. Without forcing the layout size
to a fixed value, such a configuration becomes unstable when elements whose size is
relative to the viewport size are present in the contents.

This also adds support for zero-height, non-zero width layout sizes.

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

[mkosiba (inactive), 2013-08-30 18:25:08]

These are pretty raw, but I'd appreciate a first pass of feedback.

The patch that flips this on in the webview is
https://codereview.chromium.org/23583025/

The one problem I'm hitting is that fixed position isn't working too well.
If I use 2 iterations of the layout algorithm then the fixed-position elements
appear as if they were 'stuck' to the logical viewport, not the
physical_backing_size. If I only use 1 iteration then fixed position elements
don't appear at all!
Fixed position elements also flicker on pinch-zooming (could this be because
we're resizing to 0?)

Sami said that there used to be an option to convert position:fixed to
position:absolute - do you know if this is still around? It seems like that
would make more sense for wrap-content mode than real fixed position.

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

https://codereview.chromium.org/23441020/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:2306: int height = fixedWidthMode ? 0 :
documentRenderBox->scrollHeight();
ah, actually this should be
  fixedWidthMode ? m_minAutoSize.height() : ...
to support the case where we're in wrap_content mode but are sized to be bigger
than our content.

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

https://codereview.chromium.org/23441020/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:3665: if (!m_forceFixedLayoutSizeToEqualSize)
the reason for this is that it causes RenderWidget to resize the compositor to
m_size * dipScale.

[mkosiba (inactive), 2013-08-30 20:47:47]

I was also playing around with making the autosizing work in a slightly
different way:
1) resize to (width, 0)
2) content_height = content height at this point
3) resize to (width, content_height)
4) content_height2 = content height at this point
5) resize to (width, content_height2)
6) set fixedLayoutSize to (width, content_height)
[or maybe get rid of steps 5,6 and set fixedLayoutSize to (width, 0) if that
works]

That would have the property that the WebKit::WebView's size would better match
the physical_backing_size. I'm not sure if that would help the fixed position
stuff in any way though.
The problem I've had with the above approach is that setting the fixedLayoutSize
from the autosizing code resulted in an infinite layout loop and I didn't have
the time to debug that any further.

[aelias_OOO_until_Jul13, 2013-08-31 03:45:04]

Hmm.  I'm having trouble understanding what are the relevant differences between
the WebView case and the extension popup cases, that result in forcing you to
make these changes.  I can't see the principles behind the algorithm changes. 
Could you explain further (or, better, change to self-explanatory code)?

I think if we can avoid it, we should also not make any changes to Blink's
treatment of fixed-pos elements, as that seems like a workaround for bugs
elsewhere.

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

https://codereview.chromium.org/23441020/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:3665: if (!m_forceFixedLayoutSizeToEqualSize)
On 2013/08/30 18:25:09, mkosiba wrote:
> the reason for this is that it causes RenderWidget to resize the compositor to
> m_size * dipScale.

Could you explain why that's wrong?  Is it because of page scale or something
else?

[aelias_OOO_until_Jul13, 2013-08-31 03:50:10]

I may really need a summary of the principles behind WRAP_CONTENT behavior as I
don't know the details beyond the fact that it somehow sizes the view to the
content.

[mkosiba (inactive), 2013-09-02 11:28:58]

It may help to read up on Android layout a bit
(http://developer.android.com/reference/android/view/View.html, scroll down to
the Layout section) but in our case this boils down to implementing two
callbacks:
- onMeasure(widthSpec, heightSpec)
  this is what the parent tells the WebView what are the constraints and the
WebView tells the parent back on how big it _wants_ to be
- onSizeChanged(w, h, ow, oh)
   this is where the parent tells the WebView how big it must be. These values
might be different to what we asked for in onMeasure.

The above means that AutoResize code can't change the physical_backing_size
directly as a result of a Blink layout. We need to trigger an Android View
system layout pass first before the physical_backing_size can change, otherwise
hardware mode draws could have a clip rect that's bigger than
physical_backing_size.

Differences between the wrap_content case and extension popups:
1) in wrap_content mode the physical size of the webview is unfortunately
content_size * dipScale * pageScaleFactor. Extension popups don't need to handle
pinch-zooming.
2) in wrap_content mode the webview _tries_ to size itself to be as big as the
content, but the parent container may force the webview to be bigger or smaller
in any dimension.
2a) the most common case is where the parent container sets the webview's width
to be equal to its own but will allow the webview to grow/shrink as needed, the
AutoResize code tries to modify both dimensions at the same time,
3) WebViewClassic would do the initial wrap_content layout with a viewport
height of 0 (which is why I changed the number of iterations)

It'll probably help to go through a couple of cases.

The WebView is in a LinearLayout parent container:
 - the webview's physical width is fixed, but the height can change (this is why
AutoResize needs to support using a particular width instead of always using the
minimal width)
 - if the content is wider that the viewport, the webview will handle horizontal
scrolling but there should be no vertical scrolling,
 - assuming a pageScale of 1.0, this means that the CSS viewport is equal to
(physical_width/mDIPScale, content_height)

If pinch-zoom is enabled this gets a bit more complicated (also, Classic would
not support this well):
 - the physical width stays the same, but each time after a pinch-zoom gesture
we need to do a layout pass. If we assume the content's height is unconstrained
then the CSS viewport becomes (physical_width/(mDIPScale*pageScale),
content_height).

The parent container enforces a minimum height:
 - this means that if the content height is less than a particular value the
parent container will ask the WebView to size itself at (physical_width,
physical_minimum_height), that will result in a CSS viewport of
(physical_width/(mDIPScale*pageScale),
physical_minimum_height/(mDIPScale*pageScale)), but if the content height goes
over a threshold the behavior is the same as in the previous case.
 - this is why we need to start the autosize pass with a height=minimumHeight

The parent container enforces a maximum height:
  - similar to above. The WebView's physical height will grow up to a certain
upper bound. After that bound is exceeded the WebView will handle all scrolling
internally. This one is especially weird though, since even though the physical
size ends up being fixed, we still need to factor in the pageScale.

The webview can 'wrap content' in both dimensions:
 - this is the case that was least supported by the Classic WebView. In this
mode the AutoResize code would run 'as normal'.

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

https://codereview.chromium.org/23441020/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:3665: if (!m_forceFixedLayoutSizeToEqualSize)
On 2013/08/31 03:45:04, aelias wrote:
> On 2013/08/30 18:25:09, mkosiba wrote:
> > the reason for this is that it causes RenderWidget to resize the compositor
to
> > m_size * dipScale.
> 
> Could you explain why that's wrong?  Is it because of page scale or something
> else?

Page scale mostly. My concern is that this ends up overriding the
physical_backing_size
AwContents has previously set. Even if the RenderWidget factored in the
pageScale correctly
there could still rounding errors.
The AutoResize code was written with the assumption that the size of the popup
window can be
freely altered. This is not the case for webview.

[mkosiba (inactive), 2013-09-02 16:17:14]

+ Shawn for fixed position know-how

Hmm... after writing the above and a bit more thought I'm wondering whether the
AutoResize is really the way to go. It seems to me that the following might be a
better approach for wrap_content mode:
1) separate RenderWidget and physical_backing_size
  - the RenderWidget size will be webview_physical_size / (mDIPScale *
pageScale)
  - the physical_backing_size will be just webview_physical_size
2) calculate the contents height assuming viewport height of 0, use this height
in the onMeasure callback to tell the Android view system how much space the
WebView wants,
3) perform layout assuming viewport height of 0.

This matches what we want to do a bit better I think since the Android view
system gets to 'drive' the RenderWidget size. The extension popup AutoSizing
code assumed that the renderer is fully in control of the size, which 

The downside is that I can't quite get 3) to work. I can calulate the size
assuming a viewport height of 0 fine, but the way I tried to achieve 3 is to set
the FrameView height to 0 which also immediately breaks fixed position (Shawn -
could this just be an issue of having to use the RenderWidget size, instead of
the FrameView size somewhere?)

I guess the requirements could be distilled to the following:

We need a way to lay out the web contents in the WebKit::WebView in such a way
that altering the viewport doesn't cause the content size to grow (unless it's
to 'stretch' to match the viewport) and for fixed position to still work.
This is pretty much to work around the <div sytle="height:120%"> case, but also
to maintain the contract where, if the Android view system ask the WebView "how
much height do you need to not scroll" and then sizes the WebView to that
height, then the WebView doesn't scroll.

[aelias_OOO_until_Jul13, 2013-09-03 01:59:04]

Post-https://codereview.chromium.org/23171014/ (which is already cherry-picked
into klp-dev), physical_backing_size is *only* used for fixed-pos and for
root-layer scrollability, so you don't need to worry about how it relates to
drawing viewport/clip.  You just need to make sure that A) from Blink's POV, the
viewport height (used to position fixed-pos elements) is equal to the contents
height, and B) LayerTreeImpl::UpdateRootScrollLayerSizeDelta() ends up setting a
value of zero in SetFixedContainerSizeDelta.  To ensure B), I think that your
proposal in 1) is not quite right and it should be:
- the RenderWidget size will be webview_physical_size / (mDIPScale * pageScale)
- the physical_backing_size will be webview_physical_size / pageScale

Given this, the AutoResize physical_backing_size assignment seems to do the
about the right thing (except for rounding errors due to the ceil -- which may
be difficult to avoid no matter what you do because the root cause of that is
the page scale division you're doing.)  But I'm not dogmatic about using
AutoResize if it doesn't meet the other requirements.

I'm not sure exactly why a viewport height of 0 would cause a problem with
fixed-pos, maybe you're hitting some kind of early-out in Blink.  What kind of
"break" are you seeing?  What happens if you set the height to 1 instead?

[mkosiba (inactive), 2013-09-03 19:01:24]

On 2013/09/03 01:59:04, aelias wrote:
> Post-https://codereview.chromium.org/23171014/ (which is already cherry-picked
> into klp-dev), physical_backing_size is *only* used for fixed-pos and for
> root-layer scrollability, so you don't need to worry about how it relates to
> drawing viewport/clip.  You just need to make sure that A) from Blink's POV,
the
> viewport height (used to position fixed-pos elements) is equal to the contents
> height, and B) LayerTreeImpl::UpdateRootScrollLayerSizeDelta() ends up setting
a
> value of zero in SetFixedContainerSizeDelta.  To ensure B), I think that your

I got it to be less than Vetor2dF(1, 1), but not zero.
What impact would a non-zero FixedContainerSizeDelta have?

> proposal in 1) is not quite right and it should be:
> - the RenderWidget size will be webview_physical_size / (mDIPScale *
pageScale)
> - the physical_backing_size will be webview_physical_size / pageScale

Setting the physical_backing_size to webview_physical_size or
webview_physical_size / pageScale
doesn't seem to be sufficient to make fixed position work. The video below is
using
physical_backing_size == webview_physical_size
https://docs.google.com/a/chromium.org/file/d/0B_-sOZQZrxnGSndOcDdRam9Cdnc/ed...

FWIW the HTML of the email shown is
  <body>
    <div style="height:20px; position:fixed; background-color: black; top: 0px;
right:10px;">aaa</div>
    <div style="height:20px; position:fixed; background-color: black; bottom:
0px; left:10px;">bbb</div>
    <div style="height:20px; background-color:blue"> height 20px </div>
    <div style="height:120%; background-color:red"> height 120 </div>
    <button onclick="addDiv()">button</button>
  </body>

Any idea on what might be happening with the fixed pos elements (those are the
black
rectangles in the top and bottom corners)? It looks like a pageScale of < 1 is
causing them to somehow be positioned outside of the viewport, but at pageScale
>= 1.0
the fixed pos elements seem to behave correctly.
It is expected that during pinch-zoom the whole thing falls apart, what I want
is
for the fixed pos elements to be positioned correctly after the pinch gestrue
ends.

> Given this, the AutoResize physical_backing_size assignment seems to do the
> about the right thing (except for rounding errors due to the ceil -- which may
> be difficult to avoid no matter what you do because the root cause of that is
> the page scale division you're doing.)  But I'm not dogmatic about using
> AutoResize if it doesn't meet the other requirements.

I think the only reason AutoResize worked was that I made it do the following:
1) resize(width, 0)
2) layout
3) resize(width, contentHeight)
Since there isn't a 4th re-layout step, it means that what this is
effectively doing is setting a fixedLayoutSize of (with, 0). I think
that what I describe below is a more straightforward way of achieving this.

> I'm not sure exactly why a viewport height of 0 would cause a problem with
> fixed-pos, maybe you're hitting some kind of early-out in Blink.  What kind of
> "break" are you seeing?  What happens if you set the height to 1 instead?

You're probably right. The break was fixed position elements not showing up
at all, which suggests that an early out could have been the cause. 
Setting the height to > 0 makes the fix position elements show, but only if
the pageScale is > 1 (see attached video).

I got wrap_content to mostly work after realizing that ScrollView will ignore
the fixed layout size if m_fixedLayoutSize.IsEmpty is true. Your hint about
using a height of 1 was very helpful :)
The patch that I'm working on now works like this:

 - the RenderWidget size is webview_physical_size / (mDIPScale * pageScale)
 - the physical_backing_size is webview_physical_size / pageScale
 - the fixedLayoutSize will be (width / (mDIPScale * pageScale), 0*)

* 0 seems to trigger quite a few early-out paths. I'll see how many changes
  I'd need to make to remove the early-outs, but if I can't get that to work
  I'll probably do something like:

  fixedLayoutHeight = 8;
  if (contentsHeight <= fixedLayoutHeight)
    fixedLayoutHeight = 0;
  to make it possible for the WebView to shrink back to a zero height.

The trick is to use a fixedLayoutSize that is independent of the viewport
height.
I'm planning to add a WebView::setLayoutSizeOverride API that would allow
the webview to set a layout size that is fixed and can't be influenced
by the RenderWidget size or viewport meta-tag (which is what the classic webview
did btw.)
This mode would also disable a couple of pageScale-related features, namely:
 - adjusting pageScale after resize
 - m_pageScaleConstraintsSet.adjustFinalConstraintsToContentsSize


Other than the fixed position issue I described above the only problem 
I'm seeing is that sometimes I get a 1 pixel wide line of garbage on the
right and bottom edges of the view (I'm testing on an N7 which has the
1.33 dpi scale).
I'm assuming this is because I'm calculating the RenderWidget size as:
(int) (physical_size / (pageScale * dip scale))
instead of
(int) Math.ceil(physical_size / (pageScale * dip scale))
right?
I initially assumed that setting physical_backing_size to be exactly equal
to the physical viewport would ensure that issues like this do not occur,
but your previous comment suggests that would not be the case.

[aelias_OOO_until_Jul13, 2013-09-04 03:25:51]

> What impact would a non-zero FixedContainerSizeDelta have?

It applies a translation on the fixed-pos elements to try to correct for
viewport size and page_scale_delta differences.  If you got it to subpixel it
might be correcting for a rounding error.

For your video, I suspect what's happening is that Blink is positioning the
fixed-pos using visibleContentRect which is adjusted by page scale (called
visibleContentScaleFactor in this context).  If you hack FrameView to have
visibleContentScaleFactor = 1 does it fix your problem?

[mkosiba (inactive), 2013-09-05 18:16:22]

On 2013/09/04 03:25:51, aelias wrote:
> > What impact would a non-zero FixedContainerSizeDelta have?
> 
> It applies a translation on the fixed-pos elements to try to correct for
> viewport size and page_scale_delta differences.  If you got it to subpixel it
> might be correcting for a rounding error.
> 
> For your video, I suspect what's happening is that Blink is positioning the
> fixed-pos using visibleContentRect which is adjusted by page scale (called
> visibleContentScaleFactor in this context).  If you hack FrameView to have
> visibleContentScaleFactor = 1 does it fix your problem?

Yes, that seems to fix it! Thanks! It seems like there is some small
race somewhere though. If you pinch-zoom a couple of times about 1/10 times
the fixed pos elements don't get re-positioned. Pinch-zooming ever so slightly
will then cause them to snap into place. I think that's a very minor issue
though
and would prefer to fix it separately.

I've updated the CL to include all of the hacks required for this to work.
Do you think any of these are too gross to check in (the
setVisibleContentScaleFactor
hack looks kinda hairy)?

[mkosiba (inactive), 2013-09-05 18:17:38]

On 2013/09/05 18:16:22, mkosiba wrote:
> On 2013/09/04 03:25:51, aelias wrote:
> > > What impact would a non-zero FixedContainerSizeDelta have?
> > 
> > It applies a translation on the fixed-pos elements to try to correct for
> > viewport size and page_scale_delta differences.  If you got it to subpixel
it
> > might be correcting for a rounding error.
> > 
> > For your video, I suspect what's happening is that Blink is positioning the
> > fixed-pos using visibleContentRect which is adjusted by page scale (called
> > visibleContentScaleFactor in this context).  If you hack FrameView to have
> > visibleContentScaleFactor = 1 does it fix your problem?
> 
> Yes, that seems to fix it! Thanks! It seems like there is some small
> race somewhere though. If you pinch-zoom a couple of times about 1/10 times
> the fixed pos elements don't get re-positioned. Pinch-zooming ever so slightly
> will then cause them to snap into place. I think that's a very minor issue
> though
> and would prefer to fix it separately.
> 
> I've updated the CL to include all of the hacks required for this to work.
> Do you think any of these are too gross to check in (the
> setVisibleContentScaleFactor
> hack looks kinda hairy)?

Ah, forgot to mention - Chromium-side CL for this is
https://codereview.chromium.org/23478022/
you might want to look at the AwLayoutSizer changes and maybe ContentViewCore

[aelias_OOO_until_Jul13, 2013-09-06 01:50:06]

This doesn't look bad now!  I wouldn't mind something like this landing.  Please
add a bunch of WebFrameTests for every behavior you care about.

https://codereview.chromium.org/23441020/diff/13001/Source/core/platform/Scro...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/23441020/diff/13001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:214: return m_fixedLayoutSize.isZero() ||
!m_useFixedLayout ? unscaledVisibleContentSize(scrollbarInclusion) :
m_fixedLayoutSize;
Please add a unit test verifying that empty but nonzero sizes behave as you
expect.

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

https://codereview.chromium.org/23441020/diff/13001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:2846: view->setVisibleContentScaleFactor(1.0f);
Hmm, instead of setting this here, I suggest that the fixedLayoutSizeLock
boolean lives on FrameView (not WebViewImpl), and then you can look at it in the
getter for visibleContentScaleFactor.  Maybe this would also fix the minor pinch
zoom glitch you mentioned.

https://codereview.chromium.org/23441020/diff/13001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:2978: if (!m_fixedLayoutSizeLock)
Please move this up to the outer if():

if (settings()->viewportEnabled() && !m_fixedLayoutSizeLock)

https://codereview.chromium.org/23441020/diff/13001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3100: if (m_fixedLayoutSizeLock)
Please take a look at https://codereview.chromium.org/23819019/ which is working
in the same area.  That said your patch needs to land first since we need to
cherry-pick it to M30.  But we'll also need to figure out how this works in the
future world where "fixed layout mode" no longer exists.

[aelias_OOO_until_Jul13, 2013-09-06 02:02:52]

[+bokan as FYI]

[mkosiba (inactive), 2013-09-06 15:48:51]

Thanks Alex!

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

https://codereview.chromium.org/23441020/diff/13001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:2846: view->setVisibleContentScaleFactor(1.0f);
On 2013/09/06 01:50:06, aelias wrote:
> Hmm, instead of setting this here, I suggest that the fixedLayoutSizeLock
> boolean lives on FrameView (not WebViewImpl), and then you can look at it in
the
> getter for visibleContentScaleFactor.  Maybe this would also fix the minor
pinch
> zoom glitch you mentioned.

Done. Unfortunately this doesn't fix the glitch.

https://codereview.chromium.org/23441020/diff/13001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:2978: if (!m_fixedLayoutSizeLock)
On 2013/09/06 01:50:06, aelias wrote:
> Please move this up to the outer if():
> 
> if (settings()->viewportEnabled() && !m_fixedLayoutSizeLock)

Done.

https://codereview.chromium.org/23441020/diff/13001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3100: if (m_fixedLayoutSizeLock)
On 2013/09/06 01:50:06, aelias wrote:
> Please take a look at https://codereview.chromium.org/23819019/ which is
working
> in the same area.  That said your patch needs to land first since we need to
> cherry-pick it to M30.  But we'll also need to figure out how this works in
the
> future world where "fixed layout mode" no longer exists.

hmm.. that doesn't look too bad I think. This code would continue to work in a
similar way: 'locking' the layoutSize to a particular value, regardless
of what the WebViewImpl was resized to.

[mkosiba (inactive), 2013-09-06 16:45:28]

I've added tests and moved some of the code into ScrollView. PTAL and if you
like this please add an OWNER to the review.

[aelias_OOO_until_Jul13, 2013-09-06 23:11:18]

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

https://codereview.chromium.org/23441020/diff/26001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1672: 
nit: unnecessary newline diff

https://codereview.chromium.org/23441020/diff/26001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1689: if (scaleMultiplier != 1 &&
!fixedLayoutSizeLock()) {
I don't think you want the else clause of this code either.  You just want the
scroll offset to stay at zero without any fancy business.  I suggest moving this
condition up to:

bool shouldAnchorAndRescaleViewport = settings()->viewportEnabled() &&
!fixedLayoutSizeLock() && ...

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

https://codereview.chromium.org/23441020/diff/26001/public/web/WebView.h#newc...
public/web/WebView.h:317: virtual void setFixedLayoutSizeLock(bool) = 0;
I looked at this more carefully and you don't need to add this new API.  Just
change the meaning of WebView::setFixedLayoutSize().  It has no real callers on
any platform at the moment.

When a Chromium-side caller calls setFixedLayoutSize, set your lock bool and the
FrameView fixedLayoutSize.  Clear the bool if it's called with WebSize(0,0).

And change the last line of WebViewImpl::updatePageDefinedPageScaleConstraints
to stop calling the API method and do instead:

if (!fixedLayoutSizeLock())
  frame->view()->setFixedLayoutSize(layoutSize);

So I'm proposing to redefine the concept of "fixed layout" to fit your use case
and mean literally that, the layout size is fixed from Chromium separately from
the view size and Blink never spontaneously changes it.

[mkosiba (inactive), 2013-09-10 09:11:31]

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

https://codereview.chromium.org/23441020/diff/26001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1672: 
On 2013/09/06 23:11:18, aelias wrote:
> nit: unnecessary newline diff

Done.

https://codereview.chromium.org/23441020/diff/26001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:1689: if (scaleMultiplier != 1 &&
!fixedLayoutSizeLock()) {
On 2013/09/06 23:11:18, aelias wrote:
> I don't think you want the else clause of this code either.  You just want the
> scroll offset to stay at zero without any fancy business.  I suggest moving
this
> condition up to:
> 
> bool shouldAnchorAndRescaleViewport = settings()->viewportEnabled() &&
> !fixedLayoutSizeLock() && ...

Done.

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

https://codereview.chromium.org/23441020/diff/26001/public/web/WebView.h#newc...
public/web/WebView.h:317: virtual void setFixedLayoutSizeLock(bool) = 0;
On 2013/09/06 23:11:18, aelias wrote:
> I looked at this more carefully and you don't need to add this new API.  Just
> change the meaning of WebView::setFixedLayoutSize().  It has no real callers
on
> any platform at the moment.
> 
> When a Chromium-side caller calls setFixedLayoutSize, set your lock bool and
the
> FrameView fixedLayoutSize.  Clear the bool if it's called with WebSize(0,0).
> 
> And change the last line of WebViewImpl::updatePageDefinedPageScaleConstraints
> to stop calling the API method and do instead:
> 
> if (!fixedLayoutSizeLock())
>   frame->view()->setFixedLayoutSize(layoutSize);
> 
> So I'm proposing to redefine the concept of "fixed layout" to fit your use
case
> and mean literally that, the layout size is fixed from Chromium separately
from
> the view size and Blink never spontaneously changes it.

ah, cool!
Thanks for looking. I keep forgetting that these APIs might have been added for 
a different WebKit port and the fact that they're public doesn't neccessarily
mean that they're used in Chrome.

[mkosiba (inactive), 2013-09-10 12:54:45]

+jamesr for Source/core/platform/ScrollView.h OWNERS review

[aelias_OOO_until_Jul13, 2013-09-10 18:40:18]

https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:209: if (!m_fixedLayoutSizeLock)
Hmm, I don't think this is the right place for this.  Note that
WebInputEventConversion.cpp uses visibleContentScaleFactor to scale input event
position (and I believe you want them to *not* be scaled -- please try out if
single-tap navigation events do the right thing when pageScaleFactor != 1). 
Could you move the variable to FrameView::m_enableVisibleContentScaleFactor and
return 1 from visibleContentScaleFactor() instead?

https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:226: if (m_fixedLayoutSizeLock)
You can get rid of this early out and it would simplify the code in WebViewImpl.

[mkosiba (inactive), 2013-09-11 16:31:17]

https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:209: if (!m_fixedLayoutSizeLock)
On 2013/09/10 18:40:18, aelias wrote:
> Hmm, I don't think this is the right place for this.  Note that
> WebInputEventConversion.cpp uses visibleContentScaleFactor to scale input
event
> position (and I believe you want them to *not* be scaled -- please try out if
> single-tap navigation events do the right thing when pageScaleFactor != 1). 
> Could you move the variable to FrameView::m_enableVisibleContentScaleFactor
and
> return 1 from visibleContentScaleFactor() instead?

Single tap navigations work with the way it is now. If I follow your suggestion
then single tap navigations do get out of whack.

I don't understand why the visibleContentRect needs to be sized by 1/pageScale
and not by 1*pageScale btw. I'm assuming the purpose of this is so that
pinch-zoomed
contents become scrollable, right?

The only reason we need this line is to fix fixed-position, is it possible that
the
scaling here is correct and the fixed pos bug lies elsewhere?

https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:226: if (m_fixedLayoutSizeLock)
On 2013/09/10 18:40:18, aelias wrote:
> You can get rid of this early out and it would simplify the code in
WebViewImpl.

Done.

[aelias_OOO_until_Jul13, 2013-09-11 19:58:38]

Hmm, looks like you forgot to upload the latest patch, so it's unfortunately
hard to give a final review.  I agree it sounds like it needs to be applied in
ScrollView, but in case you didn't rename it, I still suggest renaming the new
bool, maybe to m_applyVisibleContentScaleFactor.

https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:209: if (!m_fixedLayoutSizeLock)
On 2013/09/11 16:31:18, mkosiba wrote:
> On 2013/09/10 18:40:18, aelias wrote:
> > Hmm, I don't think this is the right place for this.  Note that
> > WebInputEventConversion.cpp uses visibleContentScaleFactor to scale input
> event
> > position (and I believe you want them to *not* be scaled -- please try out
if
> > single-tap navigation events do the right thing when pageScaleFactor != 1). 
> > Could you move the variable to FrameView::m_enableVisibleContentScaleFactor
> and
> > return 1 from visibleContentScaleFactor() instead?
> 
> Single tap navigations work with the way it is now. If I follow your
suggestion
> then single tap navigations do get out of whack.

OK, thanks for checking.

> 
> I don't understand why the visibleContentRect needs to be sized by 1/pageScale
> and not by 1*pageScale btw. I'm assuming the purpose of this is so that
> pinch-zoomed
> contents become scrollable, right?

Yes, basically.  visibleContentRect is in CSS pixel space, therefore it gets
smaller as the page scale gets higher (the viewport shrinks relative to the
contents).

> 
> The only reason we need this line is to fix fixed-position, is it possible
that
> the
> scaling here is correct and the fixed pos bug lies elsewhere?

I think changing it here is the right place.

Well, the problem is that fixed-pos is also positioned relative to
visibleContentRect and with the scaling it fails to represent the true visible
area in your WRAP_CONTENTS case.  This rect is also used to clip input events
relative to the visible area, so if you did something specific to fixed-pos
you'd run into problems there.

We may be able to revisit this when we split into two concepts of viewport,
partly to allow more sophisticated fixed-pos behavior.  See
https://docs.google.com/a/chromium.org/document/d/1KewSqexq4Pd99RUFB-yVoUIJoc...

[mkosiba (inactive), 2013-09-11 22:21:39]

On 2013/09/11 19:58:38, aelias wrote:
> Hmm, looks like you forgot to upload the latest patch, so it's unfortunately
> hard to give a final review.  I agree it sounds like it needs to be applied in
> ScrollView, but in case you didn't rename it, I still suggest renaming the new
> bool, maybe to m_applyVisibleContentScaleFactor.

ah, sorry about that - just did that a moment ago.

> 
>
https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
> File Source/core/platform/ScrollView.cpp (right):
> 
>
https://codereview.chromium.org/23441020/diff/36001/Source/core/platform/Scro...
> Source/core/platform/ScrollView.cpp:209: if (!m_fixedLayoutSizeLock)
> On 2013/09/11 16:31:18, mkosiba wrote:
> > On 2013/09/10 18:40:18, aelias wrote:
> > > Hmm, I don't think this is the right place for this.  Note that
> > > WebInputEventConversion.cpp uses visibleContentScaleFactor to scale input
> > event
> > > position (and I believe you want them to *not* be scaled -- please try out
> if
> > > single-tap navigation events do the right thing when pageScaleFactor !=
1). 
> > > Could you move the variable to
FrameView::m_enableVisibleContentScaleFactor
> > and
> > > return 1 from visibleContentScaleFactor() instead?
> > 
> > Single tap navigations work with the way it is now. If I follow your
> suggestion
> > then single tap navigations do get out of whack.
> 
> OK, thanks for checking.
> 
> > 
> > I don't understand why the visibleContentRect needs to be sized by
1/pageScale
> > and not by 1*pageScale btw. I'm assuming the purpose of this is so that
> > pinch-zoomed
> > contents become scrollable, right?
> 
> Yes, basically.  visibleContentRect is in CSS pixel space, therefore it gets
> smaller as the page scale gets higher (the viewport shrinks relative to the
> contents).

ah, ok, the visibleContentRect is the viewport, not the size of the scrollable
region. even says so in the comments, duh!

> 
> > 
> > The only reason we need this line is to fix fixed-position, is it possible
> that
> > the
> > scaling here is correct and the fixed pos bug lies elsewhere?
> 
> I think changing it here is the right place.
> 
> Well, the problem is that fixed-pos is also positioned relative to
> visibleContentRect and with the scaling it fails to represent the true visible
> area in your WRAP_CONTENTS case. This rect is also used to clip input events
> relative to the visible area, so if you did something specific to fixed-pos
> you'd run into problems there.

thanks for explaining. 

> 
> We may be able to revisit this when we split into two concepts of viewport,
> partly to allow more sophisticated fixed-pos behavior.  See
>
https://docs.google.com/a/chromium.org/document/d/1KewSqexq4Pd99RUFB-yVoUIJoc...

I'll take a look.

[mkosiba (inactive), 2013-09-11 22:44:44]

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

https://codereview.chromium.org/23441020/diff/42001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3113:
view->enableVisibleContentSizeScaling(!m_fixedLayoutSizeLock);
shall I add a comment on why this is here? It seems a bit unrelated to the rest
of the code

[aelias_OOO_until_Jul13, 2013-09-11 22:48:37]

lgtm modulo minor comments below.  jamesr@, could you review the
Source/core/platform/ changes?

https://codereview.chromium.org/23441020/diff/42001/Source/core/platform/Scro...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/23441020/diff/42001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:216: m_enableVisibleContentSizeScaling =
value;
Add an early return here to avoid calling contentsResized() if the value doesn't
change.

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

https://codereview.chromium.org/23441020/diff/42001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3113:
view->enableVisibleContentSizeScaling(!m_fixedLayoutSizeLock);
On 2013/09/11 22:44:45, mkosiba wrote:
> shall I add a comment on why this is here? It seems a bit unrelated to the
rest
> of the code

Go ahead.

[mkosiba (inactive), 2013-09-12 17:01:43]

I took out disabling visibleContentsRect scaling from this patch and took out
dividing the viewport size and physical_backing_size by pageScaleFactor out of
the AwLayoutSize in the Chrome-side CL
(https://codereview.chromium.org/23478022/) and wrap_contents mode continues to
work.

Alex - does that sound reasonable?

[aelias_OOO_until_Jul13, 2013-09-12 18:51:24]

Still lgtm, that's a great cleanup.  jamesr@, could you OWNERS review the small
ScrollView.cpp change?  This is an M30 blocker for WebView.

[jamesr, 2013-09-12 23:22:31]

lgtm

[commit-bot: I haz the power, 2013-09-13 09:38:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/23441020/48001

[commit-bot: I haz the power, 2013-09-13 10:21:00]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-09-13 11:41:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/23441020/48001

[commit-bot: I haz the power, 2013-09-13 12:33:02]

Change committed as 157749