Update WebViewImpl::contentsPreferredMinimumSize to be agnostic to the scrolling element

Update WebViewImpl::contentsPreferredMinimumSize to be agnostic to the scrolling element

When the scrollingElement is the documentElement, documentElement.scrollHeight behaves
differently (and in unit tests may return 0).  So make sure that
contentsPreferredMinimumSize always uses the document element's layout box directly.

This is a no-op today, but will ensure the behavior of this API doesn't change
when we enable the scrollTopLeftInterop feature.

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

[Rick Byers, 2015-08-13 00:57:31]

rbyers@chromium.org changed reviewers:
+ bokan@chromium.org

[Rick Byers, 2015-08-13 00:57:32]

bokan@ PTAL

This doesn't update any tests because today in unit tests the scrollingElement
is always the body.  Instead this is tested by
https://codereview.chromium.org/1216953003/ (you can see
WebViewTest.PreferredSize was failing without this change).

I want to land this separately in case we need to roll-back the other CL.

[bokan, 2015-08-13 15:49:04]

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

https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp#...
Source/web/WebViewImpl.cpp:3406: int heightScaled =
static_cast<int>(adjustLayoutUnitForAbsoluteZoom(box->scrollHeight(), *box) *
zoomLevelToZoomFactor(zoomLevel()) + 0.5); // +0.5 to round rather than
truncating
Did you forget to remove the second zoom factor? It looks like we're double
applying. Also, just use .round() on the layout unit instead of the cast and '+
0.5'.

[Rick Byers, 2015-08-13 16:51:54]

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

https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp#...
Source/web/WebViewImpl.cpp:3406: int heightScaled =
static_cast<int>(adjustLayoutUnitForAbsoluteZoom(box->scrollHeight(), *box) *
zoomLevelToZoomFactor(zoomLevel()) + 0.5); // +0.5 to round rather than
truncating
On 2015/08/13 15:49:04, bokan wrote:
> Did you forget to remove the second zoom factor? It looks like we're double
> applying.

I was attempting to leave the behavior exactly the same as before.  I _think_
adjustLayoutUnitForAbsoluteZoom is applying only css zoom, where
zoomLevelToZoomFactor is applying page zoom.  

> Also, just use .round() on the layout unit instead of the cast and '+
> 0.5'.

The result of a LayoutUnit times a double is a double.  Of course I could use
'round' instead of this ugly addition, but the cast is still necessary.

[bokan, 2015-08-13 17:33:23]

On 2015/08/13 16:51:54, Rick Byers wrote:
> https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp#...
> Source/web/WebViewImpl.cpp:3406: int heightScaled =
> static_cast<int>(adjustLayoutUnitForAbsoluteZoom(box->scrollHeight(), *box) *
> zoomLevelToZoomFactor(zoomLevel()) + 0.5); // +0.5 to round rather than
> truncating
> On 2015/08/13 15:49:04, bokan wrote:
> > Did you forget to remove the second zoom factor? It looks like we're double
> > applying.
> 
> I was attempting to leave the behavior exactly the same as before.  I _think_
> adjustLayoutUnitForAbsoluteZoom is applying only css zoom, where
> zoomLevelToZoomFactor is applying page zoom.  

I just tried this quickly by printf'ing the document's style()->effectiveZoom()
(which adjustLayoutUnitForAbsoluteZoom uses) and it seems to take both CSS zoom
*and* page zoom into account. Looks like the page zoom gets applied as the
starting zoom in the Document's computed style:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


> 
> > Also, just use .round() on the layout unit instead of the cast and '+
> > 0.5'.
> 
> The result of a LayoutUnit times a double is a double.  Of course I could use
> 'round' instead of this ugly addition, but the cast is still necessary.

[Rick Byers, 2015-08-13 18:28:37]

On 2015/08/13 17:33:23, bokan wrote:
> On 2015/08/13 16:51:54, Rick Byers wrote:
> > https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp
> > File Source/web/WebViewImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp#...
> > Source/web/WebViewImpl.cpp:3406: int heightScaled =
> > static_cast<int>(adjustLayoutUnitForAbsoluteZoom(box->scrollHeight(), *box)
*
> > zoomLevelToZoomFactor(zoomLevel()) + 0.5); // +0.5 to round rather than
> > truncating
> > On 2015/08/13 15:49:04, bokan wrote:
> > > Did you forget to remove the second zoom factor? It looks like we're
double
> > > applying.
> > 
> > I was attempting to leave the behavior exactly the same as before.  I
_think_
> > adjustLayoutUnitForAbsoluteZoom is applying only css zoom, where
> > zoomLevelToZoomFactor is applying page zoom.  
> 
> I just tried this quickly by printf'ing the document's
style()->effectiveZoom()
> (which adjustLayoutUnitForAbsoluteZoom uses) and it seems to take both CSS
zoom
> *and* page zoom into account. Looks like the page zoom gets applied as the
> starting zoom in the Document's computed style:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Interesting, thanks!  It sounds like we have an existing bug here with
WebView::contentsPreferredMinimumSize double applying page zoom, but that's not
what the WebViewTest.PreferredSize test is seeing.  It looks like only Android
WebView ever calls this API, so perhaps it's not surprising that it's broken in
page zoom (can webview ever trigger page zoom at all?).  I'll debug the test and
figure out why it fails if I remove the apparent double application.

> 
> 
> > 
> > > Also, just use .round() on the layout unit instead of the cast and '+
> > > 0.5'.
> > 
> > The result of a LayoutUnit times a double is a double.  Of course I could
use
> > 'round' instead of this ugly addition, but the cast is still necessary.

[bokan, 2015-08-13 18:33:33]

On 2015/08/13 18:28:37, Rick Byers wrote:
> On 2015/08/13 17:33:23, bokan wrote:
> > On 2015/08/13 16:51:54, Rick Byers wrote:
> > >
https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp
> > > File Source/web/WebViewImpl.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp#...
> > > Source/web/WebViewImpl.cpp:3406: int heightScaled =
> > > static_cast<int>(adjustLayoutUnitForAbsoluteZoom(box->scrollHeight(),
*box)
> *
> > > zoomLevelToZoomFactor(zoomLevel()) + 0.5); // +0.5 to round rather than
> > > truncating
> > > On 2015/08/13 15:49:04, bokan wrote:
> > > > Did you forget to remove the second zoom factor? It looks like we're
> double
> > > > applying.
> > > 
> > > I was attempting to leave the behavior exactly the same as before.  I
> _think_
> > > adjustLayoutUnitForAbsoluteZoom is applying only css zoom, where
> > > zoomLevelToZoomFactor is applying page zoom.  
> > 
> > I just tried this quickly by printf'ing the document's
> style()->effectiveZoom()
> > (which adjustLayoutUnitForAbsoluteZoom uses) and it seems to take both CSS
> zoom
> > *and* page zoom into account. Looks like the page zoom gets applied as the
> > starting zoom in the Document's computed style:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Interesting, thanks!  It sounds like we have an existing bug here with
> WebView::contentsPreferredMinimumSize double applying page zoom, but that's
not
> what the WebViewTest.PreferredSize test is seeing.  It looks like only Android
> WebView ever calls this API, so perhaps it's not surprising that it's broken
in
> page zoom (can webview ever trigger page zoom at all?).  I'll debug the test
and
> figure out why it fails if I remove the apparent double application.
> 
Looking at the test, my guess is it looks like we don't recompute style after
setting the zoom level. Since zoomLevel() reads the zoom variable directly it
gets accounted for there, but if we did a style recalc I'm guessing it would
then double apply.

[Rick Byers, 2015-08-13 20:27:16]

On 2015/08/13 18:33:33, bokan wrote:
> On 2015/08/13 18:28:37, Rick Byers wrote:
> > On 2015/08/13 17:33:23, bokan wrote:
> > > On 2015/08/13 16:51:54, Rick Byers wrote:
> > > >
> https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp
> > > > File Source/web/WebViewImpl.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1291723005/diff/1/Source/web/WebViewImpl.cpp#...
> > > > Source/web/WebViewImpl.cpp:3406: int heightScaled =
> > > > static_cast<int>(adjustLayoutUnitForAbsoluteZoom(box->scrollHeight(),
> *box)
> > *
> > > > zoomLevelToZoomFactor(zoomLevel()) + 0.5); // +0.5 to round rather than
> > > > truncating
> > > > On 2015/08/13 15:49:04, bokan wrote:
> > > > > Did you forget to remove the second zoom factor? It looks like we're
> > double
> > > > > applying.
> > > > 
> > > > I was attempting to leave the behavior exactly the same as before.  I
> > _think_
> > > > adjustLayoutUnitForAbsoluteZoom is applying only css zoom, where
> > > > zoomLevelToZoomFactor is applying page zoom.  
> > > 
> > > I just tried this quickly by printf'ing the document's
> > style()->effectiveZoom()
> > > (which adjustLayoutUnitForAbsoluteZoom uses) and it seems to take both CSS
> > zoom
> > > *and* page zoom into account. Looks like the page zoom gets applied as the
> > > starting zoom in the Document's computed style:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Interesting, thanks!  It sounds like we have an existing bug here with
> > WebView::contentsPreferredMinimumSize double applying page zoom, but that's
> not
> > what the WebViewTest.PreferredSize test is seeing.  It looks like only
Android
> > WebView ever calls this API, so perhaps it's not surprising that it's broken
> in
> > page zoom (can webview ever trigger page zoom at all?).  I'll debug the test
> and
> > figure out why it fails if I remove the apparent double application.
> > 
> Looking at the test, my guess is it looks like we don't recompute style after
> setting the zoom level. Since zoomLevel() reads the zoom variable directly it
> gets accounted for there, but if we did a style recalc I'm guessing it would
> then double apply.

Nope the code was already updating layout.  It's really stupid actually -
adjustLayoutUnitForAbsoluteZoom is REMOVING the page/csszoom (which was already
present in the value), and then we were adding it back.  So the right thing to
do is remove both, and the code is much simpler :-).

So when are you adding a typed co-ordinate system again <grin>?

[bokan, 2015-08-13 20:32:09]

> Nope the code was already updating layout.  It's really stupid actually -
> adjustLayoutUnitForAbsoluteZoom is REMOVING the page/csszoom (which was
already
> present in the value), and then we were adding it back.  So the right thing to
> do is remove both, and the code is much simpler :-).

Ugh, thanks for doing the digging. LGTM!

> 
> So when are you adding a typed co-ordinate system again <grin>?

Heh, one of these quarters... :)

[Rick Byers, 2015-08-13 20:53:36]

On 2015/08/13 20:32:09, bokan wrote:
> > Nope the code was already updating layout.  It's really stupid actually -
> > adjustLayoutUnitForAbsoluteZoom is REMOVING the page/csszoom (which was
> already
> > present in the value), and then we were adding it back.  So the right thing
to
> > do is remove both, and the code is much simpler :-).
> 
> Ugh, thanks for doing the digging. LGTM!

Thank you for pushing me on it - I was being lazy...

> > 
> > So when are you adding a typed co-ordinate system again <grin>?
> 
> Heh, one of these quarters... :)

[Rick Byers, 2015-08-13 20:53:41]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2015-08-13 20:54:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291723005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291723005/20001

[commit-bot: I haz the power, 2015-08-13 22:07:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 22:07:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Rick Byers, 2015-08-13 23:07:27]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2015-08-13 23:07:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291723005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291723005/20001

[commit-bot: I haz the power, 2015-08-14 01:14:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-14 01:14:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Rick Byers, 2015-08-14 13:06:21]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2015-08-14 13:06:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291723005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291723005/20001

[commit-bot: I haz the power, 2015-08-14 14:15:03]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200540