RenderLayer::hasVisibleDescendant should ASSERT(!m_visibleDescendantStatusDirty)

RenderLayer::hasVisibleDescendant should ASSERT(!m_visibleDescendantStatusDirty)

No one should be reading the hasVisibleDescendant state while its dirty. This
CL adds an ASSERT to prevent that from happening and fixes the callers that are
reading dirty state today.

R=skobes@chromium.org

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

[chrishtr, 2014-08-08 20:35:08]

Looks like this CL changed from the original description?

[abarth-chromium, 2014-08-08 20:38:11]

On 2014/08/08 at 20:35:08, chrishtr wrote:
> Looks like this CL changed from the original description?

Looks right...  What's inconsistent?

[skobes, 2014-08-08 20:43:18]

https://codereview.chromium.org/455963002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/455963002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1968:
layer->updateDescendantDependentFlags();
Why is this method, which doesn't seem like it should have side effects,
responsible for performing the update?

It seems like we should either make hasVisibleDescendant() do the update
automatically instead of asserting, or there should be some well-defined point
where it is updated.

[chrishtr, 2014-08-08 20:51:46]

Oh never mind, misread the code.

[abarth-chromium, 2014-08-08 21:24:40]

https://codereview.chromium.org/455963002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/455963002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1968:
layer->updateDescendantDependentFlags();
That's a good question.  The answer is slightly involved:

1) There is a defined time for clean out this state, which is just prior to
entering the InCompositingUpdate document lifecycle state.
2) Some folks need to query this information earlier (e.g., during layout), and
those call sites are responsible for cleaning the subtree they need to query.

I agree with you that we should make hasVisibleDescendant automatically clear
out the state (as well as ASSERT that we haven't yet reached
InCompositingUpdate).  I originally tried that approach, but the patch quickly
spiraled out of control and I was failing tests for reasons that were difficult
for me to understand.

The approach I've taking in this CL (and I hope to continue) is to clean up this
system in small, controlled CLs.  Once we've got the system maintaining its
dirty bits with reasonable invariants, I agree with you that we should switch to
an "automatic cleaning" design.

If you prefer, I can try the automatic cleaning approach again and attempt to
break that change down into small, controlled pieces.  I understand the system
better now and might be able to make that work.

Please let me know which approach you'd prefer.

Thanks!

[skobes, 2014-08-08 21:45:01]

https://codereview.chromium.org/455963002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/455963002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1968:
layer->updateDescendantDependentFlags();
On 2014/08/08 21:24:39, abarth wrote:
> That's a good question.  The answer is slightly involved:
> 
> 1) There is a defined time for clean out this state, which is just prior to
> entering the InCompositingUpdate document lifecycle state.
> 2) Some folks need to query this information earlier (e.g., during layout),
and
> those call sites are responsible for cleaning the subtree they need to query.
> 
> I agree with you that we should make hasVisibleDescendant automatically clear
> out the state (as well as ASSERT that we haven't yet reached
> InCompositingUpdate).  I originally tried that approach, but the patch quickly
> spiraled out of control and I was failing tests for reasons that were
difficult
> for me to understand.
> 
> The approach I've taking in this CL (and I hope to continue) is to clean up
this
> system in small, controlled CLs.  Once we've got the system maintaining its
> dirty bits with reasonable invariants, I agree with you that we should switch
to
> an "automatic cleaning" design.
> 
> If you prefer, I can try the automatic cleaning approach again and attempt to
> break that change down into small, controlled pieces.  I understand the system
> better now and might be able to make that work.
> 
> Please let me know which approach you'd prefer.
> 
> Thanks!

Got it... I'm fine with the approach here as long as there is a plan to get into
a better state.  Can you add a comment?

[abarth-chromium, 2014-08-08 21:47:30]

On 2014/08/08 at 21:45:01, skobes wrote:
> Got it... I'm fine with the approach here as long as there is a plan to get
into a better state.  Can you add a comment?

Sure.  Thanks!

[abarth-chromium, 2014-08-08 21:50:59]

On 2014/08/08 at 21:47:30, abarth wrote:
> On 2014/08/08 at 21:45:01, skobes wrote:
> > Got it... I'm fine with the approach here as long as there is a plan to get
into a better state.  Can you add a comment?
> 
> Sure.  Thanks!

I rolled the comment into the next CL because this one has green try jobs (and
I'm afraid to restart them):

https://codereview.chromium.org/453113002

[skobes, 2014-08-08 21:52:16]

lgtm

[abarth-chromium, 2014-08-08 21:57:06]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-08 21:57:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/455963002/1

[commit-bot: I haz the power, 2014-08-08 22:11:31]

Change committed as 179874