[chromium] Target surface should be damaged for a new layers even when layer had no changes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

In the CCDamageTracker, if a new layer was added to contribute to a target surface, but the layer itself had no change, then it was not causing damage to the target surface.

I don't think this bug actually caused any of our known issues, but of course it should be fixed anyway.

Patch coming in a moment.

[Shawn Singh]

Created attachment 133200 [details]
Patch

[WebKit Review Bot]

Comment on attachment 133200 [details]
Patch

Attachment 133200 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12072802

[Michal Mocny]

Shawn, I really doubt it will matter, but that patch of mine you tried would not have done anything without also applying https://chromiumcodereview.appspot.com/9699125/ to chrome.

[Dana Jansens]

Is this the iphone page bug?

[Michal Mocny]

Dana: I dont think so, shawn found this while debugging that issue, but he seemed to think it didnt fix any known issues -- its just the right thing to do(TM).

[Adrienne Walker]

Comment on attachment 133200 [details]
Patch

Out of curiosity, should getting a new parent or being assigned as a mask or replica layer cause "layerPropertyHasChanged()"?

[Shawn Singh]

(In reply to comment #3)
> Shawn, I really doubt it will matter, but that patch of mine you tried would not have done anything without also applying https://chromiumcodereview.appspot.com/9699125/ to chrome.

I guess this suggestion actually applies to http://code.google.com/p/chromium/issues/detail?id=119121 ?   OK, I'll try it... And thanks again for the offline discussion yesterday, very helpful.


(In reply to comment #4)
> Is this the iphone page bug?

Nope - as mmocny@ pointed out already =)

(In reply to comment #6)
> (From update of attachment 133200 [details])
> Out of curiosity, should getting a new parent or being assigned as a mask or replica layer cause "layerPropertyHasChanged()"?

probably not, at least not any time soon.  Tree manipulations and mask/replica setting occurs during synchronizeTrees and (at least for now) it does that forcibly every time, even when it should not be considered a "change".  We could conceivably catch these on the LayerChromium side in a later patch?  But I also feel that even if we did do that, this patch is still appropriate.  What do you think?

[Michal Mocny]

Shawn, yes you are right, I meant to post that there.

[Dana Jansens]

> (In reply to comment #6)
> > (From update of attachment 133200 [details] [details])
> > Out of curiosity, should getting a new parent or being assigned as a mask or replica layer cause "layerPropertyHasChanged()"?
> 
> probably not, at least not any time soon.  Tree manipulations and mask/replica setting occurs during synchronizeTrees and (at least for now) it does that forcibly every time, even when it should not be considered a "change".  We could conceivably catch these on the LayerChromium side in a later patch?  But I also feel that even if we did do that, this patch is still appropriate.  What do you think?

I think you can if (parent != m_parent) the change. They are not set to 0 or anything AIUI.

[Shawn Singh]

(In reply to comment #9)
> > (In reply to comment #6)
> > > (From update of attachment 133200 [details] [details] [details])
> > > Out of curiosity, should getting a new parent or being assigned as a mask or replica layer cause "layerPropertyHasChanged()"?
> > 
> > probably not, at least not any time soon.  Tree manipulations and mask/replica setting occurs during synchronizeTrees and (at least for now) it does that forcibly every time, even when it should not be considered a "change".  We could conceivably catch these on the LayerChromium side in a later patch?  But I also feel that even if we did do that, this patch is still appropriate.  What do you think?
> 
> I think you can if (parent != m_parent) the change. They are not set to 0 or anything AIUI.


CCLayerImpl::removeFromParent() does set m_parent to 0   ... and personally I think that's desired behavior, anyway, when thinking about an API of adding/removing nodes from a tree in general.  The "correct" solution in my opinion is to make the tree synchronizer avoid unnecessary tree manipulations; but its unclear how hairy and tangled that will become.

uploading a new patch in a moment,,,

[Dana Jansens]

(In reply to comment #10)
> (In reply to comment #9)
> > > (In reply to comment #6)
> > > > (From update of attachment 133200 [details] [details] [details] [details])
> > > > Out of curiosity, should getting a new parent or being assigned as a mask or replica layer cause "layerPropertyHasChanged()"?
> > > 
> > > probably not, at least not any time soon.  Tree manipulations and mask/replica setting occurs during synchronizeTrees and (at least for now) it does that forcibly every time, even when it should not be considered a "change".  We could conceivably catch these on the LayerChromium side in a later patch?  But I also feel that even if we did do that, this patch is still appropriate.  What do you think?
> > 
> > I think you can if (parent != m_parent) the change. They are not set to 0 or anything AIUI.
> 
> 
> CCLayerImpl::removeFromParent() does set m_parent to 0   ... and personally I think that's desired behavior, anyway, when thinking about an API of adding/removing nodes from a tree in general.  The "correct" solution in my opinion is to make the tree synchronizer avoid unnecessary tree manipulations; but its unclear how hairy and tangled that will become.

Oh! Oops sorry, bad memory.

> uploading a new patch in a moment,,,

[Shawn Singh]

Created attachment 133303 [details]
Should build on linux now

[James Robinson]

I think if you want to know about tree structure changes on the impl side the right thing to do is to compare layer IDs before/after the sync. They IDs are persistent by design, but the TreeSynchronizer makes no attempt to preserve tree structure (I don't know of an efficient+simple way to do that).

[Shawn Singh]

(In reply to comment #13)
> I think if you want to know about tree structure changes on the impl side the right thing to do is to compare layer IDs before/after the sync. They IDs are persistent by design, but the TreeSynchronizer makes no attempt to preserve tree structure (I don't know of an efficient+simple way to do that).

(1) James and Enne, just for clarity to me... are you guys suggesting that we should consider doing that before this patch?  If so, I disagree.  The more I think about it now, I'm not convinced we'll ever need to make layer properties as changed when the tree structure changes.  The damage tracker doesn't care about that, it only cares whether layers are "new", "disappeared" or "the layer is damaged", with respect to its target render surface.

(2) "compare layer IDs before / after sync" --> are you suggesting that we look at how the layer ID changed from the "before" snapshot of the tree to the "after" snapshot of the tree?  How is that any less complex than detecting changes during tree synchronization?    On the other hand, if you mean that we compare layer IDs of the  host tree versus the impl tree, before synchronizing, and then mark the appropriate subtrees as "changed" ... that's is not too much different than simply makring layerChromiums as "propertyChanged" and propagating that to the impl side, except the latter would be simpler.

Thanks for all the feedback so far =)

[Adrienne Walker]

Comment on attachment 133303 [details]
Should build on linux now

View in context: https://bugs.webkit.org/attachment.cgi?id=133303&action=review

Yeah, I think this does end up being cleaner than marking properties on the layer as changing elsewhere.  R=me.

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:380
> +    ASSERT_EQ(static_cast<size_t>(3), root->renderSurface()->layerList().size());

You can just say "3u" here, if that's easier.

[Shawn Singh]

Enne, if its OK with you I'll change the static_cast everywhere in CCDamageTrackerTest.cpp when landing.

[Adrienne Walker]

(In reply to comment #16)
> Enne, if its OK with you I'll change the static_cast everywhere in CCDamageTrackerTest.cpp when landing.

Sounds good!

[Shawn Singh]

Created attachment 133404 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 133404 [details]
Patch for landing

Clearing flags on attachment: 133404

Committed r111817: <http://trac.webkit.org/changeset/111817>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.