Layout overflow from transforms should update frame scrollbars

Source/core/frame/FrameView.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Dirk Mueller <mueller@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *           (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
@@ skipping 620 matching lines @@
 
 void FrameView::recalcOverflowAfterStyleChange()
 {
     RenderView* renderView = this->renderView();
     ASSERT(renderView);
     if (!renderView->needsOverflowRecalcAfterStyleChange())
         return;
 
     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
 {
     RenderView* renderView = this->renderView();
     if (!renderView)
         return false;
     if (m_frame->settings() && m_frame->settings()->compositedScrollingForFramesEnabled())
         return renderView->compositor()->inCompositingMode();
     return false;
@@ skipping 2620 matching lines @@
 void FrameView::willRemoveScrollbar(Scrollbar* scrollbar, ScrollbarOrientation orientation)
 {
     ScrollableArea::willRemoveScrollbar(scrollbar, orientation);
     if (AXObjectCache* cache = axObjectCache()) {
         cache->remove(scrollbar);
         cache->handleScrollbarUpdate(this);
     }
 }
 
 } // namespace WebCore