(CANCELED) Fix TextAutosizer not to scheduleRelayout()

Fix TextAutosizer not to scheduleRelayout()

This patch fixes TextAutosizer not to scheduleRelayout() when it's in
layout. LayoutBlockFlow and LayoutFlexibleBox were fixed in r386786,
and LayoutGrid was fixed in r387118.

This patch fixes all the rest, except TableLayoutAlgorithmAuto::
computeIntrinsicLogicalWidths() which is not easy because it's created
only while computing the intrinsic logical width.

BUG=602908

[kojii, 2016-04-15 15:01:20]

Description was changed from

==========
ta-table

BUG=602908
==========

to

==========
Fix TextAutosizer not to scheduleRelayout()

This patch fixes TextAutosizer not to scheduleRelayout() when it's in
layout. LayoutBlockFlow and LayoutFlexibleBox were fixed in r386786,
and LayoutGrid was fixed in r387118.

This patch fixes all the rest, except TableLayoutAlgorithmAuto::
computeIntrinsicLogicalWidths() which is not easy because it's created
only while computing the intrinsic logical width.

BUG=602908
==========

[kojii, 2016-04-15 15:10:57]

kojii@chromium.org changed reviewers:
+ cbiesinger@chromium.org, leviw@chromium.org

[kojii, 2016-04-15 15:10:58]

PTAL.

I was thinking to fix one by one to so that regressions if any can be located
easier, but as I read more, it looks too conservative, so put all but one into
this CL. If you prefer to split, that's fine with me, please let me know.

The last one looks quite hard. It's in computeIntrinsicsLogicalWidths() which
doesn't clearNeedsLayout() on exit, so I can't create SubtreeLayoutScope which
RELEASE_ASSERT !needsLayout() in dtor.

LayoutTable::layout() has SubtreeLayoutScope but it goes through virtual
functions that it's hard to pass it through. I could add SubtreeLayoutScope* to
the member of TableLayoutAlgorithmAuto and set/clear as appropriate, but that
doesn't look really great design.

I could do that if that looks ok to you, or try other ideas if you have any,
either in this CL or in a separate CL. I'm also ok to leave it for now.

Could you advise?

[cbiesinger, 2016-04-15 17:08:08]

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

https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:212: //
layout, but it's hard to get it here.
Hmm yeah that one is tricky...

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

https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:241:
ASSERT(!simplifiedLayout(nullptr));
I don't really know much about SVG but this seems weird?

[kojii, 2016-04-15 17:58:44]

Thank you for your prompt review.

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

https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:212: //
layout, but it's hard to get it here.
On 2016/04/15 at 17:08:08, cbiesinger wrote:
> Hmm yeah that one is tricky...

Yeah...or, what we really need is to avoid scheduleLayout() and
SubtreeLayoutScope is needed just because it makes so as of now. We could add
another value to MarkingBehavior enum as you and leviw were discussing in IRC.
It might look more reasonable?

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

https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:241:
ASSERT(!simplifiedLayout(nullptr));
On 2016/04/15 at 17:08:08, cbiesinger wrote:
> I don't really know much about SVG but this seems weird?

By "weird" do you mean the original code is weird or the change is weird?

I don't know much either, but LayoutSVGText does its layout by its own, using
platform/fonts and shaper directly, without using most of functions in
LayoutText, LineBreaker, etc.

As far as I debugged a few cases, all following 4 bits are false, so
simplifiedLayout() returns false immediately.
normalChildNeedsLayout()
posChildNeedsLayout()
needsSimplifiedNormalFlowLayout()
needsPositionedMovementLayout()

So if it's asserting that, use null SubtreeLayoutScope looks fine to me.

Can you elaborate a bit more?

[cbiesinger, 2016-04-15 18:26:46]

On 2016/04/15 17:58:44, kojii wrote:
> Thank you for your prompt review.
> 
>
https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp
(right):
> 
>
https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:212: //
> layout, but it's hard to get it here.
> On 2016/04/15 at 17:08:08, cbiesinger wrote:
> > Hmm yeah that one is tricky...
> 
> Yeah...or, what we really need is to avoid scheduleLayout() and
> SubtreeLayoutScope is needed just because it makes so as of now. We could add
> another value to MarkingBehavior enum as you and leviw were discussing in IRC.
> It might look more reasonable?

Yeah maybe. Probably best for a different patch.

>
https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp (right):
> 
>
https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:241:
> ASSERT(!simplifiedLayout(nullptr));
> On 2016/04/15 at 17:08:08, cbiesinger wrote:
> > I don't really know much about SVG but this seems weird?
> 
> By "weird" do you mean the original code is weird or the change is weird?
> 
> I don't know much either, but LayoutSVGText does its layout by its own, using
> platform/fonts and shaper directly, without using most of functions in
> LayoutText, LineBreaker, etc.
> 
> As far as I debugged a few cases, all following 4 bits are false, so
> simplifiedLayout() returns false immediately.
> normalChildNeedsLayout()
> posChildNeedsLayout()
> needsSimplifiedNormalFlowLayout()
> needsPositionedMovementLayout()
> 
> So if it's asserting that, use null SubtreeLayoutScope looks fine to me.
> 
> Can you elaborate a bit more?

I meant it's weird that it doesn't need an actual SubtreeLayoutScope. I'm not so
familiar with the text autosizer, does it not do anything in this case? I guess
in general it doesn't need one because it's a leaf node?

[kojii, 2016-04-18 04:31:51]

On 2016/04/15 at 18:26:46, cbiesinger wrote:
> On 2016/04/15 17:58:44, kojii wrote:
> > Thank you for your prompt review.
> > 
> >
https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp
(right):
> > 
> >
https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:212: //
> > layout, but it's hard to get it here.
> > On 2016/04/15 at 17:08:08, cbiesinger wrote:
> > > Hmm yeah that one is tricky...
> > 
> > Yeah...or, what we really need is to avoid scheduleLayout() and
> > SubtreeLayoutScope is needed just because it makes so as of now. We could
add
> > another value to MarkingBehavior enum as you and leviw were discussing in
IRC.
> > It might look more reasonable?
> 
> Yeah maybe. Probably best for a different patch.

Agreed. I'll try that first, as it makes some of changes in this CL not
necessary.

Put this CL on hold.

[cbiesinger, 2016-04-18 15:55:18]

On 2016/04/18 04:31:51, kojii wrote:
> On 2016/04/15 at 18:26:46, cbiesinger wrote:
> > On 2016/04/15 17:58:44, kojii wrote:
> > > Thank you for your prompt review.
> > > 
> > >
>
https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp
> (right):
> > > 
> > >
>
https://codereview.chromium.org/1887933002/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:212: //
> > > layout, but it's hard to get it here.
> > > On 2016/04/15 at 17:08:08, cbiesinger wrote:
> > > > Hmm yeah that one is tricky...
> > > 
> > > Yeah...or, what we really need is to avoid scheduleLayout() and
> > > SubtreeLayoutScope is needed just because it makes so as of now. We could
> add
> > > another value to MarkingBehavior enum as you and leviw were discussing in
> IRC.
> > > It might look more reasonable?
> > 
> > Yeah maybe. Probably best for a different patch.
> 
> Agreed. I'll try that first, as it makes some of changes in this CL not
> necessary.
> 
> Put this CL on hold.

Commented in the other CL. Let me know once/if you have a new version of this CL
to review!