Reland "Avoid layout/full-repaint on view height change if possible"

Reland "Avoid layout/full-repaint on view height change if possible"

Original CL is https://codereview.chromium.org/99663004

Previously we always layout/full-repaint of the whole view when the height
of the RenderView changes. In many cases this is unnecessary. Especially,
this causes unnecessary cost on Android when the top control shows or
hides.

Avoid repaint if the RenderView doesn't needs full layout.

The original CL was reverted because of issues caused. This CL has the
following changes compared to the original CL:

- Call ChromeClient::layoutUpdated() after the simplified layout. This
  is needed at least for WebViewImpl to update the page scale factor
  when the screen rotates on Android;

- Combined the simplified layout code in RenderView::viewResized() and
  RenderView::needsLayoutOnLogicalHeightChange() into
  Renderview::trySimplifiedLayoutOnHeightChange() because the code are
  highly dependent (e.g. the new overflow code);

- Handle overflow changes (bug 342185):
  - During a normal layout, overflow may be created or destroyed based
    on the relationship between the contents size and view size. We need
    to mimic the logic in simplified layout path;
  - Fallback to full layout if there is no vertical
    render overflow (page is not vertically scrollable) and the view
    height is shrinking, because without a full layout we can't know the
    new contents size. For Android this case rarely happen because we
    don't auto-hide the url bar if the page is not vertically scrollable.

BUG=258219, 327815, 342185
TEST=WebFrameTest.heightChangeRepaint, WebFrameTest.heightChangeNoFullRepaintOverflowChange
TEST=all layout tests still pass

[esprehn, 2014-03-10 21:32:29]

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

https://codereview.chromium.org/190973007/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:3193: // to avoid hang with resize events in
seamless frames.
We don't even have seamless anymore. This code seems suspect.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2712:
cb->addPercentHeightDescendant(const_cast<RenderBox*>(this));
Same, why is it okay to skip adding percentage height descendants inside the
loop now?

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2866: if (cb->isRenderBlock())
This skips adding anonymous percentage height descendants, why is that correct?

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1014: if (document().inQuirksMode() ||
document().paginated() || shouldUsePrintingLayout())
Why not in quirks?

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1030: if
(document().ensureStyleResolver().mediaQueryAffectedByViewportChange() ||
document().hasViewportUnits())
Doesn't this exclude all responsive websites?

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1035: if (!body ||
!body->hasTagName(HTMLNames::bodyTag))
Why is frameset special?

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1041: ||
documentElement->renderer()->rendererForRootBackground()->style()->hasBackgroundImage())
You can't do this, the style might be dirty and you didn't update it before
asking questions about it.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1054: if (childStyle->top().isPercent() ||
childStyle->height().isPercent() || childStyle->minHeight().isPercent() ||
childStyle->maxHeight().isPercent() || !childStyle->bottom().isAuto())
Same, you never updated style so this is all stale

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGRoot.cpp:179:
cb->addPercentHeightDescendant(const_cast<RenderSVGRoot*>(this));
ditto

[Xianzhu, 2014-03-11 00:55:13]

Thanks for review!

PTAL.

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

https://codereview.chromium.org/190973007/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:3193: // to avoid hang with resize events in
seamless frames.
On 2014/03/10 21:32:30, esprehn wrote:
> We don't even have seamless anymore. This code seems suspect.

Good news. Removed the comment.

Changed the code and added the comments after I encountered failure of a layout
test containing <iframe seamless> when I was preparing the first version of the
patch.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2712:
cb->addPercentHeightDescendant(const_cast<RenderBox*>(this));
On 2014/03/10 21:32:30, esprehn wrote:
> Same, why is it okay to skip adding percentage height descendants inside the
> loop now?

In existing code, percentHeightDescendants is used to force layout of an object
whose height is percentage height related to an ancestor. The intermediate
containing blocks are skipped because they don't contribute to the calculation
of the object's height, so it's unnecessary to add the object into the
intermediate containing blocks percentHeightDescendants, except the last cb.

However, because the addPercentHeightDescendant call is in the loop, it is only
called if there is any containing block skipped. That is, it is called for each
percent height grand-child or lower descendant child, but not for direct child.
This was intentional when the change was made because layout of direct children
doesn't depend on the percentHeightDescendant map.  For my change, a block needs
to know all of its descendants (including direct children) whose layout may be
affected by the height of the block, so I moved an addPercentHeightDescendent()
call after the loop.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2866: if (cb->isRenderBlock())
On 2014/03/10 21:32:30, esprehn wrote:
> This skips adding anonymous percentage height descendants, why is that
correct?

Actually this skips adding this object into percentHeightDescendants of
anonymous ancestors. By moving the call after the loop, this object is added
into the ancestor who really affects the height of this object.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1014: if (document().inQuirksMode() ||
document().paginated() || shouldUsePrintingLayout())
On 2014/03/10 21:32:30, esprehn wrote:
> Why not in quirks?

Added comments.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1030: if
(document().ensureStyleResolver().mediaQueryAffectedByViewportChange() ||
document().hasViewportUnits())
On 2014/03/10 21:32:30, esprehn wrote:
> Doesn't this exclude all responsive websites?

Optimized viewport media query and viewport sizes in the new Patch Set by
calling performPreLayoutTasks() from FrameView::setLayoutSizeInternal() before
calling this method. If viewport media query causes style changes, or there are
any viewport sizes, fallback to a full layout.

If media query doesn't change style, go through this path.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1035: if (!body ||
!body->hasTagName(HTMLNames::bodyTag))
On 2014/03/10 21:32:30, esprehn wrote:
> Why is frameset special?

Added comment.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1041: ||
documentElement->renderer()->rendererForRootBackground()->style()->hasBackgroundImage())
On 2014/03/10 21:32:30, esprehn wrote:
> You can't do this, the style might be dirty and you didn't update it before
> asking questions about it.

Now update style in FrameView::setLayoutSizeInternal() (by calling
performPreLayoutTasks()) before calling this function.

https://codereview.chromium.org/190973007/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1054: if (childStyle->top().isPercent() ||
childStyle->height().isPercent() || childStyle->minHeight().isPercent() ||
childStyle->maxHeight().isPercent() || !childStyle->bottom().isAuto())
On 2014/03/10 21:32:30, esprehn wrote:
> Same, you never updated style so this is all stale

Ditto.

[Julien - ping for review, 2014-03-11 01:23:36]

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

https://codereview.chromium.org/190973007/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:3186: ScrollView::contentsResized();
Wouldn't it be possible to remove FrameView::contentsResized? All FrameView
callers want to avoid the setNeedsLayout call. The other calls are in ScrollView
when we update the scrollbars so it's possible the SetNeedsLayout call is needed
in this case.

https://codereview.chromium.org/190973007/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:3193: if (RenderView* renderView =
this->renderView()) {
This would be better with an early return, especially since it seems like a
corner case (if you don't have a render tree, you don't have anything to do
anyway).

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2712:
cb->addPercentHeightDescendant(const_cast<RenderBox*>(this));
Wouldn't this mean that any containing block between the percentage height
descendant and |cb| would return that it has no percentage height descendant
even if it does?

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1027: document().updateStyleIfNeeded();
Isn't this line contrary with the ASSERT above about not needed style recalc?

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1071: addLayoutOverflow(layoutOverflow);
It's weird that you don't also add visual overflow ie just call
computeOverflow(). Is it intentional?

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1074:
layer()->updateLayerPositionsAfterLayout(layer(), RenderLayer::defaultFlags);
updateLayerPositions will recurse into all the layers and could cause some extra
repaints. Wouldn't calling RenderLayer::updateLayerPosition() do what you want?

[Xianzhu, 2014-03-11 19:05:13]

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

https://codereview.chromium.org/190973007/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:3186: ScrollView::contentsResized();
On 2014/03/11 01:23:37, Julien Chaffraix - PST wrote:
> Wouldn't it be possible to remove FrameView::contentsResized? All FrameView
> callers want to avoid the setNeedsLayout call. The other calls are in
ScrollView
> when we update the scrollbars so it's possible the SetNeedsLayout call is
needed
> in this case.

Agreed. Need to carefully check all callsites of it and test, so I'd like to do
it after this change.

https://codereview.chromium.org/190973007/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:3193: if (RenderView* renderView =
this->renderView()) {
On 2014/03/11 01:23:37, Julien Chaffraix - PST wrote:
> This would be better with an early return, especially since it seems like a
> corner case (if you don't have a render tree, you don't have anything to do
> anyway).

Done.

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2712:
cb->addPercentHeightDescendant(const_cast<RenderBox*>(this));
On 2014/03/11 01:23:37, Julien Chaffraix - PST wrote:
> Wouldn't this mean that any containing block between the percentage height
> descendant and |cb| would return that it has no percentage height descendant
> even if it does?

I think this depends on how we define "percentage height descendants" of an
object. If we mean it "descendants whose percent height calculation directly
depend on this object's size", I think the behavior is just what we want.

Even the original code didn't mean it "all descendants with percent height",
otherwise the loop should not just stop at
!skipContaingBlockForPercentHeightCalculation(), but needs to loop until it
reaches the root renderer.

How about renaming "percentHeightDescendant" to "percentHeightDependent"?

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1027: document().updateStyleIfNeeded();
On 2014/03/11 01:23:37, Julien Chaffraix - PST wrote:
> Isn't this line contrary with the ASSERT above about not needed style recalc?

Sorry. I added the code and later meant to move the code to FrameView but forgot
to remove this.

Removed.

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1059: return false;
Removed the above because fixed position layout has already been handled in
FrameView::setFrameRect().

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1071: addLayoutOverflow(layoutOverflow);
On 2014/03/11 01:23:37, Julien Chaffraix - PST wrote:
> It's weird that you don't also add visual overflow ie just call
> computeOverflow(). Is it intentional?

Actually I didn't notice it, so for cases needing recompute overflow, I just let
it fallback to full layout (line 1020-1024).

Removed the code and let it go computeOverflow() (yay another optimized case!).
For layout-overflow-not-changed case, still use 1070-1071 because it is more
lightweight (it will also update visual overflow by the way).

https://codereview.chromium.org/190973007/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1074:
layer()->updateLayerPositionsAfterLayout(layer(), RenderLayer::defaultFlags);
On 2014/03/11 01:23:37, Julien Chaffraix - PST wrote:
> updateLayerPositions will recurse into all the layers and could cause some
extra
> repaints. Wouldn't calling RenderLayer::updateLayerPosition() do what you
want?

The repaints caused by layer resizing and exposing are expected.
RenderLayer::updateLayerPosition() won't do that.

[Xianzhu, 2014-03-14 16:33:06]

More comments?

[eseidel, 2014-03-15 00:53:46]

This seems really hard to get right, another "fast-path".  The problem with
doing a full layout is that it causes too much re-raster?  layouts themselves
should be very cheap...

[esprehn, 2014-03-15 02:19:41]

I'm not very comfortable with this fast path, especially not when you're doing
things like adding special cases for <dialog>.

https://codereview.chromium.org/190973007/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/190973007/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1006: ASSERT(!document().needsStyleRecalc()
&& !document().childNeedsStyleRecalc());
This is not correct, you need to check !document().hasPendingStyleRecalc()

https://codereview.chromium.org/190973007/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1042: if ((*it)->node() &&
(*it)->node()->hasTagName(HTMLNames::dialogTag))
Special casing dialog is wrong, why are you doing that?

https://codereview.chromium.org/190973007/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1046: if (childStyle->top().isPercent() ||
childStyle->height().isPercent() || childStyle->minHeight().isPercent() ||
childStyle->maxHeight().isPercent() || !childStyle->bottom().isAuto())
This should be a method, the check is really hard to understand.

[Xianzhu, 2014-03-17 18:58:18]

On 2014/03/15 00:53:46, eseidel wrote:
> This seems really hard to get right, another "fast-path".  The problem with
> doing a full layout is that it causes too much re-raster?  layouts themselves
> should be very cheap...

Yes, the main purpose is to avoid re-raster by avoiding full layout which for
now always causes full re-raster.

I'm thinking of alternative ways. New "fast-path" doesn't looks good.

[eseidel, 2014-03-17 22:23:14]

I think we should just integrate this code into normal layout.  Mark a block as
needing height-layout, and then when in that mode, not bother to decend into
kids if not needed.

Maybe NeedsHeightOnlyLayout needs to be its own separate flag:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...