Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
160903002:
Move RenderLayer repainting to repaint-after-layout framework. (Closed)
|
Created:
6 years, 10 months ago by dsinclair Modified:
6 years, 9 months ago CC:
blink-reviews, bemjb+rendering_chromium.org, zoltan1, eae+blinkwatch, leviw+renderwatch, blink-layers+watch_chromium.org, jchaffraix+rendering, pdr. Base URL:
https://chromium.googlesource.com/chromium/blink@master Visibility:
Public. |
DescriptionMove RenderLayer repainting to repaint-after-layout framework.
This CL updates the RenderLayer repainter code to be a no-op during
repaint-after-layout. The repainting of updated layers is handled in the
repaintTree method, the same as non-layer RenderObjects.
While making this change we discovered that the outline rect can not be
calculated from the repaint rect. So, we've had to add the old/new outline rects to the RenderObject so we can have the correct values.
BUG=320139
COLLABORATOR=jchaffraix@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169684
Patch Set 1 : #
Total comments: 8
Patch Set 2 : #
Total comments: 7
Patch Set 3 : #
Total comments: 25
Patch Set 4 : #Patch Set 5 : Rebase to master #
Total comments: 8
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Rebase to master #Patch Set 9 : Rebase to master #Patch Set 10 : Rebase to master #
Messages
Total messages: 40 (0 generated)
This CL currently has a dozen under-painting pixel tests and a bunch of rect tests that I'll have to manually verify. I wanted to send it out to give you a chance to take a look to see if it fits with what you were thinking in this direction.
https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:474: LayoutRectRecorder recorder(*m_renderer); This seems too heavy handed (but we could leave with a FIXME while we figure out what to do). Very few elements move during a scroll (fixed positioned and sticky position are the one I can think of). https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:718: LayoutRectRecorder recorder(*m_renderer); I wouldn't expect changing visible content (mostly position: visible) to cause some position change. https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderLayerRepainter.cpp (right): https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayerRepainter.cpp:113: if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled()) My naive view is that this code shouldn't run at all if RAL is enabled. https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayerRepainter.cpp:125: LayoutRectRecorder recorder(*m_renderer); Won't this force m_oldRepaintRect == m_newRepaintRect as no layout happened in the meantime?
https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:474: LayoutRectRecorder recorder(*m_renderer); On 2014/02/14 18:05:48, Julien Chaffraix - PST wrote: > This seems too heavy handed (but we could leave with a FIXME while we figure out > what to do). > > Very few elements move during a scroll (fixed positioned and sticky position are > the one I can think of). It's possible this isn't needed at all. I've been putting in extra calls in order to see if it's missing rect storage that's causing my under painting. Will mark with a FIXME to remove if possible. https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:718: LayoutRectRecorder recorder(*m_renderer); On 2014/02/14 18:05:48, Julien Chaffraix - PST wrote: > I wouldn't expect changing visible content (mostly position: visible) to cause > some position change. So, the reason I did this here is because I'm currently mimicking when the current layer code would get the repaint rect. I wanted to make sure that I wasn't missing anything. Once I've got the bugs worked out we can experiment with removing some of these LayoutRectRecorder calls and see if anything breaks. Just trying to minimize the number of differnces between my changes and the old system in order to track down the under-painting. https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderLayerRepainter.cpp (right): https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayerRepainter.cpp:113: if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled()) On 2014/02/14 18:05:48, Julien Chaffraix - PST wrote: > My naive view is that this code shouldn't run at all if RAL is enabled. When you say this code, do you mean anything in this file, or this method specifically? For this call specifically, I figured it was easier to early return then wrap all callers in a if (!RuntimeEnabledFeature::repaintAfterLayoutEnabled()) https://codereview.chromium.org/160903002/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayerRepainter.cpp:125: LayoutRectRecorder recorder(*m_renderer); On 2014/02/14 18:05:48, Julien Chaffraix - PST wrote: > Won't this force m_oldRepaintRect == m_newRepaintRect as no layout happened in > the meantime? Possibly? If we've already updated oldRepaintRect we won't touch with this call. At most, we could change the newRepaintRect. If layout didn't happen, then this would end up setting the two rects to be the same, but then they should, basically, get dropped in repaintAfterLayoutIfNeeded because the rects are the same and we haven't set to force a full repaint.
Just noticed I didn't put any comments when I sent this out. So, everyone, PTAL. There is no known under-painting tests but I need to go through and manually verify all of the text only tests to make sure they aren't underpainting. The big thing in this CL is the added new/old outline rects we had to store due to outline rects not being based from the repaint rects.
On 2014/02/24 16:27:48, dsinclair wrote: > Just noticed I didn't put any comments when I sent this out. So, everyone, PTAL. > > There is no known under-painting tests but I need to go through and manually > verify all of the text only tests to make sure they aren't underpainting. > > The big thing in this CL is the added new/old outline rects we had to store due > to outline rects not being based from the repaint rects. This doesn't make sense to me. Your outline is definitely based on your location in the page, you shouldn't need to store both. Again I don't think we should make this change, the number of elements with outline is basically one per page. Why do the outlines not line up? We should just fix that.
On 2014/02/24 16:57:10, esprehn wrote: > On 2014/02/24 16:27:48, dsinclair wrote: > > Just noticed I didn't put any comments when I sent this out. So, everyone, > PTAL. > > > > There is no known under-painting tests but I need to go through and manually > > verify all of the text only tests to make sure they aren't underpainting. > > > > The big thing in this CL is the added new/old outline rects we had to store > due > > to outline rects not being based from the repaint rects. > > This doesn't make sense to me. Your outline is definitely based on your location > in the page, you shouldn't need to store both. > > Again I don't think we should make this change, the number of elements with > outline is basically one per page. > > Why do the outlines not line up? We should just fix that. We've filed crbug.com/345452. The one problem we'll run into is that, once the outlines line-up the test case mentioned in that bug will fail as it's the old/new outlines being different that actually trigger the painting. I'm not sure if there are other tests that will fail other then the one given.
On 2014/02/24 17:01:08, dsinclair wrote: > On 2014/02/24 16:57:10, esprehn wrote: > > On 2014/02/24 16:27:48, dsinclair wrote: > > > Just noticed I didn't put any comments when I sent this out. So, everyone, > > PTAL. > > > > > > There is no known under-painting tests but I need to go through and manually > > > verify all of the text only tests to make sure they aren't underpainting. > > > > > > The big thing in this CL is the added new/old outline rects we had to store > > due > > > to outline rects not being based from the repaint rects. > > > > This doesn't make sense to me. Your outline is definitely based on your > location > > in the page, you shouldn't need to store both. > > > > Again I don't think we should make this change, the number of elements with > > outline is basically one per page. > > > > Why do the outlines not line up? We should just fix that. > > > We've filed crbug.com/345452. The one problem we'll run into is that, once the > outlines line-up the test case mentioned in that bug will fail as it's the > old/new outlines being different that actually trigger the painting. I'm not > sure if there are other tests that will fail other then the one given. Regarding storing the outline rects off to the side instead of inside RenderObject, where would be the appropriate place to hold onto the hash? Should it go in RenderView or is there some place that's better?
RenderView SGTM. It's always the root of the rendering tree.
PTAL. I added the HashMap to RenderView and conditionally get/set the outline rects from the map. There are 3 crashes I'm seeing, not related to the HashMap code, but related to this CL that I'm trying to track down at the moment. For some reason, with this CL applied, the SVGImage code triggers a repaint during layout when it shouldn't. This doesn't happen without this CL.
ping. The crashes should all be fixed, as far as I can tell there is no underpainting with repaint-after-layout enabled.
This won't work, you can't look at your children inside style recalc. You're adding more chicken and egg bugs. https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... Source/core/rendering/RenderBox.cpp:298: for (RenderObject* child = firstChild(); child; child = child->nextSibling()) { You can't do this, you're looking at your children in the middle of recalcStyle. https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:3319: OutlineRects* outlineRects = view()->outlineRects(); ditto, you need a "hasOutlineRects" bitfield to guard the hash table. Hitting it for every renderer inside paint isn't going to work. https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:3338: OutlineRects* outlineRects = view()->outlineRects(); You need to use a bitfield so you don't hit the hash table all the time.
https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... Source/core/rendering/RenderBox.cpp:298: for (RenderObject* child = firstChild(); child; child = child->nextSibling()) { On 2014/02/28 02:54:33, esprehn wrote: > You can't do this, you're looking at your children in the middle of recalcStyle. Changed to storing a flag here and checking the isSelfPaintingLayer() when we do FrameView::repaintTree(). https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:3319: OutlineRects* outlineRects = view()->outlineRects(); On 2014/02/28 02:54:33, esprehn wrote: > ditto, you need a "hasOutlineRects" bitfield to guard the hash table. Hitting it > for every renderer inside paint isn't going to work. ack. https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:3338: OutlineRects* outlineRects = view()->outlineRects(); On 2014/02/28 02:54:33, esprehn wrote: > You need to use a bitfield so you don't hit the hash table all the time. I did the check on the other side, I only query these if hasOutline(). I've added an ASSERT(hasOutline()) into each of these methods to make it clear that it's a precondition. Although, I'm wondering now if we need a second bit anyway, is it possible that the outline state could have changed during the layout? i.e. we had an old outline but we don't have a new outline?
Rebase to master
Rebase to master
I appear to have managed to get an upload that didn't 500 and doesn't have chunk mis-match errors. Sorry for the extra patch sets.
ping
https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/160903002/diff/610001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:3338: OutlineRects* outlineRects = view()->outlineRects(); On 2014/02/28 18:58:24, dsinclair wrote: > On 2014/02/28 02:54:33, esprehn wrote: > > You need to use a bitfield so you don't hit the hash table all the time. > > I did the check on the other side, I only query these if hasOutline(). I've > added an ASSERT(hasOutline()) into each of these methods to make it clear that > it's a precondition. > > Although, I'm wondering now if we need a second bit anyway, is it possible that > the outline state could have changed during the layout? i.e. we had an old > outline but we don't have a new outline? Outline is a style property so it won't change during layout. That said, we compute the fudge factor during layout based on the maximal outline size and that will change. https://codereview.chromium.org/160903002/diff/1170001/LayoutTests/fast/repai... File LayoutTests/fast/repaint/bugzilla-6388.html (right): https://codereview.chromium.org/160903002/diff/1170001/LayoutTests/fast/repai... LayoutTests/fast/repaint/bugzilla-6388.html:27: <div id="blue" style="width: 100px; height: 100px; background: blue;"></div> These new ids are not needed for the test so they shouldn't be added. You can always add them back locally for debugging. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderBlock.cpp:1794: r->setShouldDoFullRepaintAfterLayout(true); This will cause all positioned objects to be repainted, even when they didn't need layout. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderLayer.cpp:932: LayoutRectRecorder recorder(*m_renderer); This shouldn't be needed and *will* cause over-invalidations as we will record a new repaint rectangle for renderers that were not laid out. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderLayerModelObject.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderLayerModelObject.cpp:152: layer()->renderer()->setShouldDoFullRepaintAfterLayout(true); This seems overly broad as we could get a layer without visual impact (e.g. an overflow only layer or a forced one). We should have a FIXME about removing this crazy hammer and doing the right invalidation at RenderStyle::diff time. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderLayerRepainter.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderLayerRepainter.cpp:124: // object but it makes sure we correctly set all of the repaint flags. It's also weird that we ignore all computeRepaintRects calls apart from this one. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderLayerScrollableArea.cpp:396: if (m_box->frameView()->isInPerformLayout()) Why do we need to check isInPerformLayout() and why would it matter? https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:759: setShouldDoFullRepaintAfterLayout(true); This is wrong :-/ It's OK to put a FIXME but this is an important optimization as it removes the need for repainting when the object is hardware accelerated and we just move it around. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:1610: return !document().view()->needsFullRepaint() && everHadLayout(); FYI I think we shouldn't check everHadLayout() here but it's an orthogonal change. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:3328: ASSERT(hasOutline()); The outline rectangle includes the box shadow too (see RenderBox::outlineBoundsForRepaint in particular the call to adjustRectForOutlineAndShadow). https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:3349: const LayoutRect RenderObject::oldOutlineRect() I don't see the point in making a copy of LayoutRect const. If it was a reference it would be OK but you can't (unless you have a static local for the not-found case) https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.h:1104: , m_shouldDoFullRepaintIfSelfPaintingLayer(false) Why do we care about self-painting layer in the RAL as we always use the RAL logic for repainting? https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderView.h (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderView.h:216: OutlineRects* outlineRects() { return &m_outlineRects; } It really seems like it should be a reference as the pointer can't be NULL. To avoid hidden copy, we should make OutlineRectInfo NonCopyable.
I fixed the massive over-painting, but have caused under-painting on fast/repaint/bugzilla-6388.html which I need to track down. https://codereview.chromium.org/160903002/diff/1170001/LayoutTests/fast/repai... File LayoutTests/fast/repaint/bugzilla-6388.html (right): https://codereview.chromium.org/160903002/diff/1170001/LayoutTests/fast/repai... LayoutTests/fast/repaint/bugzilla-6388.html:27: <div id="blue" style="width: 100px; height: 100px; background: blue;"></div> On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > These new ids are not needed for the test so they shouldn't be added. You can > always add them back locally for debugging. Done. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderBlock.cpp:1794: r->setShouldDoFullRepaintAfterLayout(true); On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > This will cause all positioned objects to be repainted, even when they didn't > need layout. Done. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderLayer.cpp:932: LayoutRectRecorder recorder(*m_renderer); On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > This shouldn't be needed and *will* cause over-invalidations as we will record a > new repaint rectangle for renderers that were not laid out. That should be fine as it checks if newBounds == oldBounds && newOutlineBox == oldOutlineBox and bails out early in repaintAfterLayoutIfNeeded. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderLayerModelObject.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderLayerModelObject.cpp:152: layer()->renderer()->setShouldDoFullRepaintAfterLayout(true); On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > This seems overly broad as we could get a layer without visual impact (e.g. an > overflow only layer or a forced one). We should have a FIXME about removing this > crazy hammer and doing the right invalidation at RenderStyle::diff time. Add a FIXME and crbug.com/349061. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderLayerRepainter.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderLayerRepainter.cpp:124: // object but it makes sure we correctly set all of the repaint flags. On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > It's also weird that we ignore all computeRepaintRects calls apart from this > one. This is used by the compositing code that I don't understand at the moment. It's called inside of: - CompositedLayerMapping::paintsIntoCompositedAncestorChanged - RenderLayerCompositor::allocateOrClearCompositedLayerMapping https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderLayerScrollableArea.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderLayerScrollableArea.cpp:396: if (m_box->frameView()->isInPerformLayout()) On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > Why do we need to check isInPerformLayout() and why would it matter? This code can be executed outside of layout. As far as I could tell, we need to make sure we do the repaint in that case. So, if we're in layout we defer the repaint, if we're not doing layout we allow the repaint to happen. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:759: setShouldDoFullRepaintAfterLayout(true); On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > This is wrong :-/ > > It's OK to put a FIXME but this is an important optimization as it removes the > need for repainting when the object is hardware accelerated and we just move it > around. Done. Had to store another flag in this case so we can check the composited state. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:1610: return !document().view()->needsFullRepaint() && everHadLayout(); On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > FYI I think we shouldn't check everHadLayout() here but it's an orthogonal > change. Ack. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:3328: ASSERT(hasOutline()); On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > The outline rectangle includes the box shadow too (see > RenderBox::outlineBoundsForRepaint in particular the call to > adjustRectForOutlineAndShadow). So, then the shadow isn't included in the repaint rect? Do we have a way to know if there is a shadow? Do I check if the textShadow list is empty and the boxShadow list is empty along with the hasOutline? https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:3349: const LayoutRect RenderObject::oldOutlineRect() On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > I don't see the point in making a copy of LayoutRect const. If it was a > reference it would be OK but you can't (unless you have a static local for the > not-found case) Done. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.h:1104: , m_shouldDoFullRepaintIfSelfPaintingLayer(false) On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > Why do we care about self-painting layer in the RAL as we always use the RAL > logic for repainting? Inside RenderBox::updateFromStyle() we had to disable the repaint() call for the !hasOverflowClip() check. In order to get the needed paints to happen we added in a loop over the children and were setting the shoudDoFullRepaint check at that point. But, this introduces a chicken-and-egg for style as we can't check the child isSelfPaintingLayer() since we're currently in a style update. So, I need to store that fact away so we can check later. https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderView.h (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderView.h:216: OutlineRects* outlineRects() { return &m_outlineRects; } On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > It really seems like it should be a reference as the pointer can't be NULL. To > avoid hidden copy, we should make OutlineRectInfo NonCopyable. Changed the return to be a reference. I didn't change OutlineRectInfo to be non-copyable as that's how I'm currently setting the OutlineRectInfo into the map. Is that wrong?
ping. There is one remaining under-paint case, but I think we can fix that after this is committed. I'd like to get this in so Levi can continue with the work he's doing.
https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/160903002/diff/1170001/Source/core/rendering/... Source/core/rendering/RenderObject.cpp:3328: ASSERT(hasOutline()); On 2014/03/04 19:21:52, dsinclair wrote: > On 2014/03/04 18:22:10, Julien Chaffraix - PST wrote: > > The outline rectangle includes the box shadow too (see > > RenderBox::outlineBoundsForRepaint in particular the call to > > adjustRectForOutlineAndShadow). > > > So, then the shadow isn't included in the repaint rect? Do we have a way to know > if there is a shadow? It is actually included in the repaint rect (see addVisualEffectOverflow). A reason why we would need to account for it here is if it needs to ignore some overflow clip but that seems unlikely. > Do I check if the textShadow list is empty and the boxShadow list is empty along > with the hasOutline? I would have kept the intended logic however wonky it is as this patch is already pretty heavy without it. Alternatively, we can try to remove box-shadow from the outlineBox and see if something breaks (but it should be done before this change to help narrow regressions). https://codereview.chromium.org/160903002/diff/720015/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/160903002/diff/720015/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1081: } Shouldn't we compute these only in the branch where we need them? https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... Source/core/rendering/RenderBox.cpp:297: // generated by positioned children or self painting layers. crbug.com/345403 Wouldn't the patch on https://codereview.chromium.org/187813004/ solve this problem at least for the positioned case? https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... File Source/core/rendering/RenderLayerRepainter.cpp (right): https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... Source/core/rendering/RenderLayerRepainter.cpp:124: // object but it makes sure we correctly set all of the repaint flags. This should be a FIXME as we will want this whole repainting logic to go away. https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... Source/core/rendering/RenderObject.h:1115: , m_shouldDoFullRepaintIfNotComposited(false) The second boolean is not needed if we clear the needsLayout flags during repaintTree (this is possible while keeping our no-renderer-needs-layout assert as we now track whether we actually called layout on any renderer). Adding more reference to self-painting layer is a bad smell in some code that is RenderLayer agnostic so there is something we should be doing that we don't. I expect it has to do with positioned elements that we need to relayout but don't in this case.
https://codereview.chromium.org/160903002/diff/720015/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/160903002/diff/720015/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1081: } On 2014/03/07 18:26:38, Julien Chaffraix - PST wrote: > Shouldn't we compute these only in the branch where we need them? Done. https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... File Source/core/rendering/RenderLayerRepainter.cpp (right): https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... Source/core/rendering/RenderLayerRepainter.cpp:124: // object but it makes sure we correctly set all of the repaint flags. On 2014/03/07 18:26:38, Julien Chaffraix - PST wrote: > This should be a FIXME as we will want this whole repainting logic to go away. Done. This may also go away with the work that Levi is doing to change when the invalidation rects are gathered. https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... Source/core/rendering/RenderObject.h:1115: , m_shouldDoFullRepaintIfNotComposited(false) On 2014/03/07 18:26:38, Julien Chaffraix - PST wrote: > The second boolean is not needed if we clear the needsLayout flags during > repaintTree (this is possible while keeping our no-renderer-needs-layout assert > as we now track whether we actually called layout on any renderer). I'm not sure if m_shouldDoFullRepaintIfNotComposited can go away. It's the replacement for the RenderLayerRepainter shouldRepaintAfterLayout check. In RenderObject::setLayerNeedsFullRepaintForPositionedMovementLayout() I need to know if this is a composited layer but I can't ask there as we're called from setStyle so I need to delay the check until after style is done and this flag lets me do it. > Adding more reference to self-painting layer is a bad smell in some code that is > RenderLayer agnostic so there is something we should be doing that we don't. I > expect it has to do with positioned elements that we need to relayout but don't > in this case. Ah, so you think it's either covered, or should be handled similar to https://codereview.chromium.org/187813004/. Once that's landed I'll try removing this and see if there is a better solution.
https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/160903002/diff/720015/Source/core/rendering/R... Source/core/rendering/RenderObject.h:1115: , m_shouldDoFullRepaintIfNotComposited(false) On 2014/03/07 19:29:48, dsinclair wrote: > On 2014/03/07 18:26:38, Julien Chaffraix - PST wrote: > > The second boolean is not needed if we clear the needsLayout flags during > > repaintTree (this is possible while keeping our no-renderer-needs-layout > assert > > as we now track whether we actually called layout on any renderer). > > I'm not sure if m_shouldDoFullRepaintIfNotComposited can go away. It's the > replacement for the RenderLayerRepainter shouldRepaintAfterLayout check. In > RenderObject::setLayerNeedsFullRepaintForPositionedMovementLayout() I need to > know if this is a composited layer but I can't ask there as we're called from > setStyle so I need to delay the check until after style is done and this flag > lets me do it. I wasn't too stressed out about this one. It would be just better if we just kept the needsPositionedMovementOnly boolean across layout and used that to make the invalidation decision. We will need to keep track of our layout bits anyway to skip certain invalidation so this would be going into the right direction. > > Adding more reference to self-painting layer is a bad smell in some code that > is > > RenderLayer agnostic so there is something we should be doing that we don't. I > > expect it has to do with positioned elements that we need to relayout but > don't > > in this case. > > Ah, so you think it's either covered, or should be handled similar to > https://codereview.chromium.org/187813004/. Once that's landed I'll try removing > this and see if there is a better solution. I thought that there would be a way to make this work by detecting overflow change and relaying out the appropriate renderers. It turns out that this would yield to a lot of over-repainting or we would need to check isSelfPaintingLayer. The back-story is that we don't track the visual overflow for self-painting layer. That's the reason for this boolean to exist. We should fix our visual overflow but we shouldn't gate RAL on that.
Based on my previous comment and some work I did on the change, this is good to go -> LGTM!
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/160903002/1260001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/frame/FrameView.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/frame/FrameView.cpp
Hunk #1 FAILED at 1055.
Hunk #2 succeeded at 1091 with fuzz 2 (offset -7 lines).
1 out of 2 hunks FAILED -- saving rejects to file
Source/core/frame/FrameView.cpp.rej
Patch: Source/core/frame/FrameView.cpp
Index: Source/core/frame/FrameView.cpp
diff --git a/Source/core/frame/FrameView.cpp b/Source/core/frame/FrameView.cpp
index
c111f2c4bd18fec240b84fd55b0b9862c15f6f04..280599c6474cdde3799b74db1033df13c11386f6
100644
--- a/Source/core/frame/FrameView.cpp
+++ b/Source/core/frame/FrameView.cpp
@@ -1055,28 +1055,35 @@ void FrameView::repaintTree(RenderObject* root)
const LayoutRect& oldRepaintRect = renderer->oldRepaintRect();
const LayoutRect& newRepaintRect = renderer->newRepaintRect();
- LayoutRect oldOutlineRect = oldRepaintRect;
- oldOutlineRect.inflate(renderView()->oldMaximalOutlineSize());
-
- LayoutRect newOutlineRect = newRepaintRect;
- newOutlineRect.inflate(renderView()->maximalOutlineSize());
+ if ((renderer->onlyNeededPositionedMovementLayout() &&
renderer->compositingState() != PaintsIntoOwnBacking)
+ || (renderer->shouldDoFullRepaintIfSelfPaintingLayer()
+ && renderer->hasLayer()
+ &&
toRenderLayerModelObject(renderer)->layer()->isSelfPaintingLayer())) {
+ renderer->setShouldDoFullRepaintAfterLayout(true);
+ }
// FIXME: Currently renderers with layers will get repainted when we
call updateLayerPositionsAfterLayout.
// That call should be broken apart to position the layers be done
before
// the repaintTree call so this will repaint everything.
bool didFullRepaint = false;
- if (!renderer->hasLayer()) {
- if (!renderer->layoutDidGetCalled()) {
- if (renderer->shouldDoFullRepaintAfterLayout()) {
- renderer->repaint();
- didFullRepaint = true;
- }
+ if (!renderer->layoutDidGetCalled()) {
+ if (renderer->shouldDoFullRepaintAfterLayout()) {
+ renderer->repaint();
+ didFullRepaint = true;
+ }
- } else {
- didFullRepaint =
renderer->repaintAfterLayoutIfNeeded(renderer->containerForRepaint(),
- renderer->shouldDoFullRepaintAfterLayout(), oldRepaintRect,
oldOutlineRect,
- &newRepaintRect, &newOutlineRect);
+ } else {
+ LayoutRect oldOutlineRect;
+ LayoutRect newOutlineRect;
+
+ if (renderer->hasOutline()) {
+ newOutlineRect = renderer->newOutlineRect();
+ oldOutlineRect = renderer->oldOutlineRect();
}
+
+ didFullRepaint =
renderer->repaintAfterLayoutIfNeeded(renderer->containerForRepaint(),
+ renderer->shouldDoFullRepaintAfterLayout(), oldRepaintRect,
oldOutlineRect,
+ &newRepaintRect, &newOutlineRect);
}
if (!didFullRepaint)
@@ -1098,7 +1105,7 @@ void FrameView::repaintTree(RenderObject* root)
listBox->repaintScrollbarIfNeeded();
}
- renderer->clearRepaintRects();
+ renderer->clearRepaintState();
}
renderView()->setOldMaximalOutlineSize(0);
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/160903002/1280001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/frame/FrameView.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/frame/FrameView.cpp
Hunk #1 FAILED at 1054.
Hunk #2 succeeded at 1091 with fuzz 2 (offset -6 lines).
1 out of 2 hunks FAILED -- saving rejects to file
Source/core/frame/FrameView.cpp.rej
Patch: Source/core/frame/FrameView.cpp
Index: Source/core/frame/FrameView.cpp
diff --git a/Source/core/frame/FrameView.cpp b/Source/core/frame/FrameView.cpp
index
0fb5734a1d7d2b52a08790ed62421ce3df18f375..704f89a4603ef7522db52be8e30ba28e407660eb
100644
--- a/Source/core/frame/FrameView.cpp
+++ b/Source/core/frame/FrameView.cpp
@@ -1054,28 +1054,35 @@ void FrameView::repaintTree(RenderObject* root)
const LayoutRect& oldRepaintRect = renderer->oldRepaintRect();
const LayoutRect& newRepaintRect = renderer->newRepaintRect();
- LayoutRect oldOutlineRect = oldRepaintRect;
- oldOutlineRect.inflate(renderView()->oldMaximalOutlineSize());
-
- LayoutRect newOutlineRect = newRepaintRect;
- newOutlineRect.inflate(renderView()->maximalOutlineSize());
+ if ((renderer->onlyNeededPositionedMovementLayout() &&
renderer->compositingState() != PaintsIntoOwnBacking)
+ || (renderer->shouldDoFullRepaintIfSelfPaintingLayer()
+ && renderer->hasLayer()
+ &&
toRenderLayerModelObject(renderer)->layer()->isSelfPaintingLayer())) {
+ renderer->setShouldDoFullRepaintAfterLayout(true);
+ }
// FIXME: Currently renderers with layers will get repainted when we
call updateLayerPositionsAfterLayout.
// That call should be broken apart to position the layers be done
before
// the repaintTree call so this will repaint everything.
bool didFullRepaint = false;
- if (!renderer->hasLayer()) {
- if (!renderer->layoutDidGetCalled()) {
- if (renderer->shouldDoFullRepaintAfterLayout()) {
- renderer->repaint();
- didFullRepaint = true;
- }
+ if (!renderer->layoutDidGetCalled()) {
+ if (renderer->shouldDoFullRepaintAfterLayout()) {
+ renderer->repaint();
+ didFullRepaint = true;
+ }
- } else {
- didFullRepaint =
renderer->repaintAfterLayoutIfNeeded(renderer->containerForRepaint(),
- renderer->shouldDoFullRepaintAfterLayout(), oldRepaintRect,
oldOutlineRect,
- &newRepaintRect, &newOutlineRect);
+ } else {
+ LayoutRect oldOutlineRect;
+ LayoutRect newOutlineRect;
+
+ if (renderer->hasOutline()) {
+ newOutlineRect = renderer->newOutlineRect();
+ oldOutlineRect = renderer->oldOutlineRect();
}
+
+ didFullRepaint =
renderer->repaintAfterLayoutIfNeeded(renderer->containerForRepaint(),
+ renderer->shouldDoFullRepaintAfterLayout(), oldRepaintRect,
oldOutlineRect,
+ &newRepaintRect, &newOutlineRect);
}
if (!didFullRepaint)
@@ -1097,7 +1104,7 @@ void FrameView::repaintTree(RenderObject* root)
listBox->repaintScrollbarIfNeeded();
}
- renderer->clearRepaintRects();
+ renderer->clearRepaintState();
}
renderView()->setOldMaximalOutlineSize(0);
The CQ bit was unchecked by dsinclair@chromium.org
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/160903002/1300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
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/160903002/1300001
Message was sent while issue was closed.
Change committed as 169684 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
