Bug 81879 - [chromium] Target surface should be damaged for a new layers even when layer had no changes
Summary: [chromium] Target surface should be damaged for a new layers even when layer ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-22 00:35 PDT by Shawn Singh
Modified: 2012-03-22 20:03 PDT (History)
9 users (show)

See Also:


Attachments
Patch (8.88 KB, patch)
2012-03-22 00:43 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Should build on linux now (8.83 KB, patch)
2012-03-22 11:42 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (12.09 KB, patch)
2012-03-22 18:14 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-03-22 00:35:30 PDT
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.
Comment 1 Shawn Singh 2012-03-22 00:43:17 PDT
Created attachment 133200 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-22 01:01:26 PDT
Comment on attachment 133200 [details]
Patch

Attachment 133200 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12072802
Comment 3 Michal Mocny 2012-03-22 06:32:38 PDT
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.
Comment 4 Dana Jansens 2012-03-22 06:36:55 PDT
Is this the iphone page bug?
Comment 5 Michal Mocny 2012-03-22 06:41:32 PDT
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).
Comment 6 Adrienne Walker 2012-03-22 09:32:11 PDT
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()"?
Comment 7 Shawn Singh 2012-03-22 11:27:18 PDT
(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?
Comment 8 Michal Mocny 2012-03-22 11:29:21 PDT
Shawn, yes you are right, I meant to post that there.
Comment 9 Dana Jansens 2012-03-22 11:30:17 PDT
> (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.
Comment 10 Shawn Singh 2012-03-22 11:39:08 PDT
(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,,,
Comment 11 Dana Jansens 2012-03-22 11:40:50 PDT
(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,,,
Comment 12 Shawn Singh 2012-03-22 11:42:15 PDT
Created attachment 133303 [details]
Should build on linux now
Comment 13 James Robinson 2012-03-22 14:50:56 PDT
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).
Comment 14 Shawn Singh 2012-03-22 15:38:51 PDT
(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 =)
Comment 15 Adrienne Walker 2012-03-22 17:39:18 PDT
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.
Comment 16 Shawn Singh 2012-03-22 17:53:39 PDT
Enne, if its OK with you I'll change the static_cast everywhere in CCDamageTrackerTest.cpp when landing.
Comment 17 Adrienne Walker 2012-03-22 17:54:31 PDT
(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!
Comment 18 Shawn Singh 2012-03-22 18:14:57 PDT
Created attachment 133404 [details]
Patch for landing
Comment 19 WebKit Review Bot 2012-03-22 20:02:56 PDT
Comment on attachment 133404 [details]
Patch for landing

Clearing flags on attachment: 133404

Committed r111817: <http://trac.webkit.org/changeset/111817>
Comment 20 WebKit Review Bot 2012-03-22 20:03:01 PDT
All reviewed patches have been landed.  Closing bug.