Descend into inner blocks for inflateAutoTable.

Descend into inner blocks for inflateAutoTable.

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

[pdr., 2014-06-10 03:42:10]

Idea: would applying word-wrap allow us to render this testcase nicely without
the O(n^2) descending?
https://developer.mozilla.org/en-US/docs/Web/CSS/word-wrap

https://codereview.chromium.org/320393003/diff/1/Source/core/rendering/FastTe...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/320393003/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:470: multiplier = inflate(child,
behavior, multiplier);
This can lead to an assert or incorrect values in multiplierFromBlock. When we
start inflating the multiplier will be 0 since it hasn't been calculated yet. If
we encounter a block with text, computing the multiplier can lead to an assert
because the block hasn't entered layout yet.

[skobes, 2014-06-10 17:22:51]

I don't understand the comment about word-wrap, can you elaborate?

https://codereview.chromium.org/320393003/diff/1/Source/core/rendering/FastTe...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/320393003/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:470: multiplier = inflate(child,
behavior, multiplier);
On 2014/06/10 03:42:10, pdr wrote:
> This can lead to an assert or incorrect values in multiplierFromBlock. When we
> start inflating the multiplier will be 0 since it hasn't been calculated yet.
If
> we encounter a block with text, computing the multiplier can lead to an assert
> because the block hasn't entered layout yet.

There is special logic in widthFromBlock to handle tables that haven't entered
layout yet.

[pdr., 2014-06-11 04:07:17]

On 2014/06/10 17:22:51, skobes wrote:
> I don't understand the comment about word-wrap, can you elaborate?

The idea would be to autosize nested tables as we do today, even though that can
lead to long words spilling out of the cell (as your testcase shows). We could
apply word wrapping to this text to force it to wrap instead of spilling out,
and that would let us keep skip the descendant walk but still render pretty
nicely.

> 
>
https://codereview.chromium.org/320393003/diff/1/Source/core/rendering/FastTe...
> File Source/core/rendering/FastTextAutosizer.cpp (right):
> 
>
https://codereview.chromium.org/320393003/diff/1/Source/core/rendering/FastTe...
> Source/core/rendering/FastTextAutosizer.cpp:470: multiplier = inflate(child,
> behavior, multiplier);
> On 2014/06/10 03:42:10, pdr wrote:
> > This can lead to an assert or incorrect values in multiplierFromBlock. When
we
> > start inflating the multiplier will be 0 since it hasn't been calculated
yet.
> If
> > we encounter a block with text, computing the multiplier can lead to an
assert
> > because the block hasn't entered layout yet.
> 
> There is special logic in widthFromBlock to handle tables that haven't entered
> layout yet.

There is but the new code descends into non-table descendants. I don't think we
properly handle that since we have this early return in widthFromBlock:
    if (!(block->isTable() || block->isTableCell() || block->isListItem()))
        return block->contentLogicalWidth().toFloat();

[pdr., 2014-06-11 23:40:55]

On 2014/06/11 04:07:17, pdr wrote:
> On 2014/06/10 17:22:51, skobes wrote:
> > I don't understand the comment about word-wrap, can you elaborate?
> 
> The idea would be to autosize nested tables as we do today, even though that
can
> lead to long words spilling out of the cell (as your testcase shows). We could
> apply word wrapping to this text to force it to wrap instead of spilling out,
> and that would let us keep skip the descendant walk but still render pretty
> nicely.
> 
> > 
> >
>
https://codereview.chromium.org/320393003/diff/1/Source/core/rendering/FastTe...
> > File Source/core/rendering/FastTextAutosizer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/320393003/diff/1/Source/core/rendering/FastTe...
> > Source/core/rendering/FastTextAutosizer.cpp:470: multiplier = inflate(child,
> > behavior, multiplier);
> > On 2014/06/10 03:42:10, pdr wrote:
> > > This can lead to an assert or incorrect values in multiplierFromBlock.
When
> we
> > > start inflating the multiplier will be 0 since it hasn't been calculated
> yet.
> > If
> > > we encounter a block with text, computing the multiplier can lead to an
> assert
> > > because the block hasn't entered layout yet.
> > 
> > There is special logic in widthFromBlock to handle tables that haven't
entered
> > layout yet.
> 
> There is but the new code descends into non-table descendants. I don't think
we
> properly handle that since we have this early return in widthFromBlock:
>     if (!(block->isTable() || block->isTableCell() || block->isListItem()))
>         return block->contentLogicalWidth().toFloat();

I didn't catch that we're using the root here and not the block itself.

LGTM after all.

[skobes, 2014-06-11 23:45:27]

Thanks!

On 2014/06/11 04:07:17, pdr wrote:
> The idea would be to autosize nested tables as we do today, even though that
can
> lead to long words spilling out of the cell (as your testcase shows). We could
> apply word wrapping to this text to force it to wrap instead of spilling out,
> and that would let us keep skip the descendant walk but still render pretty
> nicely.

This is an interesting idea.  However as discussed offline I think FTA shouldn't
be modifying CSS properties like word-wrap and I definitely don't want to add
one-offs to the line breaking code for this.

[skobes, 2014-06-12 05:27:16]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-06-12 05:28:04]

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

[commit-bot: I haz the power, 2014-06-12 07:00:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/11166)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11683)

[commit-bot: I haz the power, 2014-06-12 07:31:17]

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

[commit-bot: I haz the power, 2014-06-12 07:31:19]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[skobes, 2014-06-12 17:57:18]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-06-12 17:57:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/320393003/20001

[commit-bot: I haz the power, 2014-06-12 21:25:20]

Change committed as 176047