Make logical height calculations on RenderView use unscaled FrameRect

Make logical height calculations on RenderView use unscaled FrameRect

RenderBox's methods for computing logical height would use
frameView()->visibleWidth() and visibleHeight() once they
reach the RenderView. These methods return the frame's
dimensions divided by the content scale factor (i.e. pinch-zoom).
This was causing problems in the linked bug since the method
would remove scrollbars before applying scale, causing the removed
scrollbar area to scale with the page. This CL changes the
occurances of visibleWidth/Height to unscaledVisibleContentSize().

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

[bokan, 2014-08-12 23:06:08]

Hi Julien, PTAL?

I'm not really sure how to test this and I think it'd be more trouble than it's
worth since the new virtual viewport content scaling will make it's way to
remaining platforms next milestone and this code path wont be invoked at all
under pinch. Let me know if you have other ideas. Thanks!

[Julien - ping for review, 2014-08-15 18:25:19]

The change looks fine but can't we have a test for that?

https://codereview.chromium.org/462243002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/462243002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2858: return isHorizontalWritingMode() ?
toRenderView(this)->frameView()->unscaledVisibleContentSize().height() :
toRenderView(this)->frameView()->unscaledVisibleContentSize().width();
AFAICT those were the only 2 uses of visibleWidth / visibleHeight outside
FrameView. Shouldn't we make the 2 functions private to avoid more issues as
people write more code?

[bokan, 2014-08-15 22:50:42]

PTAL, test added and the visibleWidth/Height concern (hopefully) addressed.
Thanks.

https://codereview.chromium.org/462243002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/462243002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2858: return isHorizontalWritingMode() ?
toRenderView(this)->frameView()->unscaledVisibleContentSize().height() :
toRenderView(this)->frameView()->unscaledVisibleContentSize().width();
On 2014/08/15 18:25:19, Julien Chaffraix - PST wrote:
> AFAICT those were the only 2 uses of visibleWidth / visibleHeight outside
> FrameView. Shouldn't we make the 2 functions private to avoid more issues as
> people write more code?

visibleWidth/Height are actually declared in ScrollableArea and defined in
ScrollView (they're used elsewhere, e.g. WebLocalFrameImpl) so there'd be a
little more work to remove. Like I said, this will be a moot point in a
milestone or so when FrameView no longer has page scale attached to it, but I've
filed crbug.com/404284 against myself to clean these up once that happens.

[Julien - ping for review, 2014-08-16 00:54:17]

lgtm

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
File LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html (right):

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:1: <title>
DOCTYPE PLEASE!!

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:2: This test
checks that page scale does not affect a plugin object's height when it depends
on the window height
Let's include an expected result (dumped into the output).

Also it's good to include instructions for people testing manually (like a
gardener trying to understand what's going on with this test).

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:3: </title>
I don't like using title as it doesn't dump into the output (and is not shown in
DRT at all): the information is important enough to put it, it should be dumped
into the output.

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:12:
document.body.innerHTML =
internals.elementRenderTreeAsText(document.getElementById('box'));
I would keep the description about in the output.

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:15: <body
marginwidth="0" marginheight="0" onload="runTest()">
I just learned about these 2 attributes.

Nit: I would prefer some correct CSS instead of relying on some obscure
ancillary attribute (wouldn't surprise me if they would be put to the
deprecation roster one of these days).

[bokan, 2014-08-18 14:27:33]

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
File LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html (right):

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:1: <title>
On 2014/08/16 00:54:16, Julien Chaffraix - PST wrote:
> DOCTYPE PLEASE!!

Done.

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:2: This test
checks that page scale does not affect a plugin object's height when it depends
on the window height
On 2014/08/16 00:54:16, Julien Chaffraix - PST wrote:
> Let's include an expected result (dumped into the output).
> 
> Also it's good to include instructions for people testing manually (like a
> gardener trying to understand what's going on with this test).

Done.

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:3: </title>
On 2014/08/16 00:54:16, Julien Chaffraix - PST wrote:
> I don't like using title as it doesn't dump into the output (and is not shown
in
> DRT at all): the information is important enough to put it, it should be
dumped
> into the output.

Done.

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:12:
document.body.innerHTML =
internals.elementRenderTreeAsText(document.getElementById('box'));
On 2014/08/16 00:54:17, Julien Chaffraix - PST wrote:
> I would keep the description about in the output.

Done.

https://codereview.chromium.org/462243002/diff/40001/LayoutTests/plugins/page...
LayoutTests/plugins/page-scale-does-not-affect-plugin-height.html:15: <body
marginwidth="0" marginheight="0" onload="runTest()">
On 2014/08/16 00:54:16, Julien Chaffraix - PST wrote:
> I just learned about these 2 attributes.
> 
> Nit: I would prefer some correct CSS instead of relying on some obscure
> ancillary attribute (wouldn't surprise me if they would be put to the
> deprecation roster one of these days).

Done.

[bokan, 2014-08-18 14:27:40]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-08-18 14:28:13]

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

[commit-bot: I haz the power, 2014-08-18 15:39:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23243)

[commit-bot: I haz the power, 2014-08-18 16:14:01]

Committed patchset #4 (60001) as 180471