Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 9 months ago by dsinclair Modified:
6 years, 8 months ago Reviewers:
ojan, leviw_travelin_and_unemployed, eseidel, esprehn, Julien - ping for review CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr. Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionUse 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
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Created: 6 years, 9 months ago
Messages
Total messages: 12 (0 generated)
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.
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.
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.
> 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?
> 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.
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
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.
The CQ bit was checked by dsinclair@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dsinclair@chromium.org/187813004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel
Took over the change in https://codereview.chromium.org/192863004/ as we need that for repaint-after-layout. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
