Add ASSERT childNeedsDistributionRecalc in ComposedTreeTraversal

Add ASSERT childNeedsDistributionRecalc in ComposedTreeTraversal

Bug 450115 is about changing behaviors of some attributes to
use the composed trees when used in Shadow DOM. We'll need to
change some NodeTraversal to ComposedTreeTraversal, but before
doing so, this patch adds necessary but currently missing ASSERT
to ComposedTreeTraversal, matching its ASSERTs to the one in
NodeRenderingTraversal.

The fix to Text::textRendererIsNeeded() supresses, under rare
condition, its memory opmitization to avoid creating Renderer
when it's not needed. The comment suggests it's not essential,
and it's very rare to hit this condition; one such example is
when a JS handler of DOMNodeInsertedIntoDocument modifies
text.

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

[kojii, 2015-01-23 03:50:47]

kojii@chromium.org changed reviewers:
+ hayato@chromium.org

[kojii, 2015-01-23 03:50:48]

PTAL!

[hayato, 2015-01-23 04:06:31]

Thanks.

https://codereview.chromium.org/845453006/diff/40001/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/845453006/diff/40001/Source/core/dom/Text.cpp...
Source/core/dom/Text.cpp:281: if (document().childNeedsDistributionRecalc())
Could you help me to understand why it is okay to return false  if
document().childNeedsDistributionRecalc() is true here?

It's unclear to me.

[kojii, 2015-01-23 04:23:13]

https://codereview.chromium.org/845453006/diff/40001/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/845453006/diff/40001/Source/core/dom/Text.cpp...
Source/core/dom/Text.cpp:281: if (document().childNeedsDistributionRecalc())
On 2015/01/23 04:06:30, hayato wrote:
> Could you help me to understand why it is okay to return false  if
> document().childNeedsDistributionRecalc() is true here?
> 
> It's unclear to me.

In short, this fix disables a non-essential memory optimization in a rare
condition.

Comments in line 296 says this function is to avoid creation of a Renderer for a
non-essential memory optimization, and and if it takes too long to figure that
out precisely, this function returns true. It does so by limiting the number of
siblings to traverse up to 50.

So, returning true here should be no worse than creating a possibly unnecessary
Renderer. WebKit does not have this optimization, so when this hits, we're as
not-optimized as WebKit.

The only case I found to reach here is when a JavaScript handler for
DOMNodeInsertedIntoDocument calls Range::surroundContents() and when it tries to
delete some text, it calls Text::updateTextRenderer(). No other tests hit this
condition, so I consider this is rare enough.

[kojii, 2015-01-23 04:48:30]

On 2015/01/23 04:23:13, koji wrote:
> https://codereview.chromium.org/845453006/diff/40001/Source/core/dom/Text.cpp
> File Source/core/dom/Text.cpp (right):
> 
>
https://codereview.chromium.org/845453006/diff/40001/Source/core/dom/Text.cpp...
> Source/core/dom/Text.cpp:281: if (document().childNeedsDistributionRecalc())
> On 2015/01/23 04:06:30, hayato wrote:
> > Could you help me to understand why it is okay to return false  if
> > document().childNeedsDistributionRecalc() is true here?
> > 
> > It's unclear to me.
> 
> In short, this fix disables a non-essential memory optimization in a rare
> condition.
> 
> Comments in line 296 says this function is to avoid creation of a Renderer for
a
> non-essential memory optimization, and and if it takes too long to figure that
> out precisely, this function returns true. It does so by limiting the number
of
> siblings to traverse up to 50.
> 
> So, returning true here should be no worse than creating a possibly
unnecessary
> Renderer. WebKit does not have this optimization, so when this hits, we're as
> not-optimized as WebKit.
> 
> The only case I found to reach here is when a JavaScript handler for
> DOMNodeInsertedIntoDocument calls Range::surroundContents() and when it tries
to
> delete some text, it calls Text::updateTextRenderer(). No other tests hit this
> condition, so I consider this is rare enough.

Updated description to include this information.

[hayato, 2015-01-23 04:52:32]

Thank you for the explanation. lgtm.

[kojii, 2015-01-23 04:53:11]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-01-23 04:53:58]

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

[commit-bot: I haz the power, 2015-01-23 04:57:12]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188852