Use outlineBox if we have an outline or shadow

Use outlineBox if we have an outline or shadow

Currently we decided to repaint if the outline box has moved or if we have to
paint the background and our outline box has changed shape. This CL updates the
code to only look at the outlineBox if we have an outline or we have a box
shadow.

BUG=320139

[eseidel, 2014-03-05 22:09:41]

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

https://codereview.chromium.org/187813004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1520: || (mustRepaintBackgroundOrBorder()
&& (newBounds != oldBounds || (hasOutlineBox && newOutlineBox !=
oldOutlineBox))))
You already check hasOutlineBox and location != location.  If that's true, then
this later one must be true.  This seems uneeded.

[dsinclair, 2014-03-05 22:15:05]

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

https://codereview.chromium.org/187813004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1520: || (mustRepaintBackgroundOrBorder()
&& (newBounds != oldBounds || (hasOutlineBox && newOutlineBox !=
oldOutlineBox))))
On 2014/03/05 22:09:41, eseidel wrote:
> You already check hasOutlineBox and location != location.  If that's true,
then
> this later one must be true.  This seems uneeded.

If the location's are equal that doesn't mean that the sizes can't be different.
Possibly I should split this to be a nested if to make it easier to read?

If the locations are the same we won't do the second comparison.

[dsinclair, 2014-03-06 21:35:28]

This works to fix the painting issue when we ignore the outline box. I'm
guessing it's the wrong solution, but I'm not sure what the correct solution is.

[Julien - ping for review, 2014-03-07 00:01:09]

> I'm guessing it's the wrong solution, but I'm not sure what the correct
solution is.

The positioned elements part is the right solution IMO but doesn't go far enough
AFAICT.

There are cases where I don't see how we could avoid forcing your descendants to
layout / repaint with the current code: this is one case, but toggling the
overflow clip and changing transform are other ones.

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

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1734: void
RenderBlock::layoutPositionedObjects(bool relayoutChildren, bool
fixedPositionObjectsOnly, bool parentMoved)
s/parent/containingBlock/ (positioned object is the reason why we talk about
containing blocks and not parents :))

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1790: r->setNeedsLayout();
Is this guaranteed to work? We only ensure that the positioned objects that are
under |this| get repainted, not the whole positioned hierarchy, which is what we
would need (logicalLeft is the offset from your containing block which has only
changed for the renderer being shifted)

Also shouldn't this be setNeedsPositionedMovementLayout() for the case you're
interested in?

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:293: void layoutPositionedObjects(bool
relayoutChildren, bool fixedPositionObjectsOnly = false, bool parentMoved =
false);
Really not a huge fan of more booleans here.

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

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1518: bool hasOutlineBox = hasOutline()
|| (style()->boxShadow() && style()->boxShadow()->shadows().size() > 0);
style()->boxShadow() && style()->boxShadow()->shadows().size() > 0 looks like it
could be moved to RenderStyle::hasBoxShadow() to match other getters.

Maybe we want hasOutlineOrBoxShadow as a new getter?

[dsinclair, 2014-03-07 03:18:55]

> There are cases where I don't see how we could avoid forcing your descendants
to layout / repaint with the current code: this is one case, but toggling the
overflow clip and changing transform are other ones.

I'm not sure how to proceed with this at the moment, without an example file I
won't know if I've fixed the issue. Should I put in a FIXME that there are other
cases to be handled in layoutPositionedObjects and file a bug for handling
overflow clip and transform?

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

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1734: void
RenderBlock::layoutPositionedObjects(bool relayoutChildren, bool
fixedPositionObjectsOnly, bool parentMoved)
On 2014/03/07 00:01:10, Julien Chaffraix - PST wrote:
> s/parent/containingBlock/ (positioned object is the reason why we talk about
> containing blocks and not parents :))

Done.

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1790: r->setNeedsLayout();
On 2014/03/07 00:01:10, Julien Chaffraix - PST wrote:
> Is this guaranteed to work? We only ensure that the positioned objects that
are
> under |this| get repainted, not the whole positioned hierarchy, which is what
we
> would need (logicalLeft is the offset from your containing block which has
only
> changed for the renderer being shifted)

I don't know? This seemed to work for the test case I have, not sure if there
are other cases that need to be handled in here as well.


> Also shouldn't this be setNeedsPositionedMovementLayout() for the case you're
> interested in?

If I do r->setNeedsPositionedMovementLayout() here it doesn't seem to cause the
invalidation. Having it as setNeedsLayout() does trigger the invalidation. Is
there something else I'm missing?.

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:293: void layoutPositionedObjects(bool
relayoutChildren, bool fixedPositionObjectsOnly = false, bool parentMoved =
false);
On 2014/03/07 00:01:10, Julien Chaffraix - PST wrote:
> Really not a huge fan of more booleans here.

Better?

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

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1518: bool hasOutlineBox = hasOutline()
|| (style()->boxShadow() && style()->boxShadow()->shadows().size() > 0);
On 2014/03/07 00:01:10, Julien Chaffraix - PST wrote:
> style()->boxShadow() && style()->boxShadow()->shadows().size() > 0 looks like
it
> could be moved to RenderStyle::hasBoxShadow() to match other getters.
> 
> Maybe we want hasOutlineOrBoxShadow as a new getter?

Done.

[Julien - ping for review, 2014-03-08 01:26:52]

lgtm

>> There are cases where I don't see how we could avoid forcing your descendants
to layout / repaint with the current code: this is one case, but toggling the
overflow clip and changing transform are other ones.

> I'm not sure how to proceed with this at the moment, without an example file I
won't know if I've fixed the issue. Should I put in a FIXME that there are other
cases to be handled in layoutPositionedObjects and file a bug for handling
overflow clip and transform?

This was a more general comment that we would need some mechanism to force some
subtree relayout (we do it in some case but with RAL we have to do it in
others). It's not really actionable at this moment.

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

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1790: r->setNeedsLayout();
On 2014/03/07 03:18:56, dsinclair wrote:
> On 2014/03/07 00:01:10, Julien Chaffraix - PST wrote:
> > Is this guaranteed to work? We only ensure that the positioned objects that
> are
> > under |this| get repainted, not the whole positioned hierarchy, which is
what
> we
> > would need (logicalLeft is the offset from your containing block which has
> only
> > changed for the renderer being shifted)
> 
> I don't know? This seemed to work for the test case I have, not sure if there
> are other cases that need to be handled in here as well.

The whole positioned hierarchy is shifted in this case but we only ever force an
invalidation for the direct positioned descendant. We talked about some cases I
thought would defeat this but they worked which makes me wonder if something
else is not generating invalidations.

> > Also shouldn't this be setNeedsPositionedMovementLayout() for the case
you're
> > interested in?
> 
> If I do r->setNeedsPositionedMovementLayout() here it doesn't seem to cause
the
> invalidation. Having it as setNeedsLayout() does trigger the invalidation. Is
> there something else I'm missing?.

That's weird, maybe a full layout forces the descendants to be invalidated. I
would rather have this understood and investigated as this will definitely bite
us down the road. I could live with a FIXME with a blocking bug on RAL but only
if this blocks something.

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:293: enum LayoutPositionedInfo {
How about PositionedLayoutBehavior? (info isn't really the word here, it's more
like the behavior / reason for layout)

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:294: LayoutPositionedDefault,
Maybe just DefaultLayout?

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:295: LayoutPositionedOnlyFixed,
LayoutOnlyFixedPositionedObjects

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:296: LayoutPositionedContainingBlockMoved
ForcedLayoutAfterContainingBlockMoved

[dsinclair, 2014-03-10 00:22:51]

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

https://codereview.chromium.org/187813004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1790: r->setNeedsLayout();
On 2014/03/08 01:26:53, Julien Chaffraix - PST wrote:
> On 2014/03/07 03:18:56, dsinclair wrote:
> > On 2014/03/07 00:01:10, Julien Chaffraix - PST wrote:
> > > Is this guaranteed to work? We only ensure that the positioned objects
that
> > are
> > > under |this| get repainted, not the whole positioned hierarchy, which is
> what
> > we
> > > would need (logicalLeft is the offset from your containing block which has
> > only
> > > changed for the renderer being shifted)
> > 
> > I don't know? This seemed to work for the test case I have, not sure if
there
> > are other cases that need to be handled in here as well.
> 
> The whole positioned hierarchy is shifted in this case but we only ever force
an
> invalidation for the direct positioned descendant. We talked about some cases
I
> thought would defeat this but they worked which makes me wonder if something
> else is not generating invalidations.
> 
> > > Also shouldn't this be setNeedsPositionedMovementLayout() for the case
> you're
> > > interested in?
> > 
> > If I do r->setNeedsPositionedMovementLayout() here it doesn't seem to cause
> the
> > invalidation. Having it as setNeedsLayout() does trigger the invalidation.
Is
> > there something else I'm missing?.
> 
> That's weird, maybe a full layout forces the descendants to be invalidated. I
> would rather have this understood and investigated as this will definitely
bite
> us down the road. I could live with a FIXME with a blocking bug on RAL but
only
> if this blocks something.

Done.

crbug.com/350756  I didn't mark the bug as blocking RAL as I don't think it
blocks anything at the moment?

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:293: enum LayoutPositionedInfo {
On 2014/03/08 01:26:53, Julien Chaffraix - PST wrote:
> How about PositionedLayoutBehavior? (info isn't really the word here, it's
more
> like the behavior / reason for layout)

Done.

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:294: LayoutPositionedDefault,
On 2014/03/08 01:26:53, Julien Chaffraix - PST wrote:
> Maybe just DefaultLayout?

Done.

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:295: LayoutPositionedOnlyFixed,
On 2014/03/08 01:26:53, Julien Chaffraix - PST wrote:
> LayoutOnlyFixedPositionedObjects

Done.

https://codereview.chromium.org/187813004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:296: LayoutPositionedContainingBlockMoved
On 2014/03/08 01:26:53, Julien Chaffraix - PST wrote:
> ForcedLayoutAfterContainingBlockMoved

Done.

[dsinclair, 2014-03-10 00:22:54]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2014-03-10 00:23:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/187813004/60001

[commit-bot: I haz the power, 2014-03-10 00:53:02]

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

[commit-bot: I haz the power, 2014-03-10 00:53:03]

Try jobs failed on following builders: mac_blink_rel

[Julien - ping for review, 2014-03-10 22:28:51]

Took over the change in https://codereview.chromium.org/192863004/ as we need
that for repaint-after-layout.