Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
395463003:
Invalidate previous paint rect in RenderObject::paintInvalidationForWholeRenderer() (Closed)
|
Created:
6 years, 5 months ago by Xianzhu Modified:
6 years, 4 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionInvalidate previous paint rect in RenderObject::paintInvalidationForWholeRenderer()
When an object is being removed, it calls
paintInvalidationForWholeRenderer() which previously invalidated the
current repaint rect only. However if the object has been layouted
after the last invalidation, we'll miss invalidation of the object's
previous location.
Now let the method also invalidate the previous paint rect if possible.
BUG=392794, 394004
TEST=fast/repaint/remove-block-after-layout.html
TEST=fast/repaint/remove-inline-after-layout.html
TEST=fast/repaint/remove-inline-layer-after-layout.html
R=dsinclair@chromium.org, jchaffraix@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179059
Patch Set 1 #
Total comments: 2
Patch Set 2 : FIXME about invalidation of current location #Patch Set 3 : Update previousPaintInvalidationRect #
Total comments: 4
Patch Set 4 : Add savesPreviousPaintInvalidationRect; Remove extra optimization code #
Total comments: 2
Patch Set 5 : Simplified #
Total comments: 7
Patch Set 6 : Based on https://codereview.chromium.org/398343003/ (Unified invalidation) #Patch Set 7 : Back to the original method #Patch Set 8 : Invalidate both old and new paint rects #
Total comments: 6
Patch Set 9 : Nits #
Total comments: 4
Patch Set 10 : Try layout tests #Patch Set 11 : For landing #Messages
Total messages: 49 (0 generated)
The code change makes sense. From your description: Which existing tests will be affected? Can you upload expected results for them or at least run the try bots so we can see what breaks?
On 2014/07/14 19:03:50, leviw wrote: > The code change makes sense. > > From your description: Which existing tests will be affected? Can you upload > expected results for them or at least run the try bots so we can see what > breaks? I planned to run try bots but forgot to do that (sorry). Will let you know when the results are available.
Now part of layout test results are available on try bots. Note that because of current NeedsRebaselines, some test expectation changes may be hidden.
On 2014/07/14 at 21:31:36, wangxianzhu wrote: > Now part of layout test results are available on try bots. Note that because of current NeedsRebaselines, some test expectation changes may be hidden. Looking at the results, your comment in the description that no new invalidation occurs on old tests is wrong... The first 3 results all have new invalidation rects: https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
On 2014/07/14 21:49:23, leviw wrote: > On 2014/07/14 at 21:31:36, wangxianzhu wrote: > > Now part of layout test results are available on try bots. Note that because > of current NeedsRebaselines, some test expectation changes may be hidden. > > Looking at the results, your comment in the description that no new invalidation > occurs on old tests is wrong... The first 3 results all have new invalidation > rects: > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... Please look at results of the second run instead, e.g. http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1.... Please ignore virtual/softwarecompositing/repaint/repaint/resize-repaint.html which will be rebaselined for another bug but not included in current TestExpectations because the rebaseline-o-matic seems problematic for this virtual test. The first run contains some failures of the based version (which also failed without patch).
https://codereview.chromium.org/395463003/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderObject.cpp:1537: invalidatePaintUsingContainer(paintInvalidationContainer, previousPaintInvalidationRect(), InvalidationPaint); I think we should only invalidate the previousPaintInvalidation here actually. The other invalidation above will invalidate at position we don't care about (e.g. intermediate forced layout + style recalc between frames). The reason is simple: all our invalidations should be based on the previous stored rectangles (and positions), if we don't have any that means we didn't push a frame so we don't care about invalidating.
https://codereview.chromium.org/395463003/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderObject.cpp:1537: invalidatePaintUsingContainer(paintInvalidationContainer, previousPaintInvalidationRect(), InvalidationPaint); On 2014/07/15 00:58:31, Julien Chaffraix - PST wrote: > I think we should only invalidate the previousPaintInvalidation here actually. > The other invalidation above will invalidate at position we don't care about > (e.g. intermediate forced layout + style recalc between frames). > > The reason is simple: all our invalidations should be based on the previous > stored rectangles (and positions), if we don't have any that means we didn't > push a frame so we don't care about invalidating. I tried that but layout test results showed under-paints. Most of such tests are about transformations (perhaps in squashed layers), such as fast/repaint/fixed-scale.html. Only the rects before change of transformations were invalidated. It seems that code path is parallel with the normal RAL path and expects this method to invalidate the current paint rect. Will look into the callers in such cases, but I wonder if there are also other callers expecting the same.
On 2014/07/15 at 01:16:24, wangxianzhu wrote: > https://codereview.chromium.org/395463003/diff/1/Source/core/rendering/Render... > File Source/core/rendering/RenderObject.cpp (right): > > https://codereview.chromium.org/395463003/diff/1/Source/core/rendering/Render... > Source/core/rendering/RenderObject.cpp:1537: invalidatePaintUsingContainer(paintInvalidationContainer, previousPaintInvalidationRect(), InvalidationPaint); > On 2014/07/15 00:58:31, Julien Chaffraix - PST wrote: > > I think we should only invalidate the previousPaintInvalidation here actually. > > The other invalidation above will invalidate at position we don't care about > > (e.g. intermediate forced layout + style recalc between frames). > > > > The reason is simple: all our invalidations should be based on the previous > > stored rectangles (and positions), if we don't have any that means we didn't > > push a frame so we don't care about invalidating. > > I tried that but layout test results showed under-paints. Most of such tests are about transformations (perhaps in squashed layers), such as fast/repaint/fixed-scale.html. Only the rects before change of transformations were invalidated. It seems that code path is parallel with the normal RAL path and expects this method to invalidate the current paint rect. Will look into the callers in such cases, but I wonder if there are also other callers expecting the same. I would like that to be investigated as part of this change as it seems wrong that we invalidate intermediate positions. If it's not too difficult, it should be part of this fix. If not, we can always mark them, file a bug and do the appropriate work.
On 2014/07/15 15:21:47, Julien Chaffraix - PST wrote: > On 2014/07/15 at 01:16:24, wangxianzhu wrote: > > > https://codereview.chromium.org/395463003/diff/1/Source/core/rendering/Render... > > File Source/core/rendering/RenderObject.cpp (right): > > > > > https://codereview.chromium.org/395463003/diff/1/Source/core/rendering/Render... > > Source/core/rendering/RenderObject.cpp:1537: > invalidatePaintUsingContainer(paintInvalidationContainer, > previousPaintInvalidationRect(), InvalidationPaint); > > On 2014/07/15 00:58:31, Julien Chaffraix - PST wrote: > > > I think we should only invalidate the previousPaintInvalidation here > actually. > > > The other invalidation above will invalidate at position we don't care about > > > (e.g. intermediate forced layout + style recalc between frames). > > > > > > The reason is simple: all our invalidations should be based on the previous > > > stored rectangles (and positions), if we don't have any that means we didn't > > > push a frame so we don't care about invalidating. > > > > I tried that but layout test results showed under-paints. Most of such tests > are about transformations (perhaps in squashed layers), such as > fast/repaint/fixed-scale.html. Only the rects before change of transformations > were invalidated. It seems that code path is parallel with the normal RAL path > and expects this method to invalidate the current paint rect. Will look into the > callers in such cases, but I wonder if there are also other callers expecting > the same. > > I would like that to be investigated as part of this change as it seems wrong > that we invalidate intermediate positions. If it's not too difficult, it should > be part of this fix. If not, we can always mark them, file a bug and do the > appropriate work. Filed crbug.com/394004 and added FIXME.
Based on https://code.google.com/p/chromium/issues/detail?id=394050#c3 (from chrishtr@) I think I should create a unified (but not complicated) solution for the issues. Closing this one.
Reopened this CL and uploaded a new patch. Updated description according to the new patch. PTAL.
https://codereview.chromium.org/395463003/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:1537: LayoutRect paintInvalidationRect = isBox() || isSVG() ? previousPaintInvalidationRect() : boundsRectForPaintInvalidation(paintInvalidationContainer); This will still be broken for inlines and such, no? How about a test showing it is, and a fixme. Also, I don't like these magic values to mean "we have a previous paint invalidation rect." I'd rather there at least be a descriptive function. We should also have some asserts around this, like asserting that no unexpected types are having setPreviousPaintInvalidationRect called. https://codereview.chromium.org/395463003/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2074: if (!diff.needsRepaint()) It feels to me like this optimization and the fix above to use previousPaintInvalidationRect should be different patches. One is correctness, one is optimization, no?
https://codereview.chromium.org/395463003/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:1537: LayoutRect paintInvalidationRect = isBox() || isSVG() ? previousPaintInvalidationRect() : boundsRectForPaintInvalidation(paintInvalidationContainer); On 2014/07/15 23:41:29, leviw wrote: > This will still be broken for inlines and such, no? How about a test showing it > is, and a fixme. > Add FIXME with link to crbug.com/394133. > > Also, I don't like these magic values to mean "we have a previous paint > invalidation rect." I'd rather there at least be a descriptive function. We > should also have some asserts around this, like asserting that no unexpected > types are having setPreviousPaintInvalidationRect called. Added bool savesPreviousPaintInvalidationRect() method and ASSERTs in previousPaintInvalildationRect() and setPreviousPaintInvalidationRect(). https://codereview.chromium.org/395463003/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2074: if (!diff.needsRepaint()) On 2014/07/15 23:41:29, leviw wrote: > It feels to me like this optimization and the fix above to use > previousPaintInvalidationRect should be different patches. One is correctness, > one is optimization, no? Done.
https://codereview.chromium.org/395463003/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2075: if (!diff.needsRepaint()) Why the !diff.needsRepaint() conditional? Why don't we also need to invalidate at the new location? Is it really critical to submit a fix for just this? I'd rather see us go directly for setting the dirty bit on paint invalidation for style-only updates and do invalidation in one place. This code looks complicated.
On 2014/07/16 at 00:49:22, chrishtr wrote: > https://codereview.chromium.org/395463003/diff/60001/Source/core/rendering/Re... > File Source/core/rendering/RenderObject.cpp (right): > > https://codereview.chromium.org/395463003/diff/60001/Source/core/rendering/Re... > Source/core/rendering/RenderObject.cpp:2075: if (!diff.needsRepaint()) > Why the !diff.needsRepaint() conditional? > > Why don't we also need to invalidate at the new location? The previous rect should be the last one we put up a frame for. If it's in a new location than that, we haven't put a frame up for it so we don't need to invalidate (conceptually). > > Is it really critical to submit a fix for just this? I'd rather see us go directly for setting the dirty bit on > paint invalidation for style-only updates and do invalidation in one place. This code looks complicated.
https://codereview.chromium.org/395463003/diff/60001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/60001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2075: if (!diff.needsRepaint()) On 2014/07/16 00:49:22, chrishtr wrote: > Why the !diff.needsRepaint() conditional? > '!diff.needsRepaint() && updatedDiff.needsRepaint())' might be possible in theory. As we are changing previousPaintInvalidationRect, if we don't call previousPaintInvalidationRect() here, the previous previousPaintInvalidationRect will be lost and be never invalidated. if diff.needsRepaint(), we have already invalidated the old location in styleWillChange(), so we don't need do it again here. However, based on RenderObject::adjustStyleDifference(), the cases causing '!diff.needsRepaint() && updatedDiff.needsRepaint())' seem all related to layers and we have other code paths for invalidation of them, so I think I can delete these lines (line 2073-2076). > Why don't we also need to invalidate at the new location? > With this patch, the new location is invalidated at line 2080 after previousPaintInvalidationRect is set to the new location at line 2077. In the future, we don't need to invalidate the new location. > Is it really critical to submit a fix for just this? I'd rather see us go > directly for setting the dirty bit on > paint invalidation for style-only updates and do invalidation in one place. This > code looks complicated. I planned multiple incremental small changes, first to resolve the P1 bug with this change. With line 2073-2076 deleted, I think this patch will be small enough. https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... Source/core/rendering/RenderObject.h:992: const LayoutRect& previousPaintInvalidationRect() const { return m_previousPaintInvalidationRect; } For now we can't ASSERT(savesPreviousPaintInvalidationRect()) here because RenderLayerRepainter may fail it.
Uploaded a simplified change. PTAL.
https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (left): https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2058: if (updatedDiff.needsRepaint()) { Just discussed some more with Levi. I think you can go directly to the desired solution of setting bits only and getting rid of the call to paintInvalidationForWholeRenderer() entirely. Just do this: if (updatedDiff.needsRepaint()) setShouldDoFullPaintInvalidationAfterLayout(true); and that's it. This will cause an invalidation to happen after style and/or layout updates have happened.
https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (left): https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2058: if (updatedDiff.needsRepaint()) { On 2014/07/16 17:56:25, chrishtr wrote: > Just discussed some more with Levi. I think you can go directly to the desired > solution of setting bits only and getting rid of the call to > paintInvalidationForWholeRenderer() entirely. Just do this: > > if (updatedDiff.needsRepaint()) > setShouldDoFullPaintInvalidationAfterLayout(true); > > > and that's it. This will cause an invalidation to happen after style and/or > layout updates have happened. OK will do. I have tried that way, but it seems to need a much bigger change than just setting shouldDoFullPaintInvalidationAfterLayout here. Will continue. I think we'll still need paintInvalidationForWholeRenderer() when a RenderObject is removed.
On 2014/07/16 18:05:40, Xianzhu wrote: > https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... > File Source/core/rendering/RenderObject.cpp (left): > > https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... > Source/core/rendering/RenderObject.cpp:2058: if (updatedDiff.needsRepaint()) { > On 2014/07/16 17:56:25, chrishtr wrote: > > Just discussed some more with Levi. I think you can go directly to the desired > > solution of setting bits only and getting rid of the call to > > paintInvalidationForWholeRenderer() entirely. Just do this: > > > > if (updatedDiff.needsRepaint()) > > setShouldDoFullPaintInvalidationAfterLayout(true); > > > > > > and that's it. This will cause an invalidation to happen after style and/or > > layout updates have happened. > > OK will do. I have tried that way, but it seems to need a much bigger change > than just setting shouldDoFullPaintInvalidationAfterLayout here. Will continue. > > I think we'll still need paintInvalidationForWholeRenderer() when a RenderObject > is removed. Agreed, we'll need it for that case. To summarize the findings from the meeting yesterday, we'll need paint invalidation: 1. During the paint invalidation pass after layout & compositing 2. During destructive updates in compositing 3. During certain updates of squashing layers as they move around (though we hope to unify this with #1 4. During destructive updates in the render tree (the one you mentioned) We should get rid of all of the rest of them that don't fall into these four items.
Message was sent while issue was closed.
https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2071: setPreviousPaintInvalidationRect(boundsRectForPaintInvalidation(containerForPaintInvalidation())); Isn't this redundant with the change you're making to paintInvalidationForWholeRenderer()?
Message was sent while issue was closed.
https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2071: setPreviousPaintInvalidationRect(boundsRectForPaintInvalidation(containerForPaintInvalidation())); On 2014/07/16 23:06:48, leviw wrote: > Isn't this redundant with the change you're making to > paintInvalidationForWholeRenderer()? No. Actually this is paired with the change in paintInvalidationForWholeRenderer because here the caller wants to invalidate the new location. With unified invalidation path (I'm working on), the above code won't be needed.
Message was sent while issue was closed.
https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2071: setPreviousPaintInvalidationRect(boundsRectForPaintInvalidation(containerForPaintInvalidation())); If we've ever put up a frame with the thing at the new location, we should have the right location saved, no? Can you help me understand why this case would be different and we'd want the "new" location?
Message was sent while issue was closed.
https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/70001/Source/core/rendering/Re... Source/core/rendering/RenderObject.cpp:2071: setPreviousPaintInvalidationRect(boundsRectForPaintInvalidation(containerForPaintInvalidation())); On 2014/07/22 21:42:29, leviw wrote: > If we've ever put up a frame with the thing at the new location, we should have > the right location saved, no? Can you help me understand why this case would be > different and we'd want the "new" location? This change has been abandoned. The above code was only needed before we go unified invalidation path (https://codereview.chromium.org/398343003). Will continue the new change after landing of depended changes.
Uploaded Patch Set 6 which is based on https://codereview.chromium.org/398343003/ (Unified invalidation). Reopen this CL.
As the unified invalidation change (https://codereview.chromium.org/398343003/) and its depended changes need more time, I uploaded the original patch again to resolve the P1 bug first. Added comment in code to answer leviw@'s question in #24. PTAL.
On 2014/07/23 at 21:41:02, wangxianzhu wrote: > As the unified invalidation change (https://codereview.chromium.org/398343003/) and its depended changes need more time, I uploaded the original patch again to resolve the P1 bug first. > > Added comment in code to answer leviw@'s question in #24. > > PTAL. Why not invalidate both the old and the new rect, like RenderObject::invalidatePaintIfNeeded does? I bet this patch also fixes http://code.google.com/p/chromium/issues/detail?id=321264.
On 2014/07/23 22:19:21, rhogan wrote: > On 2014/07/23 at 21:41:02, wangxianzhu wrote: > > As the unified invalidation change > (https://codereview.chromium.org/398343003/) and its depended changes need more > time, I uploaded the original patch again to resolve the P1 bug first. > > > > Added comment in code to answer leviw@'s question in #24. > > > > PTAL. > > Why not invalidate both the old and the new rect, like > RenderObject::invalidatePaintIfNeeded does? > Actually Patch Set 1 did exactly that which caused many duplicated invalidations in layout tests. We don't want that. In the future, paintInvalidationForWholeRenderer will be only used in very limited cases which all need to invalidate the previous paint rect only. > I bet this patch also fixes > http://code.google.com/p/chromium/issues/detail?id=321264. Yes. Have marked it as a dup.
After chatting with Levi and Julien, we can up with, what we think, is a good way forward. We update paintInvalidationForWhole renderer to invalidate the current position and the previous position (assuming they're different). Then, inside the invalidateTree walk, when we get down into the RenderInline's, if they have a layer, we store the bounds into the previousPaintInvalidationRect as we do with other types of renderers. This means, in the case where they're moved but not invalidated and then removed we'll be able to depend on the previousPaintInvalidationRect. The reason we gate on the inline having a layer is that, in the case when the inline doesn't have a layer, it should be correctly invalidated by the blocks bounding box. While this may trigger over-invalidations for now, but it fixes the outstanding regressions, and as we move forward and start removing calls to paintInvaldiationForWholeRenderer those extra invalidations should start to go away. What do you think?
On 2014/07/24 19:43:42, dsinclair wrote: > After chatting with Levi and Julien, we can up with, what we think, is a good > way forward. > > We update paintInvalidationForWhole renderer to invalidate the current position > and the previous position (assuming they're different). Then, inside the > invalidateTree walk, when we get down into the RenderInline's, if they have a > layer, we store the bounds into the previousPaintInvalidationRect as we do with > other types of renderers. > > This means, in the case where they're moved but not invalidated and then removed > we'll be able to depend on the previousPaintInvalidationRect. > > The reason we gate on the inline having a layer is that, in the case when the > inline doesn't have a layer, it should be correctly invalidated by the blocks > bounding box. > > While this may trigger over-invalidations for now, but it fixes the outstanding > regressions, and as we move forward and start removing calls to > paintInvaldiationForWholeRenderer those extra invalidations should start to go > away. > > What do you think? I think this is reasonable. Will upload a patch soon.
PTAL. The patch doesn't contain special case for layered inline. Based on my test, it seems not needed - In my test, boundsRectForPaintInvalidation() for RenderInline is always empty (while RenderText has non-empty bounds); - When a inline is removed, the container always full repaint no matter whether the inline has layer. Covered by new layout tests.
A couple of nits, but lgtm. https://codereview.chromium.org/395463003/diff/150001/LayoutTests/fast/repain... File LayoutTests/fast/repaint/remove-block-after-layout.html (right): https://codereview.chromium.org/395463003/diff/150001/LayoutTests/fast/repain... LayoutTests/fast/repaint/remove-block-after-layout.html:10: runRepaintTest(); onload = runRepaintTest? https://codereview.chromium.org/395463003/diff/150001/LayoutTests/fast/repain... File LayoutTests/fast/repaint/remove-inline-after-layout.html (right): https://codereview.chromium.org/395463003/diff/150001/LayoutTests/fast/repain... LayoutTests/fast/repaint/remove-inline-after-layout.html:10: runRepaintTest(); onload = runRepaintTest; https://codereview.chromium.org/395463003/diff/150001/Source/core/rendering/R... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:1535: // both the previous and current paint rects to meet the expectations of some callers in some cases: Can we add a crbug for this and block it on the two bugs below?
https://codereview.chromium.org/395463003/diff/150001/LayoutTests/fast/repain... File LayoutTests/fast/repaint/remove-block-after-layout.html (right): https://codereview.chromium.org/395463003/diff/150001/LayoutTests/fast/repain... LayoutTests/fast/repaint/remove-block-after-layout.html:10: runRepaintTest(); On 2014/07/25 14:16:31, dsinclair wrote: > onload = runRepaintTest? I thought this worked and is simpler, but noticed it may be a source of flakiness (in case of slow load). Done. https://codereview.chromium.org/395463003/diff/150001/LayoutTests/fast/repain... File LayoutTests/fast/repaint/remove-inline-after-layout.html (right): https://codereview.chromium.org/395463003/diff/150001/LayoutTests/fast/repain... LayoutTests/fast/repaint/remove-inline-after-layout.html:10: runRepaintTest(); On 2014/07/25 14:16:31, dsinclair wrote: > onload = runRepaintTest; Done. https://codereview.chromium.org/395463003/diff/150001/Source/core/rendering/R... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:1535: // both the previous and current paint rects to meet the expectations of some callers in some cases: On 2014/07/25 14:16:31, dsinclair wrote: > Can we add a crbug for this and block it on the two bugs below? Done.
lgtm https://codereview.chromium.org/395463003/diff/170001/LayoutTests/fast/repain... File LayoutTests/fast/repaint/remove-block-after-layout.html (right): https://codereview.chromium.org/395463003/diff/170001/LayoutTests/fast/repain... LayoutTests/fast/repaint/remove-block-after-layout.html:11: </script> Please let's add a description of what we are testing for all tests. https://codereview.chromium.org/395463003/diff/170001/Source/core/rendering/R... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:1534: // FIXME: We should invalidate previousPaintInvalidationRect, but for now we invalidate both the previous s/invalidate/invalidate only/?
https://codereview.chromium.org/395463003/diff/170001/LayoutTests/fast/repain... File LayoutTests/fast/repaint/remove-block-after-layout.html (right): https://codereview.chromium.org/395463003/diff/170001/LayoutTests/fast/repain... LayoutTests/fast/repaint/remove-block-after-layout.html:11: </script> On 2014/07/28 16:57:05, Julien Chaffraix - PST wrote: > Please let's add a description of what we are testing for all tests. Done. https://codereview.chromium.org/395463003/diff/170001/Source/core/rendering/R... File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/395463003/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderObject.cpp:1534: // FIXME: We should invalidate previousPaintInvalidationRect, but for now we invalidate both the previous On 2014/07/28 16:57:05, Julien Chaffraix - PST wrote: > s/invalidate/invalidate only/? Done.
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/395463003/210001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19271) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/395463003/210001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19271) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/395463003/210001
Message was sent while issue was closed.
Committed patchset #11 manually as r179059 (presubmit successful). |
