Promote inlines to first-class invalidation citizens

Promote inlines to first-class invalidation citizens

Have RenderInline's update and invalidate themselves like other descendants of
RenderLayerModelObject by pulling most of the invalidation logic down into
RenderLayerModelObject.

We end up doing slightly more work, but we simplify multiple codepaths, which I believe is superior.

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

[dsinclair, 2014-07-31 17:49:42]

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

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.cpp:200: // FIXME: SVG should
probably also go through this unified paint invalidation system.
Should we make a bug for this and track why SVG doesn't go through here?

[Julien - ping for review, 2014-08-04 19:36:39]

I really like the direction of this change.

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Pa...
File Source/core/rendering/PaintInvalidationState.cpp (left):

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.cpp:49: // FIXME: SVG could
probably benefit from a stack-based optimization like html does.
crbug.com/391054
We removed this comment, does that mean that the bug is solved?

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerModelObject.h (right):

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.h:69: virtual void
invalidateTreeIfNeeded(const PaintInvalidationState&) OVERRIDE;
Shouldn't this be private as we assume we call RenderObject's override?

[dsinclair, 2014-08-05 15:09:48]

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Pa...
File Source/core/rendering/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.cpp:68: m_paintOffset +=
toRenderInline(container)->offsetForInFlowPositionedInline(toRenderBox(renderer));
renderer is a RenderLayerModelObject, does this need a renderer->isBox() check
before we cast?

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

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1583: savePreviousBorderBoxSizeIfNeeded();
Should we keep the comment that this has to be after the invalidatePaintIfNeeded
call?

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

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.cpp:189: // If we are set to do a
full paint invalidation that means the RenderView will be
s/will be/will/

[leviw_travelin_and_unemployed, 2014-08-05 22:34:44]

PTAL.

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Pa...
File Source/core/rendering/PaintInvalidationState.cpp (left):

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.cpp:49: // FIXME: SVG could
probably benefit from a stack-based optimization like html does.
crbug.com/391054
No. I put it back.

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Pa...
File Source/core/rendering/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.cpp:68: m_paintOffset +=
toRenderInline(container)->offsetForInFlowPositionedInline(toRenderBox(renderer));
We're guaranteed that this is the case, but toRenderBox also has an assert. See
similar logic in RenderLayer.

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

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1583: savePreviousBorderBoxSizeIfNeeded();
Done.

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

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.cpp:189: // If we are set to do a
full paint invalidation that means the RenderView will be
Done.

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerModelObject.h (right):

https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.h:69: virtual void
invalidateTreeIfNeeded(const PaintInvalidationState&) OVERRIDE;
Done.

[leviw_travelin_and_unemployed, 2014-08-05 22:47:07]

>
https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayerModelObject.h (right):
> 
>
https://codereview.chromium.org/434453002/diff/90001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayerModelObject.h:69: virtual void
invalidateTreeIfNeeded(const PaintInvalidationState&) OVERRIDE;
> Shouldn't this be private as we assume we call RenderObject's override?

Actually, this has to be public :(

[dsinclair, 2014-08-06 15:03:27]

lgtm

[Julien - ping for review, 2014-08-06 18:14:29]

lgtm

[leviw_travelin_and_unemployed, 2014-08-06 18:24:44]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-08-06 18:26:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/434453002/190001

[commit-bot: I haz the power, 2014-08-06 20:14:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18684)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21403)

[leviw_travelin_and_unemployed, 2014-08-06 20:30:04]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-08-06 20:30:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/434453002/210001

[commit-bot: I haz the power, 2014-08-06 21:58:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[leviw_travelin_and_unemployed, 2014-08-06 22:09:02]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-08-06 22:09:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/434453002/230001

[leviw_travelin_and_unemployed, 2014-08-06 22:09:26]

Other huge rebaselines of the same tests are making landing this a nightmare :-/

[leviw_travelin_and_unemployed, 2014-08-06 22:25:07]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-08-06 22:25:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/434453002/250001

[commit-bot: I haz the power, 2014-08-06 23:54:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18724)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21449)

[commit-bot: I haz the power, 2014-08-06 23:55:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 23:55:57]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18735)

[Xianzhu, 2014-08-07 00:22:08]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-08-07 00:23:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/434453002/260001

[commit-bot: I haz the power, 2014-08-07 00:23:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-07 00:23:32]

Failed to apply patch for Source/core/rendering/PaintInvalidationState.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/PaintInvalidationState.cpp
  Hunk #1 FAILED at 41.
  Hunk #2 succeeded at 102 with fuzz 1.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/rendering/PaintInvalidationState.cpp.rej

Patch:       Source/core/rendering/PaintInvalidationState.cpp
Index: Source/core/rendering/PaintInvalidationState.cpp
diff --git a/Source/core/rendering/PaintInvalidationState.cpp
b/Source/core/rendering/PaintInvalidationState.cpp
index
b7db828df3b975936c8780d5479152f472cef29f..fc30a91e4c2a24386edbcb67265b132045aec33a
100644
--- a/Source/core/rendering/PaintInvalidationState.cpp
+++ b/Source/core/rendering/PaintInvalidationState.cpp
@@ -41,55 +41,14 @@ PaintInvalidationState::PaintInvalidationState(RenderObject&
renderer)
     }
 }
 
-PaintInvalidationState::PaintInvalidationState(const PaintInvalidationState&
next, RenderSVGModelObject& renderer, const RenderLayerModelObject&
paintInvalidationContainer)
-    : m_clipped(next.m_clipped)
-    , m_cachedOffsetsEnabled(next.m_cachedOffsetsEnabled)
-    , m_forceCheckForPaintInvalidation(next.m_forceCheckForPaintInvalidation)
-    , m_paintInvalidationContainer(paintInvalidationContainer)
-    , m_renderer(renderer)
-{
-    // FIXME: SVG could probably benefit from a stack-based optimization like
html does. crbug.com/391054
-    ASSERT(!m_cachedOffsetsEnabled);
-}
-
-PaintInvalidationState::PaintInvalidationState(const PaintInvalidationState&
next, RenderInline& renderer, const RenderLayerModelObject&
paintInvalidationContainer)
-    : m_clipped(false)
-    , m_cachedOffsetsEnabled(true)
-    , m_forceCheckForPaintInvalidation(next.m_forceCheckForPaintInvalidation)
-    , m_paintInvalidationContainer(paintInvalidationContainer)
-    , m_renderer(renderer)
-{
-    bool establishesPaintInvalidationContainer = &m_renderer ==
&m_paintInvalidationContainer;
-    if (establishesPaintInvalidationContainer) {
-        // When we hit a new paint invalidation container, we don't need to
-        // continue forcing a check for paint invalidation because movement
-        // from our parents will just move the whole invalidation container.
-        m_forceCheckForPaintInvalidation = false;
-    } else {
-        if (!renderer.supportsLayoutStateCachedOffsets() ||
!next.m_cachedOffsetsEnabled) {
-            m_cachedOffsetsEnabled = false;
-        } else if (m_cachedOffsetsEnabled) {
-            m_paintOffset = next.m_paintOffset;
-            // Handle relative positioned inline.
-            if (renderer.style()->hasInFlowPosition() && renderer.layer())
-                m_paintOffset += renderer.layer()->offsetForInFlowPosition();
-
-            // RenderInline can't be out-of-flow positioned.
-        }
-
-        // The following can't apply to RenderInline so we just propagate them.
-        m_clipped = next.m_clipped;
-        m_clipRect = next.m_clipRect;
-    }
-}
-
-PaintInvalidationState::PaintInvalidationState(const PaintInvalidationState&
next, RenderBox& renderer, const RenderLayerModelObject&
paintInvalidationContainer)
+PaintInvalidationState::PaintInvalidationState(const PaintInvalidationState&
next, RenderLayerModelObject& renderer, const RenderLayerModelObject&
paintInvalidationContainer)
     : m_clipped(false)
     , m_cachedOffsetsEnabled(true)
     , m_forceCheckForPaintInvalidation(next.m_forceCheckForPaintInvalidation)
     , m_paintInvalidationContainer(paintInvalidationContainer)
     , m_renderer(renderer)
 {
+    // FIXME: SVG could probably benefit from a stack-based optimization like
html does. crbug.com/391054
     bool establishesPaintInvalidationContainer = &m_renderer ==
&m_paintInvalidationContainer;
     bool fixed = m_renderer.isOutOfFlowPositioned() &&
m_renderer.style()->position() == FixedPosition;
 
@@ -102,7 +61,7 @@ PaintInvalidationState::PaintInvalidationState(const
PaintInvalidationState& nex
         if (!renderer.supportsLayoutStateCachedOffsets() ||
!next.m_cachedOffsetsEnabled) {
             m_cachedOffsetsEnabled = false;
         } else {
-            LayoutSize offset = m_renderer.isTableRow() ? LayoutSize() :
renderer.locationOffset();
+            LayoutSize offset = m_renderer.isBox() && !m_renderer.isTableRow()
? toRenderBox(renderer).locationOffset() : LayoutSize();
             if (fixed) {
                 // FIXME: This doesn't work correctly with transforms.
                 FloatPoint fixedOffset =
m_renderer.view()->localToAbsolute(FloatPoint(), IsFixed);
@@ -114,7 +73,7 @@ PaintInvalidationState::PaintInvalidationState(const
PaintInvalidationState& nex
             if (m_renderer.isOutOfFlowPositioned() && !fixed) {
                 if (RenderObject* container = m_renderer.container()) {
                     if (container->style()->hasInFlowPosition() &&
container->isRenderInline())
-                        m_paintOffset +=
toRenderInline(container)->offsetForInFlowPositionedInline(renderer);
+                        m_paintOffset +=
toRenderInline(container)->offsetForInFlowPositionedInline(toRenderBox(renderer));
                 }
             }

[leviw_travelin_and_unemployed, 2014-08-07 01:39:10]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-08-07 01:39:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/434453002/300001

[commit-bot: I haz the power, 2014-08-07 03:04:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-08-07 04:00:46]

Change committed as 179682