Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184043033:
Remove obsolete FIXME that no longer applies for blink rendering architecture (Closed)
|
Created:
6 years, 9 months ago by atreat Modified:
6 years, 9 months ago CC:
bemjb+rendering_chromium.org, blink-reviews, dsinclair, eae+blinkwatch, enne (OOO), jamesr, Julien - ping for review, leviw_travelin_and_unemployed, leviw+renderwatch, pdr., zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionRemove obsolete FIXME that no longer applies for blink rendering architecture
After studying this FIXME and the motivation behind it I attempted to fix it by
clipping to the viewport as suggested. However, it turns out this FIXME is no
longer applicable because the architecture of Blink's rendering routine requires
all Blink to inform the compositor of all invalidations not just the visible ones.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169065
Patch Set 1 #Patch Set 2 : Add new expectations for mac and windows #
Total comments: 2
Patch Set 3 : New strategy to just delete the FIXME #Messages
Total messages: 18 (0 generated)
Please have a look. I believe the windows failures are a problem with the bots not the patch. They are having trouble today with disk space apparently.
I'm not sure this is correct, how do we deal with the prepaint window? To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/03/06 21:47:49, esprehn wrote: > I'm not sure this is correct, how do we deal with the prepaint window? I'm not sure I understand the question. The prepaint window?
On 2014/03/06 21:52:47, atreat wrote: > On 2014/03/06 21:47:49, esprehn wrote: > > I'm not sure this is correct, how do we deal with the prepaint window? > > I'm not sure I understand the question. The prepaint window? You can't just clip all invalidations to things inside the viewport. We paint lots of things outside the viewport so you can scroll quickly. The prepaint window is a rectangle that the compositor decides it wants to paint. So the viewport might be 800x600, but the document is actually 10000px long, and we'll paint 800x4000px of it so you can scroll faster without painting as you scroll.
On 2014/03/06 23:09:14, esprehn wrote: > On 2014/03/06 21:52:47, atreat wrote: > > On 2014/03/06 21:47:49, esprehn wrote: > > > I'm not sure this is correct, how do we deal with the prepaint window? > > > > I'm not sure I understand the question. The prepaint window? > > You can't just clip all invalidations to things inside the viewport. We paint > lots of things outside the viewport so you can scroll quickly. The prepaint > window is a rectangle that the compositor decides it wants to paint. So the > viewport might be 800x600, but the document is actually 10000px long, and we'll > paint 800x4000px of it so you can scroll faster without painting as you scroll. This will only clip when the RenderView is the renderContainer. See the first comment in RenderBox::computeRectForRepaint. This does not break any pixel tests whatsoever.
On 2014/03/07 15:26:33, atreat wrote: > On 2014/03/06 23:09:14, esprehn wrote: > > On 2014/03/06 21:52:47, atreat wrote: > > > On 2014/03/06 21:47:49, esprehn wrote: > > > > I'm not sure this is correct, how do we deal with the prepaint window? > > > > > > I'm not sure I understand the question. The prepaint window? > > > > You can't just clip all invalidations to things inside the viewport. We paint > > lots of things outside the viewport so you can scroll quickly. The prepaint > > window is a rectangle that the compositor decides it wants to paint. So the > > viewport might be 800x600, but the document is actually 10000px long, and > we'll > > paint 800x4000px of it so you can scroll faster without painting as you > scroll. > > This will only clip when the RenderView is the renderContainer. See the first > comment > in RenderBox::computeRectForRepaint. This does not break any pixel tests > whatsoever. Hello? Anyone? :)
lgtm It's not clear to me that this is correct, but I anticipate that if it's wrong, we'll notice very quickly. :) https://codereview.chromium.org/184043033/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/184043033/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderBox.cpp:2045: if (layoutState->m_clipped && !rect.isEmpty()) I'm surprised this is necessary? Doesn't rect.intersect already early-out when one of them is empty?
On 2014/03/11 18:13:14, atreat wrote: > On 2014/03/07 15:26:33, atreat wrote: > > On 2014/03/06 23:09:14, esprehn wrote: > > > On 2014/03/06 21:52:47, atreat wrote: > > > > On 2014/03/06 21:47:49, esprehn wrote: > > > > > I'm not sure this is correct, how do we deal with the prepaint window? > > > > > > > > I'm not sure I understand the question. The prepaint window? > > > > > > You can't just clip all invalidations to things inside the viewport. We > paint > > > lots of things outside the viewport so you can scroll quickly. The prepaint > > > window is a rectangle that the compositor decides it wants to paint. So the > > > viewport might be 800x600, but the document is actually 10000px long, and > > we'll > > > paint 800x4000px of it so you can scroll faster without painting as you > > scroll. > > > > This will only clip when the RenderView is the renderContainer. See the first > > comment > > in RenderBox::computeRectForRepaint. This does not break any pixel tests > > whatsoever. > > Hello? Anyone? :) Pixel tests aren't testing the prepaint window. You can't assume that only things "visible" are things that are inside the viewport bounds. We regularly paint stuff that's outside it.
This patch isn't correct, can you explain in the patch description what you're trying to do better? not lgtm for now. This patch doesn't look right to me.
https://codereview.chromium.org/184043033/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderView.cpp (left): https://codereview.chromium.org/184043033/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderView.cpp:196: // FIXME: May be better to push a clip and avoid issuing offscreen repaints. Based on what I now understand, we should just remove this FIXME. We don't control our viewport, the compositor does. We paint whatever rects it tells us to paint, and do not get to implicitly clip our painting based on what we think the viewport is. LayoutState's clipped state is used for computing repaint rects, which in turn are used for layer boundaries. We are able to pre-paint the root layer for accelerated scrolling, but we do not yet prepaint any arbitrary overflow: scroll div, in part due to this clipped state not being aware of prepainting and the compositor controlling clipping of our layers.
Please have a look.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/adam.treat@samsung.com/184043033/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_blink_compile
The CQ bit was checked by esprehn@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/adam.treat@samsung.com/184043033/40001
Message was sent while issue was closed.
Change committed as 169065 |
