Update parent layout when child frame requests innerHeight/innerWidth.

Update parent layout when child frame requests innerHeight/innerWidth.

The child frame dimensions may depend on the parent frame's layout. So
we need to update the parent frame's layout when the child asks for
innerHeight or innerWidth.

Without this patch, the test case incorrectly returns 0 for the
innerHeight/innerWidth queriers prior to the forced layout.

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

[tonyg, 2013-07-16 00:24:20]

ptal

[eseidel, 2013-07-16 00:28:29]

lgtm

Bleh.  I suspect we do need this.  But we also need to give this a smaller
hammer. :(

[eseidel, 2013-07-16 00:29:15]

Actually. Why do we need to do this here?  This is where we send off the load,
no?  Why not do the layout once we have the load back?

[tonyg, 2013-07-16 00:48:25]

On 2013/07/16 00:29:15, eseidel wrote:
> Actually. Why do we need to do this here?  This is where we send off the load,
> no?  Why not do the layout once we have the load back?

Because I want it done before SubframeLoader::loadSubframe calls setWidget(view)
which sets the frame's geometry. I moved the layout there to make this
relationship more obvious. Do you still think there is a good way to defer this
farther?

[esprehn, 2013-07-16 00:49:41]

not lgtm. You can't trigger it in there, this will cause 100 layouts and style
recalcs on a page with 100 iframes one for each time the parser appends an
<iframe> element.

I think you want to hook this on the load event, even then you should check for
about:blank iframes otherwise we're going to do terrible things for perf on
pages like G+ or facebook that have lots of iframes they appendChild().

https://codereview.chromium.org/19272007/diff/1/LayoutTests/fast/frames/scrip...
File LayoutTests/fast/frames/script-tests/frame-height-before-parent-layout.js
(right):

https://codereview.chromium.org/19272007/diff/1/LayoutTests/fast/frames/scrip...
LayoutTests/fast/frames/script-tests/frame-height-before-parent-layout.js:5: 
Lets not add more of these script tests, can you just put the script inside the
html file?

https://codereview.chromium.org/19272007/diff/1/Source/core/html/HTMLFrameEle...
File Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/19272007/diff/1/Source/core/html/HTMLFrameEle...
Source/core/html/HTMLFrameElementBase.cpp:91:
document()->updateLayoutIgnorePendingStylesheets();
This isn't the right place to do this, you're going to cause hundreds of layouts
and recalc styles on pages with iframes as the parser appends them to the
document.

[tonyg, 2013-07-16 00:53:59]

> I think you want to hook this on the load event,

Good point. I'm really nervous about the perf of this patch and want to make it
as light as possible.

Unfortunately, I don't think the subframe's load event will be sufficient. The
subframe could easily query its own dimensions prior to its load event. In fact,
my test case should demonstrate that (it doesn't wait for the frame to load).

[eseidel, 2013-07-16 01:05:29]

How does the subframe query its own dimentions?  Shouldn't any of those queries
force a layout? (or at least ASSERT that things are already laid out?)

Eventually I'd like direct most non-rendering-internal callers to use methods
which either update layout/style for you, or ASSERT that it has been done.

[esprehn, 2013-07-16 01:08:03]

On 2013/07/16 00:53:59, tonyg wrote:
> > I think you want to hook this on the load event,
> 
> Good point. I'm really nervous about the perf of this patch and want to make
it
> as light as possible.
> 
> Unfortunately, I don't think the subframe's load event will be sufficient. The
> subframe could easily query its own dimensions prior to its load event. In
fact,
> my test case should demonstrate that (it doesn't wait for the frame to load).

I think you want to solve this at a different level, touching innerHeight or
innerWidth should cause a layout of the parent document to make sure the values
are correct. Document::updateLayout() already knows to updateLayout() on the
parent document (recursively up the tree), it's not clear why innerHeight
doesn't do that. Any way the iframe attempts to query it's size should cause the
layout.

[tonyg, 2013-07-16 02:48:05]

On 2013/07/16 01:08:03, esprehn wrote:
> On 2013/07/16 00:53:59, tonyg wrote:
> > > I think you want to hook this on the load event,
> > 
> > Good point. I'm really nervous about the perf of this patch and want to make
> it
> > as light as possible.
> > 
> > Unfortunately, I don't think the subframe's load event will be sufficient.
The
> > subframe could easily query its own dimensions prior to its load event. In
> fact,
> > my test case should demonstrate that (it doesn't wait for the frame to
load).
> 
> I think you want to solve this at a different level, touching innerHeight or
> innerWidth should cause a layout of the parent document to make sure the
values
> are correct. Document::updateLayout() already knows to updateLayout() on the
> parent document (recursively up the tree), it's not clear why innerHeight
> doesn't do that. Any way the iframe attempts to query it's size should cause
the
> layout.

You are right. My first patch probably would have been a disaster for perf.

Please check out the updated approach of only forcing layout of the parent in
innerWidth/innerHeight.

[tonyg, 2013-07-16 02:48:32]

On 2013/07/16 01:08:03, esprehn wrote:
> On 2013/07/16 00:53:59, tonyg wrote:
> > > I think you want to hook this on the load event,
> > 
> > Good point. I'm really nervous about the perf of this patch and want to make
> it
> > as light as possible.
> > 
> > Unfortunately, I don't think the subframe's load event will be sufficient.
The
> > subframe could easily query its own dimensions prior to its load event. In
> fact,
> > my test case should demonstrate that (it doesn't wait for the frame to
load).
> 
> I think you want to solve this at a different level, touching innerHeight or
> innerWidth should cause a layout of the parent document to make sure the
values
> are correct. Document::updateLayout() already knows to updateLayout() on the
> parent document (recursively up the tree), it's not clear why innerHeight
> doesn't do that. Any way the iframe attempts to query it's size should cause
the
> layout.

You are right. My first patch probably would have been a disaster for perf.

Please check out the updated approach of only forcing layout of the parent in
innerWidth/innerHeight.

[esprehn, 2013-07-16 02:58:50]

LGTM

https://codereview.chromium.org/19272007/diff/14001/Source/core/page/DOMWindo...
File Source/core/page/DOMWindow.cpp (right):

https://codereview.chromium.org/19272007/diff/14001/Source/core/page/DOMWindo...
Source/core/page/DOMWindow.cpp:1028: // FIXME: This is potentially too much
work. We really only need to know the dimensions of the parent frame's renderer.
I don't think you need the fixme, this is true of lots
updateLayoutIgnorePendingStylesheets calls. You can leave it if you want though.

[tonyg, 2013-07-16 03:31:44]

On 2013/07/16 02:58:50, esprehn wrote:
> LGTM
> 

Thanks again for your help with this.

>
https://codereview.chromium.org/19272007/diff/14001/Source/core/page/DOMWindo...
> File Source/core/page/DOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/19272007/diff/14001/Source/core/page/DOMWindo...
> Source/core/page/DOMWindow.cpp:1028: // FIXME: This is potentially too much
> work. We really only need to know the dimensions of the parent frame's
renderer.
> I don't think you need the fixme, this is true of lots
> updateLayoutIgnorePendingStylesheets calls. You can leave it if you want
though.

Since it is okay with you, I'll leave them in. When we do make this better,
these call sites will have to be changed to the cheaper incantation. And if
these happen to show up as a bottle neck for someone, the FIXMEs will point them
in the right direction towards making it better.

[commit-bot: I haz the power, 2013-07-16 03:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/19272007/14001

[commit-bot: I haz the power, 2013-07-16 09:09:53]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-07-16 13:56:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/19272007/14001

[commit-bot: I haz the power, 2013-07-16 14:35:42]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-07-16 14:39:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/19272007/14001

[commit-bot: I haz the power, 2013-07-16 15:23:54]

Change committed as 154292