Check DisplayItemCient aliveness in cached subsequences

Check DisplayItemCient aliveness in cached subsequences

BUG=609218
Committed: https://crrev.com/127e5fadca5f431a5de1bef939f7f43aec1d136f
Cr-Commit-Position: refs/heads/master@{#400607}

[Xianzhu, 2016-06-17 23:17:36]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2016-06-18 08:49:57]

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:965: // We don't invalidate
layers during document detach(), so must clear the should-keep-alive
Does this need to be true any more now that we use cache generations?

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:73:
displayItem.client().beginShouldKeepAlive(m_subsequenceClients.last());
What does this do exactly? beginShouldKeepAlive for the begin-subsequence marker
when processing something within the subsequence?
I don't fully understand this yet.

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:244:
Vector<const DisplayItemClient*> m_subsequenceClients;
Document what this is for.

[Xianzhu, 2016-06-18 17:41:30]

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:965: // We don't invalidate
layers during document detach(), so must clear the should-keep-alive
On 2016/06/18 08:49:57, chrishtr wrote:
> Does this need to be true any more now that we use cache generations?

This doesn't affect cache correctness, but is only related to aliveness
tracking. Because when a document is detached, LayoutObjects are deleted before
PaintLayers, without this code we would crash when LayoutObjects were deleted
because they were still under shouldKeepAlive control of the ancestor
PaintLayers.

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:73:
displayItem.client().beginShouldKeepAlive(m_subsequenceClients.last());
On 2016/06/18 08:49:57, chrishtr wrote:
> What does this do exactly? beginShouldKeepAlive for the begin-subsequence
marker
> when processing something within the subsequence?
> I don't fully understand this yet.

Yes, this is to mark the new display item shouldKeepAlive under the current
subsequence. The shouldKeepAlive status will end when the subsequence owner is
invalidated or deleted, which is handled in DisplayItemClient.cpp.

Added comments.

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2066603005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:244:
Vector<const DisplayItemClient*> m_subsequenceClients;
On 2016/06/18 08:49:57, chrishtr wrote:
> Document what this is for.

Done.

[chrishtr, 2016-06-19 01:04:12]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-06-19 01:04:13]

lgtm

[commit-bot: I haz the power, 2016-06-19 01:04:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-19 01:04:23]

This CL has an open dependency (Issue 2071053003 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Xianzhu, 2016-06-19 03:07:55]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-06-19 03:08:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2066603005/100001

[commit-bot: I haz the power, 2016-06-19 03:12:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-19 03:14:26]

Description was changed from

==========
Check DisplayItemCient aliveness in cached subsequences

BUG=609218
==========

to

==========
Check DisplayItemCient aliveness in cached subsequences

BUG=609218

Committed: https://crrev.com/127e5fadca5f431a5de1bef939f7f43aec1d136f
Cr-Commit-Position: refs/heads/master@{#400607}
==========

[commit-bot: I haz the power, 2016-06-19 03:14:28]

Patchset 6 (id:??) landed as
https://crrev.com/127e5fadca5f431a5de1bef939f7f43aec1d136f
Cr-Commit-Position: refs/heads/master@{#400607}

[vasilii, 2016-06-20 10:02:43]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2078393003/ by vasilii@chromium.org.

The reason for reverting is: Based on r400605 that is to be reverted..

[Xianzhu, 2016-06-20 17:04:50]

On 2016/06/20 10:02:43, vasilii wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/2078393003/ by mailto:vasilii@chromium.org.
> 
> The reason for reverting is: Based on r400605 that is to be reverted..

Note that the revert was reverted later.