Fix ASSERT(needsLayout()) failing when autosize mode is enabled

Fix ASSERT(needsLayout()) failing when autosize mode is enabled

This is just a revert of http://trac.webkit.org/changeset/147373

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

[mthiesse, 2013-04-16 20:57:48]

Sending to levin@ for review because he reviewed
http://trac.webkit.org/changeset/147373 (and wrote the autosize code
originally).

Any idea who would be the proper OWNER for this?

[mthiesse, 2013-04-16 20:59:57]

On 2013/04/16 20:57:48, mthiesse wrote:
> Sending to levin@ for review because he reviewed
> http://trac.webkit.org/changeset/147373 (and wrote the autosize code
> originally).
> 
> Any idea who would be the proper OWNER for this?

Oh, you are an OWNER, should be good then :P

[levin, 2013-04-16 21:22:05]

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

https://codereview.chromium.org/14139021/diff/1/Source/WebCore/page/FrameView...
Source/WebCore/page/FrameView.cpp:2445: resize(frameRect().width(),
m_minAutoSize.height());
This stops "unconditionally start laying out from the minimum height, so that
the
documentRect can shrink below its previous height if needed."

Why does this line cause an assert?

And why won't it cause an assert the first time that autosize is run?

[levin, 2013-04-16 21:27:03]

Consider https://bugs.webkit.org/attachment.cgi?id=196505&action=prettypatch
instead :)

[mthiesse, 2013-04-16 21:48:45]

On 2013/04/16 21:27:03, levin wrote:
> Consider https://bugs.webkit.org/attachment.cgi?id=196505&action=prettypatch
> instead :)

I don't think it actually matters, width is always calculated correctly even
when the size shrinks (html content layout is inherently width based as far as I
understand).

[mthiesse, 2013-04-16 21:48:51]

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

https://codereview.chromium.org/14139021/diff/1/Source/WebCore/page/FrameView...
Source/WebCore/page/FrameView.cpp:2445: resize(frameRect().width(),
m_minAutoSize.height());
On 2013/04/16 21:22:05, levin wrote:
> This stops "unconditionally start laying out from the minimum height, so that
> the
> documentRect can shrink below its previous height if needed."
> 
> Why does this line cause an assert?
> 
> And why won't it cause an assert the first time that autosize is run?

My thinking is we trigger the ASSERT because we cause a relayout when there's no
need to (duh). This is probably because the calculated new size ends up being
the size the page was before we shrunk it, so the layout code thinks nothing's
changed so there's no reason to re-layout?

I can't think of a nice way to fix this... we could force needsLayout to true
when we do this, but I don't know how we'd know this up front to prevent the
initial resize.

I assume this works accidentally on page load, because there is no previous
size, so a layout is always expected?

[mthiesse, 2013-04-16 22:12:02]

As discussed with levin@ we're reverting this patch for now (it should be looked
into and fixed correctly in the future).

[levin, 2013-04-16 22:15:47]

On 2013/04/16 22:12:02, mthiesse wrote:
> As discussed with levin@ we're reverting this patch for now (it should be
looked
> into and fixed correctly in the future).

Please fix the check in comment. (It is now just a revert of 
http://trac.webkit.org/changeset/147373)

Also file a bug to address the same issue as
http://trac.webkit.org/changeset/147373 ("Autosizing fails to compute the
correct height if the root element is very
small but the document has significant overflow." and perhaps reference that
change and this one.)

[levin, 2013-04-16 22:15:52]

lgtm

[mthiesse, 2013-04-16 22:31:37]

On 2013/04/16 22:15:52, levin wrote:
> lgtm

Issue updated and bug filed:
https://code.google.com/p/chromium/issues/detail?id=232132

Will CQ this when CQ is back online.

[commit-bot: I haz the power, 2013-04-18 15:12:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mthiesse@chromium.org/14139021/2003

[commit-bot: I haz the power, 2013-04-18 15:12:17]

Failed to apply patch for Source/WebCore/page/FrameView.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         Source/WebCore
  Created missing directory Source/WebCore.
  A         Source/WebCore/page
  Created missing directory Source/WebCore/page.
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/WebCore/page/FrameView.cpp
  |diff --git a/Source/WebCore/page/FrameView.cpp
b/Source/WebCore/page/FrameView.cpp
  |index
819f93f03c638c5e69b442135c30ee02cf6db162..9494aaa2ae7551cc0dea92d61cd01e12da55edc5
100644
  |--- a/Source/WebCore/page/FrameView.cpp
  |+++ b/Source/WebCore/page/FrameView.cpp
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       Source/WebCore/page/FrameView.cpp
Index: Source/WebCore/page/FrameView.cpp
diff --git a/Source/WebCore/page/FrameView.cpp
b/Source/WebCore/page/FrameView.cpp
index
819f93f03c638c5e69b442135c30ee02cf6db162..9494aaa2ae7551cc0dea92d61cd01e12da55edc5
100644
--- a/Source/WebCore/page/FrameView.cpp
+++ b/Source/WebCore/page/FrameView.cpp
@@ -2439,8 +2439,14 @@ void FrameView::autoSizeIfEnabled()
     if (!documentView || !documentElement)
         return;
 
-    // Start from the minimum height and allow it to grow.
-    resize(frameRect().width(), m_minAutoSize.height());
+    RenderBox* documentRenderBox = documentElement->renderBox();
+    if (!documentRenderBox)
+        return;
+
+    // If this is the first time we run autosize, start from small height and
+    // allow it to grow.
+    if (!m_didRunAutosize)
+        resize(frameRect().width(), m_minAutoSize.height());
 
     IntSize size = frameRect().size();
 
@@ -2450,7 +2456,7 @@ void FrameView::autoSizeIfEnabled()
         // Update various sizes including contentsSize, scrollHeight, etc.
         document->updateLayoutIgnorePendingStylesheets();
         int width = documentView->minPreferredLogicalWidth();
-        int height = documentView->documentRect().height();
+        int height = documentRenderBox->scrollHeight();
         IntSize newSize(width, height);
 
         // Check to see if a scrollbar is needed for a given dimension and

[commit-bot: I haz the power, 2013-04-18 15:13:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mthiesse@chromium.org/14139021/11001

[commit-bot: I haz the power, 2013-04-18 16:04:55]

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-04-18 16:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mthiesse@chromium.org/14139021/11001

[commit-bot: I haz the power, 2013-04-18 16:30:40]

Change committed as 148662