Separate repaint and layout requirements of StyleDifference (Step 4)

Separate repaint and layout requirements of StyleDifference (Step 4)

In this step RenderStyle::visualInvalidationDiff() sets both
needsFullLayout and needsRepaintObject if diffNeedsFullLayoutAndRepaint.
For now, all previous diffNeedsFullLayout cases become
diffNeedsFullLayoutRepaint.

In RenderObject, if both needsFullLayout and needsRepaint[Object] are
set, we set shouldDoFullRepaintAfterLayout(), and removed the condition
that needsSelfLayout always triggering shouldDoFullRepaintAfterLayout.

Previous RenderObject::setNeedsLayoutAndFullRepaint() still triggers
full repaint. Added back setNeedsLayout() to allow scheduling layout
without triggering full repaint.

With this change, we can do the following optimizations in next steps:

1. Repaint on RenderView resize (crbug.com/258219):
 - set FrameView::m_doFullRepaint only when needed;
 - use the flag to trigger full repaint of RenderView;
 - change renderView->setNeedsLayoutAndFullRepaint() in FrameView to
   renderView->setNeedsLayout() if no full repaint of RenderView is
   needed;

2. Repaint on style change needing full layout (e.g. crbug.com/339940):
 - move conditions not always needing repaint from
   RenderStyle::diffNeedsFullLayoutAndRepaint() into
   RenderStyle::diffNeedsFullLayout();
 - for above conditions that still may need repaint in some cases,
   correctly handle repaints of them during renderer layout.

3. Repaint on other RenderObject::setNeedsLayoutAndFullRepaint():
 - examine each caller of RenderObject::setNeedsLayoutAndFullRepaint()
   whether full repaint is actually always needed;
 - if not, change the call to RenderObject::setNeedsLayout() and
   correctly handle repaints during renderer layout if needed.

BUG=358460
TEST=existing layout tests

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

[Xianzhu, 2014-05-05 23:26:44]

ping...

[Xianzhu, 2014-05-06 21:33:35]

jchaffraix@ could you PTAL?

Also added leviw@ and dsinclair@ for reviewing the changed usage of
shouldDoFullRepaintAfterLayout flag.

[Julien - ping for review, 2014-05-07 00:24:31]

not lgtm, this change is confused and confusing. There are some good parts but I
think t it's trying to do too many of things some of which are just crazy.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1318: }
For me, this bit is total madness: we *don't* want each renderer marking to
trigger a full layout by default and I am surprised that the viewport doesn't
get invalidated all the time after this change.

Anything that does a several pass layout calls setNeedsLayout on its children.
You will force them to be invalidated, even if they don't move nor change size
:(

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/SVGRenderStyle.cpp (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/SVGRenderStyle.cpp:127:
styleDifference.setNeedsRepaintObject();
We didn't do that before, why do we want to do this change?

Furthermore, SVG has its own way of invalidating that doesn't really use this
flag AFAICT.

[Xianzhu, 2014-05-07 00:39:22]

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1318: }
On 2014/05/07 00:24:32, Julien Chaffraix - PST wrote:
> For me, this bit is total madness: we *don't* want each renderer marking to
> trigger a full layout by default and I am surprised that the viewport doesn't
> get invalidated all the time after this change.
> 
> Anything that does a several pass layout calls setNeedsLayout on its children.
> You will force them to be invalidated, even if they don't move nor change size
> :(

Could you give more details about how this CL changes the current behavior? I
thought we do always fully repaint on selfNeedsLayout and this change just moves
the setShouldDoFullRepaintAfterLayout() call from clearNeedsLayout() into
setNeedsLayout().

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/SVGRenderStyle.cpp (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/SVGRenderStyle.cpp:127:
styleDifference.setNeedsRepaintObject();
On 2014/05/07 00:24:32, Julien Chaffraix - PST wrote:
> We didn't do that before, why do we want to do this change?
> 
> Furthermore, SVG has its own way of invalidating that doesn't really use this
> flag AFAICT.

I'm a bit confused. Doesn't RenderObject::setStyle() and
RenderObject::shouldDoFullRepaintAfterLayout() apply to SVG objects?

[Xianzhu, 2014-05-07 01:17:30]

More explanations:

- I've run all layout tests and it only changes the expectations of the tests
included in the patch (actually reduces duplicated invalidations).

- RenderView doesn't always get fully repainted (with or without this change)
because its selfNeedsLayout flag is not always set when any descendant needs
layout. It may have some layout flag of RenderView, but normally not
selfNeedsLayout. However, direct renderView->setNeedsLayout() will set
selfNeedsLayout flag and will cause full repaint of the RenderView.

- Some confusion may come from the fact that currently setNeedsLayout() is not
symmetrical with needsLayout(). Perhaps we can rename setNeedsLayout() to
setNeedsFullLayout() to avoid the confusion.

[Xianzhu, 2014-05-08 00:51:50]

Julien, do you still think this CL is crazy? I believe it doesn't change the
current behavior which is already crazy, and it prepares to resolve the crazy
behavior.

BTW, https://codereview.chromium.org/275543003/ is 1st next step optimization
based on this CL. Perhaps it would help explain this one.

[Julien - ping for review, 2014-05-08 21:03:48]

> Julien, do you still think this CL is crazy? I believe it doesn't change the
current behavior which is already crazy, and it prepares to resolve the crazy
behavior.

I still think this change is crazy and confused. That's probably because we
think differently about what setNeedsRepaintObject means. If you outline *why*
you think the 2 snippets we are arguing over are conceptually correct, that
would help me understand what I am missing. I tried to present my arguments
about why I think they are wrong.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1318: }
On 2014/05/07 00:39:22, Xianzhu wrote:
> On 2014/05/07 00:24:32, Julien Chaffraix - PST wrote:
> > For me, this bit is total madness: we *don't* want each renderer marking to
> > trigger a full layout by default and I am surprised that the viewport
doesn't
> > get invalidated all the time after this change.
> > 
> > Anything that does a several pass layout calls setNeedsLayout on its
children.
> > You will force them to be invalidated, even if they don't move nor change
size
> > :(
> 
> Could you give more details about how this CL changes the current behavior? I
> thought we do always fully repaint on selfNeedsLayout and this change just
moves
> the setShouldDoFullRepaintAfterLayout() call from clearNeedsLayout() into
> setNeedsLayout().

We have 2 ways to generate invalidations: incremental (invalidate the difference
between the old and the new rectangles) and full (invalidate the old and new
rects). Incremental is the default and setShouldDoFullRepaintAfterLayout is used
to force a full invalidation.

A full invalidation should only be done when we know we can't do incremental
ones (like if the object moved, ...). This code is basically forcing multi pass
layout into a full invalidation, which was not the old behavior and is just
conceptually wrong.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/SVGRenderStyle.cpp (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/SVGRenderStyle.cpp:127:
styleDifference.setNeedsRepaintObject();
On 2014/05/07 00:39:22, Xianzhu wrote:
> On 2014/05/07 00:24:32, Julien Chaffraix - PST wrote:
> > We didn't do that before, why do we want to do this change?
> > 
> > Furthermore, SVG has its own way of invalidating that doesn't really use
this
> > flag AFAICT.
> 
> I'm a bit confused. Doesn't RenderObject::setStyle() and
> RenderObject::shouldDoFullRepaintAfterLayout() apply to SVG objects?

They both do. However this is forcing the object to repaint, just because it
needs layout seems like a conceptual mistake (layout doesn't imply calling
repaint() during style change as we can infer the old / new rectangles).

The only reason this is not generating more invalidation is because there is a
check for needsRepaintObject() && ! needsLayout() because calling repaint().

[Xianzhu, 2014-05-08 21:39:15]

On 2014/05/08 21:03:48, Julien Chaffraix - PST wrote:
> > Julien, do you still think this CL is crazy? I believe it doesn't change the
> current behavior which is already crazy, and it prepares to resolve the crazy
> behavior.
> 
> I still think this change is crazy and confused. That's probably because we
> think differently about what setNeedsRepaintObject means. If you outline *why*
> you think the 2 snippets we are arguing over are conceptually correct, that
> would help me understand what I am missing. I tried to present my arguments
> about why I think they are wrong.
> 

I think I know the source of the confusion: the asymmetrical setNeedsLayout()
(which sets selfNeedsLayout) and needsLayout() (which includes also other layout
types).

InvalidationIncremental is not for selfNeedsLayout(). selfNeedsLayout() always
causes full repaint.

I think just one question can make it clear: before my series of changes, how
did the following work?

  o.style.backgroundColor = 'blue';  // from 'white'
  o.style.height = '100px';  // from '80px'

There was only StyleDifferenceLayout before my change. If incremental
invalidation ever worked for selfNeedsLayout, how did the renderer know the
change of background color and not doing an under-repaint?

[Xianzhu, 2014-05-08 21:52:54]

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1318: }
On 2014/05/08 21:03:49, Julien Chaffraix - PST wrote:
> On 2014/05/07 00:39:22, Xianzhu wrote:
> > On 2014/05/07 00:24:32, Julien Chaffraix - PST wrote:
> > > For me, this bit is total madness: we *don't* want each renderer marking
to
> > > trigger a full layout by default and I am surprised that the viewport
> doesn't
> > > get invalidated all the time after this change.
> > > 
> > > Anything that does a several pass layout calls setNeedsLayout on its
> children.
> > > You will force them to be invalidated, even if they don't move nor change
> size
> > > :(
> > 
> > Could you give more details about how this CL changes the current behavior?
I
> > thought we do always fully repaint on selfNeedsLayout and this change just
> moves
> > the setShouldDoFullRepaintAfterLayout() call from clearNeedsLayout() into
> > setNeedsLayout().
> 
> We have 2 ways to generate invalidations: incremental (invalidate the
difference
> between the old and the new rectangles) and full (invalidate the old and new
> rects). Incremental is the default and setShouldDoFullRepaintAfterLayout is
used
> to force a full invalidation.
> 
> A full invalidation should only be done when we know we can't do incremental
> ones (like if the object moved, ...). This code is basically forcing multi
pass
> layout into a full invalidation, which was not the old behavior and is just
> conceptually wrong.

Incremental invalidation is only possible when there is no selfNeedsLayout().
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

The above change is equivalent with the original code, but just set
shouldDoFullRepaintAfterLayout eariler.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/SVGRenderStyle.cpp (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/SVGRenderStyle.cpp:127:
styleDifference.setNeedsRepaintObject();
On 2014/05/08 21:03:49, Julien Chaffraix - PST wrote:
> On 2014/05/07 00:39:22, Xianzhu wrote:
> > On 2014/05/07 00:24:32, Julien Chaffraix - PST wrote:
> > > We didn't do that before, why do we want to do this change?
> > > 
> > > Furthermore, SVG has its own way of invalidating that doesn't really use
> this
> > > flag AFAICT.
> > 
> > I'm a bit confused. Doesn't RenderObject::setStyle() and
> > RenderObject::shouldDoFullRepaintAfterLayout() apply to SVG objects?
> 
> They both do. However this is forcing the object to repaint, just because it
> needs layout seems like a conceptual mistake (layout doesn't imply calling
> repaint() during style change as we can infer the old / new rectangles).
> 
> The only reason this is not generating more invalidation is because there is a
> check for needsRepaintObject() && ! needsLayout() because calling repaint().

The code in RenderObject.cpp is changed to the following in this CL, no
!needsLayout() any more:

        if (m_parent && diff.needsRepaintObject()) {
            if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled() &&
diff.needsLayout())
                setShouldDoFullRepaintAfterLayout(true);
            else
                repaint();
        }

[Xianzhu, 2014-05-08 22:46:07]

On 2014/05/08 21:39:15, Xianzhu wrote:
> InvalidationIncremental is not for selfNeedsLayout(). selfNeedsLayout() always
> causes full repaint.
> 

References:
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

    InvalidationReason invalidationReason = wasSelfLayout ?
InvalidationSelfLayout : InvalidationIncremental;

[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

inline void RenderObject::clearNeedsLayout()
{
    if (!shouldDoFullRepaintAfterLayout())
        setShouldDoFullRepaintAfterLayout(selfNeedsLayout());
    ...
}

[Xianzhu, 2014-05-09 05:55:29]

Let me explain the change with graphics (RAL mode; non-RAL mode is different but
similar):

For style diff triggered layout
===============================

Before this change (before and after the previous changes in this series of
CLs):
  StyleDifferenceLayout
  1. (in RenderObject::setStyle etc) renderObject->setNeedsLayout
  2. (in RenderObject::clearNeedsLayout)
renderObject->setShouldDoFullRepaintAfterLayout(true)

After this change
  RenderStyle::diffNeedsFullLayoutAndRepaint (All diffs needing full layout is
temporarily explicitly marked needsRepaintObject)
  1. (in RenderObject::setStyle etc)
renderObject->setNeedsLayoutWithoutForcingRepaint
  2. (in RenderObject::setStyle etc)
renderObject->setShouldDoFullRepaintAfterLayout(true)

Next steps ('2' in this CL's description):
  A) For diffs needing full layout but not directly needing repaint:
RenderStyle::diffNeedsFullLayout
  1. (in RenderObject::setStyle etc)
renderObject->setNeedsLayoutWithoutForcingRepaint
  2. (in RenderObject::layout etc) incremental invalidation or no invalidation

  B) For diffs still needing both full layout and full repaint: still
RenderStyle::diffNeedsFullLayoutAndRepaint
  Same as "After this change" above


For FrameView triggered layout
==============================
 
Currently:
  A) If renderView->setNeedsLayout has been called, renderView will be fully
repainted (in the same steps as in "For style diff triggered layout - Before
this change")
  B) If renderView->setNeedsLayout has not been called, but renderView is
resized, renderView will be fully repainted (controlled by
FrameView::m_doFullRepaint)
  C) On other conditions triggering FrameView::m_doFullRepaint, renderView will
be fully repainted.

Next steps:
  See '1' in this CL's description, https://codereview.chromium.org/270663003/
and https://codereview.chromium.org/275543003/.


For layout triggered layout
===========================

Currently:
  A) If the renderObject's setNeedsLayout() has been called, it will be fully
repainted (in the same steps as in "For style diff triggered layout - Before
this change")
  B) If the renderObject is scheduled layout of types other than
selfNeedsLayout(), it may be not fully repainted. It may be incrementally
repainted, or not repainted.

Next steps:
  See '3' in this CL's description.

[Xianzhu, 2014-05-09 05:56:35]

On 2014/05/09 05:55:29, Xianzhu wrote:
> Let me explain the change with graphics

Sorry, not actually graphics, but lists of cases :)

[Julien - ping for review, 2014-05-12 15:08:46]

Xianzhu was kind enough to walk me through the change and removed a lot of the
confusion I had about this change.

After that, I think the general direction is OK but what confused me is that the
new code makes existing assumptions implicit opt-outs (force a full repaint by
default), when I would rather have them explicit opt-ins.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:2043: if
(RuntimeEnabledFeatures::repaintAfterLayoutEnabled() && needsLayout())
Ideally we would want any direct repaint calls to just disappear with
repaint-after-layout. This means that this should be a FIXME:

// FIXME: We will want to migrate all repaint() calls to repaint-after-layout,
probably by setting / propagating the shouldDoFullRepaintAfterLayout boolean.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:2084: if
(RuntimeEnabledFeatures::repaintAfterLayoutEnabled() && diff.needsLayout())
Same comment.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:636: void
setNeedsLayoutWithoutForcingFullRepaint(MarkingBehavior =
MarkContainingBlockChain, SubtreeLayoutScope* = 0);
This function is not used outside RenderObject so it could make sense to make it
private.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:650: void
setNeedsLayoutAndPrefWidthsRecalcWithoutForcingFullRepaint()
I think we should make this the default, not an opt-out.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1318: }
On 2014/05/08 21:52:55, Xianzhu wrote:
> On 2014/05/08 21:03:49, Julien Chaffraix - PST wrote:
> > On 2014/05/07 00:39:22, Xianzhu wrote:
> > > On 2014/05/07 00:24:32, Julien Chaffraix - PST wrote:
> > > > For me, this bit is total madness: we *don't* want each renderer marking
> to
> > > > trigger a full layout by default and I am surprised that the viewport
> > doesn't
> > > > get invalidated all the time after this change.
> > > > 
> > > > Anything that does a several pass layout calls setNeedsLayout on its
> > children.
> > > > You will force them to be invalidated, even if they don't move nor
change
> > size
> > > > :(
> > > 
> > > Could you give more details about how this CL changes the current
behavior?
> I
> > > thought we do always fully repaint on selfNeedsLayout and this change just
> > moves
> > > the setShouldDoFullRepaintAfterLayout() call from clearNeedsLayout() into
> > > setNeedsLayout().
> > 
> > We have 2 ways to generate invalidations: incremental (invalidate the
> difference
> > between the old and the new rectangles) and full (invalidate the old and new
> > rects). Incremental is the default and setShouldDoFullRepaintAfterLayout is
> used
> > to force a full invalidation.
> > 
> > A full invalidation should only be done when we know we can't do incremental
> > ones (like if the object moved, ...). This code is basically forcing multi
> pass
> > layout into a full invalidation, which was not the old behavior and is just
> > conceptually wrong.
> 
> Incremental invalidation is only possible when there is no selfNeedsLayout().
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> 
> The above change is equivalent with the original code, but just set
> shouldDoFullRepaintAfterLayout eariler.

OK, I see your point. It makes an assumption in the code very clear (that if we
set the layout flags, we assume that we are changing past the point of
incremental invalidation). This is unfortunate from my perspective that we keep
this bad assumption silent in setNeedsLayout: I would prefer if setNeedsLayout
would be renamed to setNeedsLayoutAndDoFullRepaint to make it obvious that we
are doing more than just layout. I expect this to be a significant refactoring
though so let's have a big FIXME there that the default shouldn't be silent.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:424: // - or the renderer knows how
to repaint caused by layout change.
I don't understand this sentence: when do we *not* know how to repaint after a
layout change?

[Xianzhu, 2014-05-12 17:58:04]

Thanks Julien for review! To avoid too big CL, I created a separate CL
(https://codereview.chromium.org/280633002/) to rename the current
setNeedsLayout and setNeedsLayoutAndPrefWidthsRecalc to be opting-in-repaint
style. This CL will base on it.

[Xianzhu, 2014-05-12 21:31:50]

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:2043: if
(RuntimeEnabledFeatures::repaintAfterLayoutEnabled() && needsLayout())
On 2014/05/12 15:08:46, Julien Chaffraix - CET wrote:
> Ideally we would want any direct repaint calls to just disappear with
> repaint-after-layout. This means that this should be a FIXME:
> 
> // FIXME: We will want to migrate all repaint() calls to repaint-after-layout,
> probably by setting / propagating the shouldDoFullRepaintAfterLayout boolean.

Don't we still need repaint() on repaint-only (no layout) style change?

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:636: void
setNeedsLayoutWithoutForcingFullRepaint(MarkingBehavior =
MarkContainingBlockChain, SubtreeLayoutScope* = 0);
On 2014/05/12 15:08:46, Julien Chaffraix - CET wrote:
> This function is not used outside RenderObject so it could make sense to make
it
> private.

We will need this function (in new opting-in-repaint code, it's just
setNeedsLayout()) from outside RenderObject in the following steps.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:650: void
setNeedsLayoutAndPrefWidthsRecalcWithoutForcingFullRepaint()
On 2014/05/12 15:08:46, Julien Chaffraix - CET wrote:
> I think we should make this the default, not an opt-out.

Done.

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/265703012/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:424: // - or the renderer knows how
to repaint caused by layout change.
On 2014/05/12 15:08:46, Julien Chaffraix - CET wrote:
> I don't understand this sentence: when do we *not* know how to repaint after a
> layout change?

I mean the renderer knows how to repaint optimally other than the forced full
repaint. Updated the comments.

[Xianzhu, 2014-05-19 16:11:23]

jchaffraix@ are you back? Could you PTAL?

[Julien - ping for review, 2014-05-20 16:33:29]

lgtm

https://chromiumcodereview.appspot.com/265703012/diff/1/Source/core/rendering...
File Source/core/rendering/RenderObject.cpp (right):

https://chromiumcodereview.appspot.com/265703012/diff/1/Source/core/rendering...
Source/core/rendering/RenderObject.cpp:2043: if
(RuntimeEnabledFeatures::repaintAfterLayoutEnabled() && needsLayout())
On 2014/05/12 21:31:51, Xianzhu wrote:
> On 2014/05/12 15:08:46, Julien Chaffraix - CET wrote:
> > Ideally we would want any direct repaint calls to just disappear with
> > repaint-after-layout. This means that this should be a FIXME:
> > 
> > // FIXME: We will want to migrate all repaint() calls to
repaint-after-layout,
> > probably by setting / propagating the shouldDoFullRepaintAfterLayout
boolean.
> 
> Don't we still need repaint() on repaint-only (no layout) style change?

We do indeed. However we could make those use repaint-after-layout (which we
should rename at that point). This is a mid-term goal.

https://chromiumcodereview.appspot.com/265703012/diff/60001/Source/core/rende...
File Source/core/rendering/RenderObject.h (right):

https://chromiumcodereview.appspot.com/265703012/diff/60001/Source/core/rende...
Source/core/rendering/RenderObject.h:1350:
setShouldDoFullRepaintAfterLayout(true);
Shouldn't this line check shouldDoRepaintAfterLayout like clearNeedsLayout used
to?

[Xianzhu, 2014-05-20 16:47:08]

https://chromiumcodereview.appspot.com/265703012/diff/60001/Source/core/rende...
File Source/core/rendering/RenderObject.h (right):

https://chromiumcodereview.appspot.com/265703012/diff/60001/Source/core/rende...
Source/core/rendering/RenderObject.h:1350:
setShouldDoFullRepaintAfterLayout(true);
On 2014/05/20 16:33:30, Julien Chaffraix - CET wrote:
> Shouldn't this line check shouldDoRepaintAfterLayout like clearNeedsLayout
used
> to?

I think the original check avoided calling setNeedsLayout() if
shouldDoFullRepaintAfterLayout() has already been set, but in the above code
directly setting it seems to cost almost the same as checking it first.

[Xianzhu, 2014-05-20 16:59:26]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-05-20 16:59:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/265703012/80001

[commit-bot: I haz the power, 2014-05-20 18:24:33]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9011)

[commit-bot: I haz the power, 2014-05-20 19:59:07]

Change committed as 174401