Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1791543005:
InPrePaint document state and PrePaintTreeWalk class (Closed)
|
Created:
4 years, 9 months ago by Xianzhu Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-wtf_chromium.org, chromium-reviews, dglazkov+blink, dshwang, krit, eae+blinkwatch, f(malita), fs, gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, Mikhail, pdr+svgwatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, sof, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInPrePaint document state and PrePaintTreeWalk class
- InUpdatePaintProperty document state is renamed to InPrePaint;
- The original PaintPropertyTreeBuilder class is split into PrePaintTreeWalk
which walks the tree and the new PaintPropertyTreeBuilder which build the
property tree.
This is preparation for a combined treewalk for both
paint property tree generation and paint invalidation.
For more information, see:
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-doA/view
BUG=591199
Committed: https://crrev.com/c3932e54885cbcc80b011bda5b45c592b22f1ff0
Cr-Commit-Position: refs/heads/master@{#397280}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : Remove unnecessary includes #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Revert unnecessary changes in LayoutObject.h #
Total comments: 7
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Dependent Patchsets: Messages
Total messages: 41 (12 generated)
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org, trchen@chromium.org
This is separated from https://codereview.chromium.org/1774193002/.
pdr@chromium.org changed reviewers: + jbroman@chromium.org
Description was changed from ========== Rename PaintPropertyTreeBuilder to PrePaintTreeWalk We'll integerate paint invalidation into the tree walk. BUG=591199 ========== to ========== Rename PaintPropertyTreeBuilder to PrePaintTreeWalk This is a renaming patch in preparation for a combined treewalk for both paint property tree generation and paint invalidation. For more information, see: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... BUG=591199 ==========
LGTM but please give trchen and jbroman a chance to review as well. @trchen and @jbroman, are you ok with this naming too? https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp (right): https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp:223: case PrePaintTreeWalkClean: Nit: PrePaintTreeWalkFinished or PrePaintTreeWalkComplete? https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentLifecycle.h (right): https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentLifecycle.h:71: InPrePaintTreeWalk, Nit: Can you update the comment in LayoutObject.h above objectPaintProperties too? https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h (right): https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h:18: // It expects to be invoked after layout clean, i.e. during InPaintPropertyUpdate phase. Nit: this comment should be updated to not use InPaintPropertyUpdate anymore.
High-level thought: even if we do want these happening in the same walk, do we want to entangle them by having them be in the same class? Or would it be better to keep PaintPropertyTreeBuilder in some capacity, and have it be called during the walk to build paint properties for each node the walk reaches? https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentLifecycle.h (right): https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentLifecycle.h:71: InPrePaintTreeWalk, On 2016/03/12 at 04:04:24, pdr wrote: > Nit: Can you update the comment in LayoutObject.h above objectPaintProperties too? It becomes less obvious what this step is, now that it only describes when it happens, rather than what it does. Something like "If RuntimeEnabledFeatures::slimmingPaintV2Enabled, paint property trees are build in this step. If (other condition), paint invalidation happens during this step.".
On 2016/03/14 15:05:15, jbroman wrote: > High-level thought: even if we do want these happening in the same walk, do we > want to entangle them by having them be in the same class? > > Or would it be better to keep PaintPropertyTreeBuilder in some capacity, and > have it be called during the walk to build paint properties for each node the > walk reaches? Good idea. Done.
https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp (right): https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp:223: case PrePaintTreeWalkClean: On 2016/03/12 04:04:24, pdr wrote: > Nit: PrePaintTreeWalkFinished or PrePaintTreeWalkComplete? I removed "TreeWalk" from the new state names. How about "PrePaintClean"? https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentLifecycle.h (right): https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentLifecycle.h:71: InPrePaintTreeWalk, On 2016/03/14 15:05:15, jbroman wrote: > On 2016/03/12 at 04:04:24, pdr wrote: > > Nit: Can you update the comment in LayoutObject.h above objectPaintProperties > too? > > It becomes less obvious what this step is, now that it only describes when it > happens, rather than what it does. > > Something like "If RuntimeEnabledFeatures::slimmingPaintV2Enabled, paint > property trees are build in this step. If (other condition), paint invalidation > happens during this step.". Changed the state name "InPrePaintTreeWalk" to "InPrePaint" and added comment: // In InPrePaint step, any data needed for painting are prepared. // When RuntimeEnabledFeatures::slimmingPaintV2Enabled, paint property trees are built. // Otherwise these steps are not applicable. Will add comment about paint invalidation when it is added in this step. https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h (right): https://codereview.chromium.org/1791543005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h:18: // It expects to be invoked after layout clean, i.e. during InPaintPropertyUpdate phase. On 2016/03/12 04:04:25, pdr wrote: > Nit: this comment should be updated to not use InPaintPropertyUpdate anymore. Done.
On 2016/03/14 18:55:57, Xianzhu wrote: > On 2016/03/14 15:05:15, jbroman wrote: > > High-level thought: even if we do want these happening in the same walk, do we > > want to entangle them by having them be in the same class? > > > > Or would it be better to keep PaintPropertyTreeBuilder in some capacity, and > > have it be called during the walk to build paint properties for each node the > > walk reaches? > > Good idea. Done. Actually I'm not pretty sure it is a good idea. Because during property node generation, the local context will be adjusted multiple times. Going from parent's scrolled padding box --adjust by box layout location--> layout border box --transform--> paint border box --apply clip--> paint padding box --apply scroll and perspective--> scrolled padding box I suspect paint invalidation may need to use different contexts for each part of layout object. Right now we don't do it, but I can see an advantage doing so. BTW, it is orthogonal to this CL, but I'm planning to add a cached local-screen matrix on transform nodes, for paint layer cull rect adjustment. I think it will be useful for paint offset computation for invalidation too.
On 2016/03/14 19:13:34, trchen wrote: > On 2016/03/14 18:55:57, Xianzhu wrote: > > On 2016/03/14 15:05:15, jbroman wrote: > > > High-level thought: even if we do want these happening in the same walk, do > we > > > want to entangle them by having them be in the same class? > > > > > > Or would it be better to keep PaintPropertyTreeBuilder in some capacity, and > > > have it be called during the walk to build paint properties for each node > the > > > walk reaches? > > > > Good idea. Done. > > Actually I'm not pretty sure it is a good idea. Because during property node > generation, the local context will be adjusted multiple times. Going from > > parent's scrolled padding box --adjust by box layout location--> > layout border box --transform--> > paint border box --apply clip--> > paint padding box --apply scroll and perspective--> > scrolled padding box > > I suspect paint invalidation may need to use different contexts for each part of > layout object. Right now we don't do it, but I can see an advantage doing so. I'm rebasing https://codereview.chromium.org/1774193002/ onto this CL to see if paint invalidation could be happy with this CL. My current plan is: - add PaintInvalidator class having a single method: invalidatePaintIfNeeded(const LayoutObject&, const PaintPropertyTreeBuilderContext&, PaintInvalidationContext&) which requires the result of paint property tree builder for the object; - if paint invalidation needs more data from the tree builder (e.g. intermediate state of context updates), the tree builder can save such intermediate states into the context. Wdyt? > BTW, it is orthogonal to this CL, but I'm planning to add a cached local-screen > matrix on transform nodes, for paint layer cull rect adjustment. I think it will > be useful for paint offset computation for invalidation too. That will be awesome. Thanks.
Description was changed from ========== Rename PaintPropertyTreeBuilder to PrePaintTreeWalk This is a renaming patch in preparation for a combined treewalk for both paint property tree generation and paint invalidation. For more information, see: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... BUG=591199 ========== to ========== InPrePaint document state and PrePaintTreeWalk class - InUpdatePaintProperty document state is renamed to InPrePaint; - The original PaintPropertyTreeBuilder class is split into PrePaintTreeWalk which walks the tree and the new PaintPropertyTreeBuilder which build the property tree. This is preparation for a combined treewalk for both paint property tree generation and paint invalidation. For more information, see: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... BUG=591199 ==========
Chris et al:
After trying different ways, I feel it complex to integrate paint invalidation
into paint property tree walking for SPv1 because of paint invalidation
containers. For SPv1 we need to track paint offsets from paint invalidation
containers which are different from the offsets in
PaintPropertyTreeBuilderContext which are from transform containers. Paint
invalidation container will also cause trouble to handle transform, clipping,
etc. in case that they are applied crossing paint invalidation containers.
However, I learned a lot from the work and understand the problems of current
paint invalidation much better.
I would like the following plan:
- Fix problem of the current paint invalidation, using the knowledge learnt from
recent works:
* Walk tree in DOM order instead of the current containing-block order and
track paint offsets for normal objects, fixed-position and absolute-position
separately;
* Track paint invalidation container for normal objects and stacked objects
separately;
* In the following cases, fallback to ForceHorriblySlowRectMapping:
. transform and/or clipping are applied crossing paint invalidation
container;
. paint offset can't be calculated from paint invalidation container (e.g.
when there is transform under paint invalidation container,
out-of-flow-positioned object's containing block is above paint invalidaition
container, etc.)
- Integrate paint invalidation into PrePaintTreeWalk for SPv2 only.
I think the above plan could achieve the goals we planned for SPv1 paint
invalidation while avoid extra complexity. Integration of paint invalidation for
SPv2-only would be clean, free of SPv1 code.
Wdyt?
On 2016/03/14 23:34:36, Xianzhu wrote: > Chris et al: > > After trying different ways, I feel it complex to integrate paint invalidation > into paint property tree walking for SPv1 because of paint invalidation > containers. For SPv1 we need to track paint offsets from paint invalidation > containers which are different from the offsets in > PaintPropertyTreeBuilderContext which are from transform containers. Paint > invalidation container will also cause trouble to handle transform, clipping, > etc. in case that they are applied crossing paint invalidation containers. Fear not! I actually tried to keep paint offset as consistent as possible between SPv1 and SPv2. That's why we had the layer->paintsWithTransform dependency in createPaintOffsetTranslationIfNeeded(). For the cases that are not consistent, most it is due to compositing decision. Paints with composited layer effectively reset paint offset, i.e. paintWithTransform. I can fix that for you! > However, I learned a lot from the work and understand the problems of current > paint invalidation much better. > > I would like the following plan: > > - Fix problem of the current paint invalidation, using the knowledge learnt from > recent works: > > * Walk tree in DOM order instead of the current containing-block order and > track paint offsets for normal objects, fixed-position and absolute-position > separately; > * Track paint invalidation container for normal objects and stacked objects > separately; > * In the following cases, fallback to ForceHorriblySlowRectMapping: > . transform and/or clipping are applied crossing paint invalidation > container; > . paint offset can't be calculated from paint invalidation container (e.g. > when there is transform under paint invalidation container, > out-of-flow-positioned object's containing block is above paint invalidaition > container, etc.) > > - Integrate paint invalidation into PrePaintTreeWalk for SPv2 only. > > I think the above plan could achieve the goals we planned for SPv1 paint > invalidation while avoid extra complexity. Integration of paint invalidation for > SPv2-only would be clean, free of SPv1 code. > > Wdyt? From project management perspective I think it makes sense not to invest too much into old-world compositing. Personally I still hoped to see those really difficult invalidation bug getting fixed and the elimination of ForceHorriblySlow. :(
On 2016/03/15 00:30:25, trchen wrote: > On 2016/03/14 23:34:36, Xianzhu wrote: > > Chris et al: > > > > After trying different ways, I feel it complex to integrate paint invalidation > > into paint property tree walking for SPv1 because of paint invalidation > > containers. For SPv1 we need to track paint offsets from paint invalidation > > containers which are different from the offsets in > > PaintPropertyTreeBuilderContext which are from transform containers. Paint > > invalidation container will also cause trouble to handle transform, clipping, > > etc. in case that they are applied crossing paint invalidation containers. > > Fear not! I actually tried to keep paint offset as consistent as possible > between SPv1 and SPv2. That's why we had the layer->paintsWithTransform > dependency in createPaintOffsetTranslationIfNeeded(). > For the cases that are not consistent, most it is due to compositing decision. > Paints with composited layer effectively reset paint offset, i.e. > paintWithTransform. I can fix that for you! > I'm not doubting we can implement a fully correct paint invalidation for SPv1 in PaintPropertyTreeBuilder, but I'm considering the complexities. For simple cases, resetting paint offset to zero is easy and has been done in https://codereview.chromium.org/1774193002/#ps180001, but the complex cases (e.g. out-of-flow-positioned object with a containing block above the paint invalidation container) will bring much complexity to the code which is not applicable to SPv2 and will be removed in the future. > > However, I learned a lot from the work and understand the problems of current > > paint invalidation much better. > > > > I would like the following plan: > > > > - Fix problem of the current paint invalidation, using the knowledge learnt > from > > recent works: > > > > * Walk tree in DOM order instead of the current containing-block order and > > track paint offsets for normal objects, fixed-position and absolute-position > > separately; > > * Track paint invalidation container for normal objects and stacked objects > > separately; > > * In the following cases, fallback to ForceHorriblySlowRectMapping: > > . transform and/or clipping are applied crossing paint invalidation > > container; > > . paint offset can't be calculated from paint invalidation container (e.g. > > when there is transform under paint invalidation container, > > out-of-flow-positioned object's containing block is above paint invalidaition > > container, etc.) > > > > - Integrate paint invalidation into PrePaintTreeWalk for SPv2 only. > > > > I think the above plan could achieve the goals we planned for SPv1 paint > > invalidation while avoid extra complexity. Integration of paint invalidation > for > > SPv2-only would be clean, free of SPv1 code. > > > > Wdyt? > > From project management perspective I think it makes sense not to invest too > much into old-world compositing. Personally I still hoped to see those really > difficult invalidation bug getting fixed and the elimination of > ForceHorriblySlow. :( My plan above can fix many ForceHorriblySlow cases for SPv1, without polluting SPv2 code. For the remaining ones perhaps we can just live with them until they disappear [1] or are fixed [2] with SPv2. [1] cases that will disappear with SPv2: - out-of-flow-positioned object's containing block is above the paint invalidation container; - transform/clip is applied crossing paint invalidation container; [2] cases that will be fixed in SPv2 with simpler ways: - transform is applied under paint invalidation container
Hi Xianhzu, Sorry for missing your note until just now. Some comments below to understand the shape of the problem. On 2016/03/14 at 23:34:36, wangxianzhu wrote: > Chris et al: > > After trying different ways, I feel it complex to integrate paint invalidation into paint property tree walking for SPv1 because of paint invalidation containers. For SPv1 we need to track paint offsets from paint invalidation containers which are different from the offsets in PaintPropertyTreeBuilderContext which are from transform containers. Paint invalidation container will also cause trouble to handle transform, clipping, etc. in case that they are applied crossing paint invalidation containers. > > However, I learned a lot from the work and understand the problems of current paint invalidation much better. > > I would like the following plan: > > - Fix problem of the current paint invalidation, using the knowledge learnt from recent works: > > * Walk tree in DOM order instead of the current containing-block order and track paint offsets for normal objects, fixed-position and absolute-position separately; This is already needed for the existing property tree walk, right? Put another way, what is unique about paintOffset for paint invalidation that doesn't apply for property trees? > * Track paint invalidation container for normal objects and stacked objects separately; > * In the following cases, fallback to ForceHorriblySlowRectMapping: > . transform and/or clipping are applied crossing paint invalidation container; > . paint offset can't be calculated from paint invalidation container (e.g. when there is transform under paint invalidation container, out-of-flow-positioned object's containing block is above paint invalidaition container, etc.) Following up on my comment above: if it is correct, then it seems the problem is just computing the paint invalidation container correctly and efficiently, and in a way that doesn't pollute the code too much. Is this really hard with the existing approach? Maybe a concrete examples would help me to understand better. > > - Integrate paint invalidation into PrePaintTreeWalk for SPv2 only. > > I think the above plan could achieve the goals we planned for SPv1 paint invalidation while avoid extra complexity. Integration of paint invalidation for SPv2-only would be clean, free of SPv1 code. > > Wdyt?
On 2016/03/15 23:49:33, chrishtr wrote:
> Hi Xianhzu,
>
> Sorry for missing your note until just now.
>
> Some comments below to understand the shape of the problem.
>
> On 2016/03/14 at 23:34:36, wangxianzhu wrote:
> > Chris et al:
> >
> > After trying different ways, I feel it complex to integrate paint
invalidation
> into paint property tree walking for SPv1 because of paint invalidation
> containers. For SPv1 we need to track paint offsets from paint invalidation
> containers which are different from the offsets in
> PaintPropertyTreeBuilderContext which are from transform containers. Paint
> invalidation container will also cause trouble to handle transform, clipping,
> etc. in case that they are applied crossing paint invalidation containers.
> >
> > However, I learned a lot from the work and understand the problems of
current
> paint invalidation much better.
> >
> > I would like the following plan:
> >
> > - Fix problem of the current paint invalidation, using the knowledge learnt
> from recent works:
> >
> > * Walk tree in DOM order instead of the current containing-block order and
> track paint offsets for normal objects, fixed-position and absolute-position
> separately;
>
> This is already needed for the existing property tree walk, right? Put another
> way, what is unique about paintOffset for paint invalidation that doesn't
apply
> for property trees?
First Question: Yes
Second Question: No for SPv1, but for SPv1, paint invalidation container brings
a lot of complexities.
>
>
> > * Track paint invalidation container for normal objects and stacked
objects
> separately;
> > * In the following cases, fallback to ForceHorriblySlowRectMapping:
> > . transform and/or clipping are applied crossing paint invalidation
> container;
> > . paint offset can't be calculated from paint invalidation container
(e.g.
> when there is transform under paint invalidation container,
> out-of-flow-positioned object's containing block is above paint invalidaition
> container, etc.)
>
> Following up on my comment above: if it is correct, then it seems the problem
is
> just computing the paint invalidation container correctly and efficiently, and
> in a way that doesn't pollute the
> code too much. Is this really hard with the existing approach? Maybe a
concrete
> examples would help me to understand better.
Computing the correct paint invalidation container is easy, as the uploaded
patch set has done that. The problem is the complexity of paint offsets and
other things needed for fast rect mapping of paint invalidation rects.
Consider the following cases:
1) paintInvalidationContainer
transform
current object
2) absolute-position's containing block
paintInvalidationContainer
current object (absolute-position)
3) absolute-position's containing block
paintInvalidationContainerForAbsolutePosition
paintInvaidationContainerForNormalObjects (and not a containing block of
absolute-position)
current object (absolute-position)
For each case, we need information in addition to paintOffset to map a rect from
the current object space to paintInvalidationContainer space.
1) We need to track transform from the paintInvalidationContainer and the paint
offset from the transform node, to the current object. This looks not very
difficult, but will look complex if we look at the next two cases;
2) The absolute-position object's position is not determined by paintOffset of
PaintInvalidationState, but by something above the paintInvalidationContainer.
That means if we want to be faster than HorriblySlow, we must track and cache
the mapping from the containing block to the current object and the mapping from
the containing block to the paintInvalidationContainer;
3) This case shows that we need to track two paintInvalidationContainers, plus
the above different mappings, there will be many combinations of different cases
and the things we need to track and cache.
To reduce the number of things to track, I plan to still use HorriblySlow for
some difficult cases (e.g. with transforms), and fix common cases by track more
things in the current PaintInvalidationState.
Among the above cases, only 1) is applicable to SPv2. If we let
PaintPropertyTreeBuilder work for SPv1 we would introduce a lot of code that
into it just for SPv1.
>
> >
> > - Integrate paint invalidation into PrePaintTreeWalk for SPv2 only.
> >
> > I think the above plan could achieve the goals we planned for SPv1 paint
> invalidation while avoid extra complexity. Integration of paint invalidation
for
> SPv2-only would be clean, free of SPv1 code.
> >
> > Wdyt?
On 2016/03/16 01:25:25, Xianzhu wrote: > On 2016/03/15 23:49:33, chrishtr wrote: > > This is already needed for the existing property tree walk, right? Put another > > way, what is unique about paintOffset for paint invalidation that doesn't > apply > > for property trees? > > First Question: Yes > Second Question: No for SPv1, but for SPv1, paint invalidation container brings > a lot of complexities. > Please ignore the above confusing "answers". Sorry about that. I'm rewriting them: Yes, this is already needed for the existing property tree walk. This is the reason we want to integrate paint invalidation into property tree walk. There should be nothing unique about paintOffset for paint invalidation for SPv2. For SPv1, paintInvalidationContainers causes many complexities, shown with the examples in #16.
On 2016/03/16 at 04:22:32, wangxianzhu wrote: > On 2016/03/16 01:25:25, Xianzhu wrote: > > On 2016/03/15 23:49:33, chrishtr wrote: > > > This is already needed for the existing property tree walk, right? Put another > > > way, what is unique about paintOffset for paint invalidation that doesn't > > apply > > > for property trees? > > > > First Question: Yes > > Second Question: No for SPv1, but for SPv1, paint invalidation container brings > > a lot of complexities. > > > > Please ignore the above confusing "answers". Sorry about that. > > I'm rewriting them: > > Yes, this is already needed for the existing property tree walk. This is the reason we want to integrate paint invalidation into property tree walk. There should be nothing unique about paintOffset for paint invalidation for SPv2. > > For SPv1, paintInvalidationContainers causes many complexities, shown with the examples in #16. Xianzhu, Walter and I met and came up with the following plan: Step 1: Xianzhu implements fixes in the existing paint invalidation tree walk to make it follow DOM order and thereby fix several existing flaws. (Launch it, obviously, just a bugfix.) Step 2: Chris and Walter implement GeometryMapper Step 3: Xianzhu ports step 1 to the property tree tree walk once step 2 is done. Step 4: When launching SPv2, clean up some parts of step 3 involving the paintInvalidationContainer
On 2016/03/16 23:33:33, chrishtr wrote: > On 2016/03/16 at 04:22:32, wangxianzhu wrote: > > On 2016/03/16 01:25:25, Xianzhu wrote: > > > On 2016/03/15 23:49:33, chrishtr wrote: > > > > This is already needed for the existing property tree walk, right? Put > another > > > > way, what is unique about paintOffset for paint invalidation that doesn't > > > apply > > > > for property trees? > > > > > > First Question: Yes > > > Second Question: No for SPv1, but for SPv1, paint invalidation container > brings > > > a lot of complexities. > > > > > > > Please ignore the above confusing "answers". Sorry about that. > > > > I'm rewriting them: > > > > Yes, this is already needed for the existing property tree walk. This is the > reason we want to integrate paint invalidation into property tree walk. There > should be nothing unique about paintOffset for paint invalidation for SPv2. > > > > For SPv1, paintInvalidationContainers causes many complexities, shown with the > examples in #16. > > Xianzhu, Walter and I met and came up with the following plan: > > Step 1: Xianzhu implements fixes in the existing paint invalidation tree walk to > make it follow DOM order and thereby fix several existing flaws. (Launch it, > obviously, just a bugfix.) > Step 2: Chris and Walter implement GeometryMapper > Step 3: Xianzhu ports step 1 to the property tree tree walk once step 2 is done. > Step 4: When launching SPv2, clean up some parts of step 3 involving the > paintInvalidationContainer I would like to unblock paint-invalidation for spv2 from GeometryMapper, so how about the following plan: 1. (finished) Let existing paint invalidation tree walk be in DOM order and fix bugs and optimize 2. Implement paint invalidation for spv2 in PrePaintTreeWalk without depending on GeometryMapper. This should be feasible because we don't need to handle the complexities of and map geometries between interlaced containing block, paint invalidation container, etc. When fast path geometry mapping is not possible, we can just fall back to mapToVisualRectInAncestorSpace() (which will be replaced by GeometryMapper when its ready). 3. Above Step 2 4. Above Step 3 5. Above Step 4
On 2016/05/26 at 22:20:27, wangxianzhu wrote: > On 2016/03/16 23:33:33, chrishtr wrote: > > On 2016/03/16 at 04:22:32, wangxianzhu wrote: > > > On 2016/03/16 01:25:25, Xianzhu wrote: > > > > On 2016/03/15 23:49:33, chrishtr wrote: > > > > > This is already needed for the existing property tree walk, right? Put > > another > > > > > way, what is unique about paintOffset for paint invalidation that doesn't > > > > apply > > > > > for property trees? > > > > > > > > First Question: Yes > > > > Second Question: No for SPv1, but for SPv1, paint invalidation container > > brings > > > > a lot of complexities. > > > > > > > > > > Please ignore the above confusing "answers". Sorry about that. > > > > > > I'm rewriting them: > > > > > > Yes, this is already needed for the existing property tree walk. This is the > > reason we want to integrate paint invalidation into property tree walk. There > > should be nothing unique about paintOffset for paint invalidation for SPv2. > > > > > > For SPv1, paintInvalidationContainers causes many complexities, shown with the > > examples in #16. > > > > Xianzhu, Walter and I met and came up with the following plan: > > > > Step 1: Xianzhu implements fixes in the existing paint invalidation tree walk to > > make it follow DOM order and thereby fix several existing flaws. (Launch it, > > obviously, just a bugfix.) > > Step 2: Chris and Walter implement GeometryMapper > > Step 3: Xianzhu ports step 1 to the property tree tree walk once step 2 is done. > > Step 4: When launching SPv2, clean up some parts of step 3 involving the > > paintInvalidationContainer > > I would like to unblock paint-invalidation for spv2 from GeometryMapper, so how about the following plan: Go for it. GeometryMapper is taking longer than expected...Walter and I keep ending up spending almost all our time on bugs and visual rects, but we should be able to plug things in pretty easily after your work here. > > 1. (finished) Let existing paint invalidation tree walk be in DOM order and fix bugs and optimize > 2. Implement paint invalidation for spv2 in PrePaintTreeWalk without depending on GeometryMapper. This should be feasible because we don't need to handle the complexities of and map geometries between interlaced containing block, paint invalidation container, etc. When fast path geometry mapping is not possible, we can just fall back to mapToVisualRectInAncestorSpace() (which will be replaced by GeometryMapper when its ready). > 3. Above Step 2 > 4. Above Step 3 > 5. Above Step 4
Uploaded a rebased patch with the previous comments addressed. Reviewers do you have more comments?
Very nice overall, just a few small comments. Can you add a small comment about this in one of our README.md files? https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:75: // Special things include but not limit to: overflow cllip, transform, fixed-pos, animation, Nittt: clip https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016? Or 2015 is ok if this is a copy of PaintPropertyTreeWalk https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h (right): https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h:21: void walk(FrameView& rootFrame); Nit: const FrameView&? https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h:24: void walk(FrameView&, const PrePaintTreeWalkContext&); Nit: const FrameView&?
https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:75: // Special things include but not limit to: overflow cllip, transform, fixed-pos, animation, On 2016/05/31 23:17:39, pdr. wrote: > Nittt: clip Donne. https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/31 23:17:39, pdr. wrote: > 2016? Or 2015 is ok if this is a copy of PaintPropertyTreeWalk I'll keep 2015 because most code is copied. https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h (right): https://codereview.chromium.org/1791543005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.h:21: void walk(FrameView& rootFrame); On 2016/05/31 23:17:39, pdr. wrote: > Nit: const FrameView&? FrameView doesn't have getMutableForPainting() and we need to call its non-const methods (setPreTranslation() etc.) We could consider the same mechanism to isolate mutations from painting, but this seems not a big deal because FrameView's status is much simpler that that of LayoutObjects.
On 2016/05/31 23:17:39, pdr. wrote: > Very nice overall, just a few small comments. > > Can you add a small comment about this in one of our README.md files? > Done.
This patch LGTM (review seems light but we talked about this at length a couple months ago). I'd like either jbroman or trchen to review it as well to make sure we're all on the same page. @trchen or @jbroman, can you please take a look?
I don't have any objection in principle to this patch; looks good to me. (I haven't thought deeply about what the interaction between paint invalidation and paint property tree building is, but I gather Xianzhu has.)
lgtm too. On 2016/06/01 15:05:42, jbroman wrote: > I don't have any objection in principle to this patch; looks good to me. > > (I haven't thought deeply about what the interaction between paint invalidation > and paint property tree building is, but I gather Xianzhu has.) I think they are mostly orthogonal, except that sometimes you may need to invalidate the old backing thus needing old geometries? I believe the biggest advantage was to avoid walking the LayoutObject tree twice.
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1791543005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1791543005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, trchen@chromium.org Link to the patchset: https://codereview.chromium.org/1791543005/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1791543005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1791543005/200001
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, trchen@chromium.org Link to the patchset: https://codereview.chromium.org/1791543005/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1791543005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1791543005/220001
Message was sent while issue was closed.
Description was changed from ========== InPrePaint document state and PrePaintTreeWalk class - InUpdatePaintProperty document state is renamed to InPrePaint; - The original PaintPropertyTreeBuilder class is split into PrePaintTreeWalk which walks the tree and the new PaintPropertyTreeBuilder which build the property tree. This is preparation for a combined treewalk for both paint property tree generation and paint invalidation. For more information, see: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... BUG=591199 ========== to ========== InPrePaint document state and PrePaintTreeWalk class - InUpdatePaintProperty document state is renamed to InPrePaint; - The original PaintPropertyTreeBuilder class is split into PrePaintTreeWalk which walks the tree and the new PaintPropertyTreeBuilder which build the property tree. This is preparation for a combined treewalk for both paint property tree generation and paint invalidation. For more information, see: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... BUG=591199 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== InPrePaint document state and PrePaintTreeWalk class - InUpdatePaintProperty document state is renamed to InPrePaint; - The original PaintPropertyTreeBuilder class is split into PrePaintTreeWalk which walks the tree and the new PaintPropertyTreeBuilder which build the property tree. This is preparation for a combined treewalk for both paint property tree generation and paint invalidation. For more information, see: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... BUG=591199 ========== to ========== InPrePaint document state and PrePaintTreeWalk class - InUpdatePaintProperty document state is renamed to InPrePaint; - The original PaintPropertyTreeBuilder class is split into PrePaintTreeWalk which walks the tree and the new PaintPropertyTreeBuilder which build the property tree. This is preparation for a combined treewalk for both paint property tree generation and paint invalidation. For more information, see: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... BUG=591199 Committed: https://crrev.com/c3932e54885cbcc80b011bda5b45c592b22f1ff0 Cr-Commit-Position: refs/heads/master@{#397280} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c3932e54885cbcc80b011bda5b45c592b22f1ff0 Cr-Commit-Position: refs/heads/master@{#397280} |
