Layout overflow from transforms should update frame scrollbars

Source/core/frame/FrameView.cpp

Index: Source/core/frame/FrameView.cpp
diff --git a/Source/core/frame/FrameView.cpp b/Source/core/frame/FrameView.cpp
index c5f50c2f3f809096887acda647c2a68df2be0738..5464ce7b1f5e959cbb3979c7382223adc3b14310 100644
--- a/Source/core/frame/FrameView.cpp
+++ b/Source/core/frame/FrameView.cpp
@@ -638,8 +638,24 @@ void FrameView::recalcOverflowAfterStyleChange()
 
     renderView->recalcOverflowAfterStyleChange();
 
-    // FIXME: We should adjust frame scrollbar here, but that will make many
-    // tests flake in debug build.
+    if (needsLayout())
+        return;
+
+    InUpdateScrollbarsScope inUpdateScrollbarsScope(this);

[ojan, 2014/06/11 03:04:53]

Why do you need this here?

[trchen, 2014/06/11 05:04:13]

Without this, adjustViewSize() on line #657 will call setContentsSize() then
updateScrollbars() without protection, and FrameView::layout() will be called.
This will be caught by an assertion in Document::updateStyle() due to messed up
DocumentLifecycle.

I think it is okay to use InUpdateScrollbarsScope here, since we're essentially
doing a quick version of updateScrollbars().

[ojan, 2014/06/11 05:44:48]

Ah. That makes sense. I tried tracing the code to see if udpateScrollbars was
called by adjustViewSize and got lost.

+
+    bool hasHorizontalScrollbar = horizontalScrollbar();
+    bool hasVerticalScrollbar = verticalScrollbar();
+    bool shouldHaveHorizontalScrollbar = false;
+    bool shouldHaveVerticalScrollbar = false;
+    computeScrollbarExistence(shouldHaveHorizontalScrollbar, shouldHaveVerticalScrollbar);
+
+    if (hasHorizontalScrollbar != shouldHaveHorizontalScrollbar || hasVerticalScrollbar != shouldHaveVerticalScrollbar) {

[ojan, 2014/06/11 03:04:53]

Nit: Can you do the following?
if (horizontalScrollbar() != shouldHaveHorizontalScrollbar ||
verticalScrollbar() != shouldHaveVerticalScrollbar) {

It would make the code a bit more concise.

[trchen, 2014/06/11 05:04:13]

Can't compare between pointer and bool. Will do this instead:

if (static_cast<bool>(horizontalScrollbar()) != shouldHaveHorizontalScrollbar ||
static_cast<bool>(verticalScrollbar()) != shouldHaveVerticalScrollbar) {

[ojan, 2014/06/11 05:44:48]

Ugh. Actually, the local variable bools seems cleaner to me. Can you just move
them below the computeScrollbarExistence call since they're not needed for that?

[trchen, 2014/06/11 06:01:23]

Ah, that sounds even better. Will do.

+        setNeedsLayout();

[ojan, 2014/06/11 03:07:24]

Should this not also call the following?
    setHasHorizontalScrollbar(shouldHaveHorizontalScrollbar);
    setHasVerticalScrollbar(shouldHaveVerticalScrollbar);

[trchen, 2014/06/11 05:04:13]

adjustViewSize() will be called in post-layout phase, which will
setContentsSize() with the latest layout overflow and invoke full
updateScrollbars().

Adjusting scrollbar here may save us some time on the next updateScrollbars().
I'm worrying about unknown side effects, for example, newly revealed content
from deleting scrollbar might not get correctly invalidated.

+        return;
+    }
+
+    adjustViewSize();
+    updateScrollbarGeometry();
 }
 
 bool FrameView::usesCompositedScrolling() const