Separate repaint and layout requirements of StyleDifference (Step 3)

Separate repaint and layout requirements of StyleDifference (Step 3)

Step 1 and 2 replaced StyleDifference enum with StyleDifference class
but still didn't change any functionality.

This change allows multiple types of flags set in StyleDifference, in
the following cases:

1. Both needsRepaint and needsPositionedMovement: This was a full layout
   when StyleDifference enum couldn't express the exact needs of the
   case. Now we can do a repaint and positioned movement without full
   layout.

2. Both needsRepaintLayer and needsFullLayout: Previously, the needs of
   repainting layer must be checked again (about clip, transform,
   opacity etc.) in RenderLayerModelObject::styleWillChange() if layout
   is also needed. Now we can go through the normal needsRepaintLayer
   path.

3. Both needsRepaint and needsFullLayout: Previously as the repaint
   needs couldn't be explicitly expressed, we have to do a full repaint.
   In this change we still keep the original behavior unchanged.
   Will optimize in later changes.

4. Both contextSensitiveProperties and needsLayout: Now this is
   resolved by RenderObject::adjustStyleDifference() to layout only
   or case 1, 2 or 3.

Only case 2 changes the visible behavior. All rebaselined layout tests
are in this case. The extra repaint rects are because now the code
schedules both direct repaints and repaints caused by layer position
change.

BUG=358460
TEST=existing layout tests, several rebaselined.

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

[Xianzhu, 2014-04-24 18:08:27]

Hi, reviewers,
Step 1 and Step 2 have been landed. Could you please take a look at this one?
Thanks.

[Julien - ping for review, 2014-04-25 15:43:04]

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerModelObject.cpp (left):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.cpp:122: || oldStyle->filter() !=
newStyle.filter())
Could you explain why it is correct to remove these lines?

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1904: diff.setNeedsRepaintLayer();
Is this line always needed? Even when we trigger a full layout above?

If so, I would like a comment to be added as to why we need it.

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2043: // FIXME: For now if
needsFullLayout() or needsSimplifiedLayout(), repaint is handled during
layout(),
What's the long term plan?

My view of the code is that we will keep invalidating during layout as it's the
right things to do and we definitely will need to a way to force an invalidation
from time to time regardless (e.g. a background-color change + a box size change
that ends up with the same box size due to other constrains)

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGResourcesCache.cpp (right):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGResourcesCache.cpp:129: if
(renderer->isSVGResourceFilterPrimitive() && !diff.needsLayout())
This change is a no-op (SVG elements don't have layers and they don't use the
optimized layout bits) but it obfuscates what the code really does.

[Xianzhu, 2014-04-25 17:32:39]

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerModelObject.cpp (left):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.cpp:122: || oldStyle->filter() !=
newStyle.filter())
On 2014/04/25 15:43:04, Julien Chaffraix - PST wrote:
> Could you explain why it is correct to remove these lines?

zIndex, hasAutoZIndex, opacity, transform, filter changes will set
contextSensitiveProperties, and RenderObject::adjustStyleDifference() will
convert them into needsRepaintLayer or needsRecompositeLayer according to the
context.

However, previously, the above logic was not possible if the style diff already
needs full layout, so the above code was needed to recover the needsRepaintLayer
requirement. Actually this was duplicate with the code in
RenderObject::adjustStyleDifference().

Now StyleDifference can express needsRepaintLayer and needsLayout at the same
time, so the above code is no longer needed.

This is the case 2 in the CL description.

Position is still specially handled as before.

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1904: diff.setNeedsRepaintLayer();
On 2014/04/25 15:43:04, Julien Chaffraix - PST wrote:
> Is this line always needed? Even when we trigger a full layout above?
> 
> If so, I would like a comment to be added as to why we need it.

The above change will disappear if rebased on trchen@'s
https://codereview.chromium.org/250773002/ (diff.setNeedsRepaintLayer() is the
only thing left).

For now this is related to the change in RenderLayerModelObject. I removed the
original code that recovered needsRepaintLayer from needsLayout on transform
change, so we need to set needsRepaintLayer separately.

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2043: // FIXME: For now if
needsFullLayout() or needsSimplifiedLayout(), repaint is handled during
layout(),
On 2014/04/25 15:43:04, Julien Chaffraix - PST wrote:
> What's the long term plan?
> 
> My view of the code is that we will keep invalidating during layout as it's
the
> right things to do and we definitely will need to a way to force an
invalidation
> from time to time regardless (e.g. a background-color change + a box size
change
> that ends up with the same box size due to other constrains)

Yes.

In this CL, repaint-only needs are always set (e.g. background-color change),
but the changed condition below prevents duplicated repaint rects otherwise we
need to rebaseline many layout tests.

For layout-caused repaint, I haven't explored in depth. May have the following
choices:
a) let RenderStyle::visualInvalidationDiff() set all repaint needs that may be
caused by layout in advance;
b) let RenderObject::layout() only repaint if needed;
c) mix a) and b):
   - RenderStyle sets explicit repaint needs (e.g change of border-width will
set both needsFullLayout and needsRepaintObject)
   - For geometry-only changes, RenderStyle sets needsFullLayout only, and
RenderObject repaints if needed.

c) seems the most feasible way.

wdyt?

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGResourcesCache.cpp (right):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGResourcesCache.cpp:129: if
(renderer->isSVGResourceFilterPrimitive() && !diff.needsLayout())
On 2014/04/25 15:43:04, Julien Chaffraix - PST wrote:
> This change is a no-op (SVG elements don't have layers and they don't use the
> optimized layout bits) but it obfuscates what the code really does.

StyleDifference now allows needsFullLayout() and needsRepaintObject() to be set
independently, and SVGRenderStyle::diff() may set both of them in the next steps
to express repaint needs explicitly even if needsFullLayout.

I think the code is like "if (!needsRepaint()) return; else
doSthNeededWhenNeedsLayout;", so using !diff.needsLayout() here seems clearer.

[Julien - ping for review, 2014-04-28 17:40:27]

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1904: diff.setNeedsRepaintLayer();
On 2014/04/25 17:32:39, Xianzhu wrote:
> On 2014/04/25 15:43:04, Julien Chaffraix - PST wrote:
> > Is this line always needed? Even when we trigger a full layout above?
> > 
> > If so, I would like a comment to be added as to why we need it.
> 
> The above change will disappear if rebased on trchen@'s
> https://codereview.chromium.org/250773002/ (diff.setNeedsRepaintLayer() is the
> only thing left).
> 
> For now this is related to the change in RenderLayerModelObject. I removed the
> original code that recovered needsRepaintLayer from needsLayout on transform
> change, so we need to set needsRepaintLayer separately.

I am still requesting a code comment or waiting on trchen's change so that this
hack disappears.

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2043: // FIXME: For now if
needsFullLayout() or needsSimplifiedLayout(), repaint is handled during
layout(),
On 2014/04/25 17:32:39, Xianzhu wrote:
> On 2014/04/25 15:43:04, Julien Chaffraix - PST wrote:
> > What's the long term plan?
> > 
> > My view of the code is that we will keep invalidating during layout as it's
> the
> > right things to do and we definitely will need to a way to force an
> invalidation
> > from time to time regardless (e.g. a background-color change + a box size
> change
> > that ends up with the same box size due to other constrains)
> 
> Yes.
> 
> In this CL, repaint-only needs are always set (e.g. background-color change),
> but the changed condition below prevents duplicated repaint rects otherwise we
> need to rebaseline many layout tests.
> 
> For layout-caused repaint, I haven't explored in depth. May have the following
> choices:
> a) let RenderStyle::visualInvalidationDiff() set all repaint needs that may be
> caused by layout in advance;
> b) let RenderObject::layout() only repaint if needed;
> c) mix a) and b):
>    - RenderStyle sets explicit repaint needs (e.g change of border-width will
> set both needsFullLayout and needsRepaintObject)
>    - For geometry-only changes, RenderStyle sets needsFullLayout only, and
> RenderObject repaints if needed.

border-width is a bad example as we already keep track of it in our border-box
so layout can detect these changes. A better example is transform where we have
lost all the required information by layout time and can't infer it anymore.
That means we have no other option than c) in the foreseeable future.

> c) seems the most feasible way.
> 
> wdyt?

That's what I had in mind. However the new comment in the code is confusing and
should be improved.

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGResourcesCache.cpp (right):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGResourcesCache.cpp:129: if
(renderer->isSVGResourceFilterPrimitive() && !diff.needsLayout())
On 2014/04/25 17:32:39, Xianzhu wrote:
> On 2014/04/25 15:43:04, Julien Chaffraix - PST wrote:
> > This change is a no-op (SVG elements don't have layers and they don't use
the
> > optimized layout bits) but it obfuscates what the code really does.
> 
> StyleDifference now allows needsFullLayout() and needsRepaintObject() to be
set
> independently, and SVGRenderStyle::diff() may set both of them in the next
steps
> to express repaint needs explicitly even if needsFullLayout.
> 
> I think the code is like "if (!needsRepaint()) return; else
> doSthNeededWhenNeedsLayout;", so using !diff.needsLayout() here seems clearer.

OK

[Xianzhu, 2014-04-28 18:11:50]

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1904: diff.setNeedsRepaintLayer();
On 2014/04/28 17:40:28, Julien Chaffraix - PST wrote:
> On 2014/04/25 17:32:39, Xianzhu wrote:
> > On 2014/04/25 15:43:04, Julien Chaffraix - PST wrote:
> > > Is this line always needed? Even when we trigger a full layout above?
> > > 
> > > If so, I would like a comment to be added as to why we need it.
> > 
> > The above change will disappear if rebased on trchen@'s
> > https://codereview.chromium.org/250773002/ (diff.setNeedsRepaintLayer() is
the
> > only thing left).
> > 
> > For now this is related to the change in RenderLayerModelObject. I removed
the
> > original code that recovered needsRepaintLayer from needsLayout on transform
> > change, so we need to set needsRepaintLayer separately.
> 
> I am still requesting a code comment or waiting on trchen's change so that
this
> hack disappears.

Done.

https://codereview.chromium.org/247713003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2043: // FIXME: For now if
needsFullLayout() or needsSimplifiedLayout(), repaint is handled during
layout(),
How about the updated comments?

[Xianzhu, 2014-04-29 16:29:31]

PTAL.

Rebased on https://codereview.chromium.org/250773002/.

The CL is simplified because SimplifiedLayout was removed in the above change.

[Julien - ping for review, 2014-04-30 18:02:25]

lgtm

https://codereview.chromium.org/247713003/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/fixed-child-move-after-scroll-expected.txt
(right):

https://codereview.chromium.org/247713003/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/fixed-child-move-after-scroll-expected.txt:5: (rect 10
70 100 100)
It's annoying that we now generate duplicated invalidations. I think this should
be filed and tracked by a bug as we should try to generate only the minimum
subset of invalidations.

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

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2031: // Revisit the condition when we
optimize layout not to always repaint.
I don't think this comment makes sense per our discussion. If we have to force
an invalidation here, we should allow it and not care about layout generating
invalidations too.

Also a layout invalidation doesn't guarantee a FULL invalidation which will be
wrong in some cases.

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:386:
diff.setNeedsPositionedMovementLayout();
setNeedsPositionedMovementLayout checks if we need a full layout but it's
impossible as we check it above. I think we just have an ASSERT into
setNeedsPositionedMovementLayout() as we don't expect people to do this extra
work to not set it.

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/StyleDifference.h (right):

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/StyleDifference.h:16: // Only directly needed
actions are included. Indirect needed actions, such as
I don't think this comment helps, especially since we don't intent to include
the "indirect" actions, which are just consequences of the StyleDifference
anyway.

[Xianzhu, 2014-04-30 18:40:15]

Will upload the new Patch Set after RAL NeedsRebaseline layout tests finish
auto-rebaselining.

https://codereview.chromium.org/247713003/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/fixed-child-move-after-scroll-expected.txt
(right):

https://codereview.chromium.org/247713003/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/fixed-child-move-after-scroll-expected.txt:5: (rect 10
70 100 100)
On 2014/04/30 18:02:26, Julien Chaffraix - PST wrote:
> It's annoying that we now generate duplicated invalidations. I think this
should
> be filed and tracked by a bug as we should try to generate only the minimum
> subset of invalidations.

Will mention this in crbug.com/324672.

However, the repaints will be different with RAL. Waiting for all
auto-rebaselines RAL, then recheck the expectations.

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

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2031: // Revisit the condition when we
optimize layout not to always repaint.
On 2014/04/30 18:02:26, Julien Chaffraix - PST wrote:
> I don't think this comment makes sense per our discussion. If we have to force
> an invalidation here, we should allow it and not care about layout generating
> invalidations too.
> 
> Also a layout invalidation doesn't guarantee a FULL invalidation which will be
> wrong in some cases.

Updated the comments to mention this is temporary to keep the original
StyleDifference behavior and later we should force repaint here if
diff.needsRepaint().

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:386:
diff.setNeedsPositionedMovementLayout();
On 2014/04/30 18:02:26, Julien Chaffraix - PST wrote:
> setNeedsPositionedMovementLayout checks if we need a full layout but it's
> impossible as we check it above. I think we just have an ASSERT into
> setNeedsPositionedMovementLayout() as we don't expect people to do this extra
> work to not set it.

Done.

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/StyleDifference.h (right):

https://codereview.chromium.org/247713003/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/StyleDifference.h:16: // Only directly needed
actions are included. Indirect needed actions, such as
On 2014/04/30 18:02:26, Julien Chaffraix - PST wrote:
> I don't think this comment helps, especially since we don't intent to include
> the "indirect" actions, which are just consequences of the StyleDifference
> anyway.

Removed.

[Xianzhu, 2014-04-30 19:21:19]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-30 19:21:39]

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

[commit-bot: I haz the power, 2014-04-30 20:33:48]

Change committed as 173023