Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
212553004:
[repaint-after-layout] Skip invalidations if our RenderView did a full invalidation. (Closed)
|
Created:
6 years, 9 months ago by dsinclair Modified:
6 years, 8 months ago CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
Description[repaint-after-layout] Skip invalidations if our RenderView did a full invalidation.
This CL updates FrameView::repaintTree to skip issuing invalidations for any
RenderObject if they don't have a layer and their RenderView did a full
invalidation. This seems to be closer to the behaviour that the system exhibits
with repaint-after-layout disabled.
BUG=320139
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170489
Patch Set 1 #Patch Set 2 : Rebase to master #
Total comments: 4
Patch Set 3 : Rebase to master #Patch Set 4 : #Patch Set 5 : Rebase to master. #
Total comments: 4
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 6
Patch Set 8 : #Messages
Total messages: 13 (0 generated)
PTAL.
https://codereview.chromium.org/212553004/diff/10001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/212553004/diff/10001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1046: if (!renderer->hasLayer() && renderer != root && (renderer->view() == root) && renderViewDidFullInvalidation) { I don't think we should check hasLayer() here as it's conceptually wrong to treat renderers with a layer differently (especially in the world where repaint-after-layout has super-seeded RenderLayerRepainter). https://codereview.chromium.org/212553004/diff/10001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1061: if (renderer == root && root->isRenderView()) Note that this check will be run for each renderer when it's only needed for the view. This obscures the intent pretty badly. I would rather have that checked above with a fast path.
https://codereview.chromium.org/212553004/diff/10001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/212553004/diff/10001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1046: if (!renderer->hasLayer() && renderer != root && (renderer->view() == root) && renderViewDidFullInvalidation) { On 2014/03/27 22:37:26, Julien Chaffraix - PST wrote: > I don't think we should check hasLayer() here as it's conceptually wrong to > treat renderers with a layer differently (especially in the world where > repaint-after-layout has super-seeded RenderLayerRepainter). Done. https://codereview.chromium.org/212553004/diff/10001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1061: if (renderer == root && root->isRenderView()) On 2014/03/27 22:37:26, Julien Chaffraix - PST wrote: > Note that this check will be run for each renderer when it's only needed for the > view. This obscures the intent pretty badly. I would rather have that checked > above with a fast path. Done.
ping
https://codereview.chromium.org/212553004/diff/70001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/212553004/diff/70001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1047: root->updateShouldDoFullRepaintAfterLayout(); Are you sure this code is needed? (Document won't be marked for layout itself -except for the first layout which shouldn't generate any invalidation anyway - as its style can't be changed programmatically) https://codereview.chromium.org/212553004/diff/70001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1057: } I would just pull this code out of the previous loop as: - you can think of this as a fast path. - you can simplify the code a lot if you do: for (RenderObject* renderer = root->firstChild(); renderer; renderer->nextInPreOrder(root->firstChild()) renderer->clearRepaintState();
https://codereview.chromium.org/212553004/diff/70001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/212553004/diff/70001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1047: root->updateShouldDoFullRepaintAfterLayout(); On 2014/03/31 17:31:26, Julien Chaffraix - PST wrote: > Are you sure this code is needed? (Document won't be marked for layout itself > -except for the first layout which shouldn't generate any invalidation anyway - > as its style can't be changed programmatically) Done. https://codereview.chromium.org/212553004/diff/70001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1057: } On 2014/03/31 17:31:26, Julien Chaffraix - PST wrote: > I would just pull this code out of the previous loop as: > - you can think of this as a fast path. > - you can simplify the code a lot if you do: > > for (RenderObject* renderer = root->firstChild(); renderer; > renderer->nextInPreOrder(root->firstChild()) > renderer->clearRepaintState(); Done.
https://codereview.chromium.org/212553004/diff/90001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/212553004/diff/90001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1049: if (root->isRenderView() && root->shouldDoFullRepaintAfterLayout()) { I think we could simplify even further here as we do the following: - if m_doFullRepaint == true, mark RenderView as needing full repaint. - check shouldDoFullRepaintAfterLayout() here. It seems we could just remove the first part of the logic and just check m_doFullRepaint here. Also it seems like if we are doing a full repaint, we don't care what the root for layout is. We would just have to be careful to repaint the RenderView. https://codereview.chromium.org/212553004/diff/90001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1061: renderer->updateShouldDoFullRepaintAfterLayout(); I find that this function obfuscate the code as 'update' is a generic term. I would just leave the code inlined (as I couldn't think of a better name for it).
https://codereview.chromium.org/212553004/diff/90001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/212553004/diff/90001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1049: if (root->isRenderView() && root->shouldDoFullRepaintAfterLayout()) { On 2014/03/31 20:00:56, Julien Chaffraix - PST wrote: > I think we could simplify even further here as we do the following: > - if m_doFullRepaint == true, mark RenderView as needing full repaint. > - check shouldDoFullRepaintAfterLayout() here. > > It seems we could just remove the first part of the logic and just check > m_doFullRepaint here. > > Also it seems like if we are doing a full repaint, we don't care what the root > for layout is. We would just have to be careful to repaint the RenderView. Done. https://codereview.chromium.org/212553004/diff/90001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1061: renderer->updateShouldDoFullRepaintAfterLayout(); On 2014/03/31 20:00:56, Julien Chaffraix - PST wrote: > I find that this function obfuscate the code as 'update' is a generic term. I > would just leave the code inlined (as I couldn't think of a better name for it). Done.
lgtm https://codereview.chromium.org/212553004/diff/110001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/212553004/diff/110001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1048: RenderObject* view = renderView(); RenderView* view = renderView()!!! https://codereview.chromium.org/212553004/diff/110001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1052: for (RenderObject* renderer = view; renderer; renderer = renderer->nextInPreOrder()) { Shouldn't this be done on the |root|-rooted subtree and not the whole render tree? (which would eliminate the need for |view|) https://codereview.chromium.org/212553004/diff/110001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1057: How about adding an ASSERT here that we don't need m_doFullRepaint as it would mean unneeded invalidations?
https://codereview.chromium.org/212553004/diff/110001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/212553004/diff/110001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1048: RenderObject* view = renderView(); On 2014/03/31 20:28:35, Julien Chaffraix - PST wrote: > RenderView* view = renderView()!!! Done. https://codereview.chromium.org/212553004/diff/110001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1052: for (RenderObject* renderer = view; renderer; renderer = renderer->nextInPreOrder()) { On 2014/03/31 20:28:35, Julien Chaffraix - PST wrote: > Shouldn't this be done on the |root|-rooted subtree and not the whole render > tree? (which would eliminate the need for |view|) Done. Keeping view as it's used 4 times on the repaintAfterLayoutIfNeeded call. https://codereview.chromium.org/212553004/diff/110001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1057: On 2014/03/31 20:28:35, Julien Chaffraix - PST wrote: > How about adding an ASSERT here that we don't need m_doFullRepaint as it would > mean unneeded invalidations? 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/212553004/130001
Message was sent while issue was closed.
Change committed as 170489 |
