Avoid auto-resize re-entrancy problems.

Avoid auto-resize re-entrancy problems.

ASSERT(needsLayout()) failed in various layout() methods. The reason is
that autoSizeIfEnabled() will re-enter FrameView::layout(), and upon return,
the rootForThisLayout RenderObject has already been layed out.

This CL moves the auto-sizing up to the top of FrameView::layout() to avoid
the re-entrancy problem with rootForThisLayout.

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

[rune, 2014-08-14 20:51:39]

The auto-resize feature is used in pop-up windows for extensions in Opera. This
causes a bunch of asserts during layout depending on the extension content.

Dynamically adapting the size of the FrameView to content that is ideally made
to adapt to the size of the FrameView freaks me out a bit, though.

[rune, 2014-08-14 20:53:41]

https://codereview.chromium.org/473903002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/473903002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:802: autoSizeIfEnabled();

Perhaps this should be done after the layout instead?

[rune, 2014-08-15 09:27:15]

https://codereview.chromium.org/473903002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/473903002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:804: return;
This caused regressions. needsLayout() might be false when layout is needed it
seems. This needs to go.

[rune, 2014-08-21 14:48:35]

PTAL

[levin, 2014-08-22 02:11:01]

Seems fine to me as long as autoresize works in your tests and this could only
affect layout when autoresize is turned on. (In Chrome, at this point, I think
that is only the webview and the pop up for extensions.)

I'm a bit reluctant to do a lgtm (for anything in blink) largely because I
haven't worked on it for years (even though I may still in an owners file...
which I likely shouldn't be).

But I was likely the last person to touch this feature.

https://codereview.chromium.org/473903002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/473903002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:805: autoSizeIfEnabled();
Why not after protector and changeInProgrammaticScroll?

https://codereview.chromium.org/473903002/diff/40001/Source/web/tests/WebView...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/473903002/diff/40001/Source/web/tests/WebView...
Source/web/tests/WebViewTest.cpp:2159: TEST_F(WebViewTest,
AutoResizeSubtreeLayout)
If you care about this area, and you seem to, there are at least ~4 disabled
test in WebViewTest related to AutoSize. 

They all got disabled well after I stopped working on Chrome, but it would be
awesome if someone got them turned back on. (It would provide more confidence in
this feature continuing to work.)

[rune, 2014-08-22 08:52:27]

Thanks for reviewing!

The reason I care is that the ASSERT is an annoyance to our Desktop product devs
trying to debug extensions code. As you say, it's used in extension pop-ups.

https://codereview.chromium.org/473903002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/473903002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:805: autoSizeIfEnabled();
On 2014/08/22 02:11:00, levin wrote:
> Why not after protector and changeInProgrammaticScroll?

Because I thought those would be called when FrameView::layout is re-entered
from autoSizeIfEnabled(). There should be no harm having those twice on the
stack, so I'll move it down for safety.

https://codereview.chromium.org/473903002/diff/40001/Source/web/tests/WebView...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/473903002/diff/40001/Source/web/tests/WebView...
Source/web/tests/WebViewTest.cpp:2159: TEST_F(WebViewTest,
AutoResizeSubtreeLayout)
On 2014/08/22 02:11:00, levin wrote:
> If you care about this area, and you seem to, there are at least ~4 disabled
> test in WebViewTest related to AutoSize. 
> 
> They all got disabled well after I stopped working on Chrome, but it would be
> awesome if someone got them turned back on. (It would provide more confidence
in
> this feature continuing to work.)

They pass for me when I enable them locally.

Running them through some bots now: https://codereview.chromium.org/500453003

[rune, 2014-08-22 11:05:15]

On 2014/08/22 at 08:52:27, rune wrote:
> > They all got disabled well after I stopped working on Chrome, but it would
be
> > awesome if someone got them turned back on. (It would provide more
confidence in
> > this feature continuing to work.)
> 
> They pass for me when I enable them locally.

They pass locally with the changes in this CL too. I'd like to keep the enabling
in a different CL to make it easier to revert if it fails on some bot after
landing.

[levin, 2014-08-22 16:02:01]

On 2014/08/22 08:52:27, rune wrote:
> Thanks for reviewing!
> 
> The reason I care is that the ASSERT is an annoyance to our Desktop product
devs
> trying to debug extensions code. As you say, it's used in extension pop-ups.

fwiw, I totally agree. Asserts shouldn't be going off.

> I'd like to keep the enabling in a different CL to make it easier to revert if
it fails on some bot after landing.

Totally agree with that too.

[rune, 2014-08-27 10:40:22]

@levin wanted someone else to approve.

@abarth or @eseidel, PTAL?

[rune, 2014-08-29 09:02:48]

On 2014/08/27 at 10:40:22, rune wrote:
> @levin wanted someone else to approve.
> 
> @abarth or @eseidel, PTAL?

ping?

[abarth-chromium, 2014-09-02 19:51:39]

You mention an early return in your description.  Where is that in the code?

[rune, 2014-09-02 20:21:28]

On 2014/09/02 at 19:51:39, abarth wrote:
> You mention an early return in your description.  Where is that in the code?

It was in the initial patch, but removed because tests started failing.
Apparently, needsLayout() can be false when entering FrameView::layout() when a
layout is needed. I've modified the description now.

[abarth-chromium, 2014-09-02 20:23:53]

lgtm

[rune, 2014-09-02 20:26:43]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-09-02 20:27:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rune@opera.com/473903002/80001

[commit-bot: I haz the power, 2014-09-02 21:11:02]

Committed patchset #5 (id:80001) as 181258