Update hasVisibleContent() to include visibility of non-self-painting layers.

Update hasVisibleContent() to include visibility of non-self-painting layers.

BUG=498039
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197853

[chrishtr, 2015-06-25 17:26:49]

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

[chrishtr, 2015-06-25 17:26:50]

https://codereview.chromium.org/1179403002/diff/80001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1179403002/diff/80001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:598:
parent()->dirtyVisibleContentStatus();
compositing/contents-opaque/visibility-hidden.html exercises this code, because
the <nav> element in that test is a non-self-painting layer.

[trchen, 2015-06-25 19:45:38]

It feels to me that LayoutObject::insertedIntoTree and
LayoutObject::willBeRemovedFromTree needs to match the same condition. Other
than that lgtm.

[chrishtr, 2015-06-25 20:02:36]

On 2015/06/25 at 19:45:38, trchen wrote:
> It feels to me that LayoutObject::insertedIntoTree and
LayoutObject::willBeRemovedFromTree needs to match the same condition. Other
than that lgtm.

I don't think that is needed, because this patch applies only to cases when a
layer exists. In that case, the insertion or removal will be accompanied by an
insertion or removal of the corresponding DeprecatedPaintLayer, which will run
the code I added.

Also, when investigating your comment, I found that
DeprecatedPaintLayer::removeChild dirties visible content status, but only if
the parent is already marked as not-visible. I don't think that is correct with
non-self-painting children, since the child could be the one that caused the
parent to appear visible. Investigating that now.

[chrishtr, 2015-06-25 20:12:58]

On 2015/06/25 at 20:02:36, chrishtr wrote:
> On 2015/06/25 at 19:45:38, trchen wrote:
> > It feels to me that LayoutObject::insertedIntoTree and
LayoutObject::willBeRemovedFromTree needs to match the same condition. Other
than that lgtm.
> 
> I don't think that is needed, because this patch applies only to cases when a
layer exists. In that case, the insertion or removal will be accompanied by an
insertion or removal of the corresponding DeprecatedPaintLayer, which will run
the code I added.
> 
> Also, when investigating your comment, I found that
DeprecatedPaintLayer::removeChild dirties visible content status, but only if
the parent is already marked as not-visible. I don't think that is correct with
non-self-painting children, since the child could be the one that caused the
parent to appear visible. Investigating that now.

Ah it's all good. The check in ::removeChild just short-circuits if the parent
is already visible on its own without any contributing children.

[chrishtr, 2015-06-25 20:13:00]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-06-25 20:13:21]

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

[commit-bot: I haz the power, 2015-06-25 20:16:52]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197853