Autosize list markers.

Autosize list markers.

The RenderListMarker is added to the render tree during layout by RenderListItem::updateMarkerLocation, and it must be autosized *before* this happens.  To facilitate this we add FastTextAutosizer::inflateListItem.

To ensure we have a valid multiplier in inflateListItem, the logic for findDeepestBlockContainingAllText is modified to treat list items as text (with a corresponding test).

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

[skobes, 2014-01-24 01:14:41]

list-marker-with-images-and-forms-autosizing.html is still failing due to the
form input being autosized.  I'll fix that separately.

[skobes, 2014-01-25 00:31:49]

This is ready for review.

I tried to fix the margin issue by calling updateLogicalWidth() when the <ul> /
<ol> element hits beginLayout(), but unfortunately it is not so simple because
getMaxWidthListMarker() depends on the children being already laid out.  It will
need some refactoring to work in a single pass, which I would prefer to do in a
separate change.

[pdr., 2014-01-25 03:50:23]

This is turning out to be much harder than I expected. I think the current
approach has some issues but I don't have a great alternative suggestion.

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:97: 
Do we need to skip inflation here for list items we've already inflated with
inflateListItem?

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:104: #ifndef NDEBUG
These ifndef's aren't needed because the asserts will be compiled out
automatically in debug builds. I'm a hypocrite and did the same thing in
destroy() by mistake :/

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:111: applyMultiplier(listItem,
activeMultiplier());
I'm not sure we can call activeMultiplier() here. We only have a valid
multiplier when we know the cluster's root has begun layout, and the lowest
block containing all text has also begun layout. In practice, this means we can
only access the multiplier if we're within a line of text because we know at
that point that both the root and the lowest block containing all text are above
us, and therefore all in layout with valid contentLogicalWidth.

For example:
<ul clusterRoot><li><div lowestBlockContainingAllText>foo</div></li></ul>

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:133: return cluster->m_autosize ?
cluster->m_multiplier : 1.0f;
We should add an assert that the multiplier has been calculated already:
ASSERT(cluster->m_autosize ? cluster->m_multiplier : true);
Or possibly just:
ASSERT(cluster->m_multiplier);

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.h (right):

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.h:68: void inflateListItem(RenderBlock*,
RenderObject*);
This should probably be: inflateListItem(RenderListItem*, RenderListMarker*);

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderListItem.cpp (right):

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderListItem.cpp:328: RenderBlockFlow::layout();
This layout call updates the RenderListItem's contentLogicalWidth which the
cluster multiplier may depend on.

[skobes, 2014-01-28 03:10:30]

Thanks! PTAL.

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:97: 
On 2014/01/25 03:50:23, pdr wrote:
> Do we need to skip inflation here for list items we've already inflated with
> inflateListItem?

No, inflateListItem only sets the multiplier on the LI itself and the marker. 
We still want to go in and process the stuff inside the LI.

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:104: #ifndef NDEBUG
On 2014/01/25 03:50:23, pdr wrote:
> These ifndef's aren't needed because the asserts will be compiled out
> automatically in debug builds. I'm a hypocrite and did the same thing in
> destroy() by mistake :/

Done.

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:111: applyMultiplier(listItem,
activeMultiplier());
On 2014/01/25 03:50:23, pdr wrote:
> I'm not sure we can call activeMultiplier() here. We only have a valid
> multiplier when we know the cluster's root has begun layout, and the lowest
> block containing all text has also begun layout. In practice, this means we
can
> only access the multiplier if we're within a line of text because we know at
> that point that both the root and the lowest block containing all text are
above
> us, and therefore all in layout with valid contentLogicalWidth.
> 
> For example:
> <ul clusterRoot><li><div lowestBlockContainingAllText>foo</div></li></ul>

Fixed.

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:133: return cluster->m_autosize ?
cluster->m_multiplier : 1.0f;
On 2014/01/25 03:50:23, pdr wrote:
> We should add an assert that the multiplier has been calculated already:
> ASSERT(cluster->m_autosize ? cluster->m_multiplier : true);
> Or possibly just:
> ASSERT(cluster->m_multiplier);

Done.

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.h (right):

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.h:68: void inflateListItem(RenderBlock*,
RenderObject*);
On 2014/01/25 03:50:23, pdr wrote:
> This should probably be: inflateListItem(RenderListItem*, RenderListMarker*);

Even if we don't need anything from those classes, and wouldn't otherwise need
to #include their headers?

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderListItem.cpp (right):

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderListItem.cpp:328: RenderBlockFlow::layout();
On 2014/01/25 03:50:23, pdr wrote:
> This layout call updates the RenderListItem's contentLogicalWidth which the
> cluster multiplier may depend on.

Hmm... this would only be an issue if the LI had a different width from the
containing OL/UL, right?  Hopefully not very common (and the consequence is just
that the marker's size is based on the latter).

[pdr., 2014-01-28 04:50:45]

Awesome! LGTM with a few changes.

Please improve the change description to better describe what's going on here,
and to include the bug number (BUG=302005 if there isn't an existing one).

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.h (right):

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.h:68: void inflateListItem(RenderBlock*,
RenderObject*);
On 2014/01/28 03:10:30, skobes wrote:
> On 2014/01/25 03:50:23, pdr wrote:
> > This should probably be: inflateListItem(RenderListItem*,
RenderListMarker*);
> 
> Even if we don't need anything from those classes, and wouldn't otherwise need
> to #include their headers?

Yes, type safety > compile time :)

There's no need to include the headers in FastTextAutosizer.h, just
forward-declare the classes like so:
class RenderBlock;
class RenderListItem;
class RenderListMarker;

You will need to include the RenderListItem and RenderListMarker headers in
FastTextAutosizer.cpp though.

https://codereview.chromium.org/145003009/diff/210001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/145003009/diff/210001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:107: 
currentCluster() can be null here. Probably want to check for that and return.

https://codereview.chromium.org/145003009/diff/210001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:111: float multiplier =
clusterMultiplier(currentCluster(), listItem);
I like the idea here except for passing listItem in to clusterMultiplier because
it complicates the code quite a bit.

Instead, can you change findTextLeaf to look like:
const RenderObject* FastTextAutosizer::findTextLeaf(const RenderObject* parent,
size_t& depth, TextLeafSearch firstOrLast)
{
    if (parent->isEmpty())
        return parent->isText() || parent->isListItem() ? parent : 0;
...

This should have the same effect but with much less code. I verified that all
tests pass with this change.

[skobes, 2014-01-29 00:13:29]

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.h (right):

https://codereview.chromium.org/145003009/diff/110001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.h:68: void inflateListItem(RenderBlock*,
RenderObject*);
On 2014/01/28 04:50:46, pdr wrote:
> On 2014/01/28 03:10:30, skobes wrote:
> > On 2014/01/25 03:50:23, pdr wrote:
> > > This should probably be: inflateListItem(RenderListItem*,
> RenderListMarker*);
> > 
> > Even if we don't need anything from those classes, and wouldn't otherwise
need
> > to #include their headers?
> 
> Yes, type safety > compile time :)
> 
> There's no need to include the headers in FastTextAutosizer.h, just
> forward-declare the classes like so:
> class RenderBlock;
> class RenderListItem;
> class RenderListMarker;
> 
> You will need to include the RenderListItem and RenderListMarker headers in
> FastTextAutosizer.cpp though.

Done.

https://codereview.chromium.org/145003009/diff/210001/Source/core/rendering/F...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/145003009/diff/210001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:107: 
On 2014/01/28 04:50:47, pdr wrote:
> currentCluster() can be null here. Probably want to check for that and return.

Done, with FIXME to investigate further.

https://codereview.chromium.org/145003009/diff/210001/Source/core/rendering/F...
Source/core/rendering/FastTextAutosizer.cpp:111: float multiplier =
clusterMultiplier(currentCluster(), listItem);
On 2014/01/28 04:50:47, pdr wrote:
> I like the idea here except for passing listItem in to clusterMultiplier
because
> it complicates the code quite a bit.

Fixed.

[pdr., 2014-01-29 00:18:16]

LGTM, looks great

https://codereview.chromium.org/145003009/diff/250001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/145003009/diff/250001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:810: crbug.com/302005
fast/text-autosizing/list-item-above-dbcat.html [ NeedsRebaseline ]
Can you just mark this as "Failure"?

[skobes, 2014-01-29 00:22:22]

https://codereview.chromium.org/145003009/diff/250001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/145003009/diff/250001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:810: crbug.com/302005
fast/text-autosizing/list-item-above-dbcat.html [ NeedsRebaseline ]
On 2014/01/29 00:18:17, pdr wrote:
> Can you just mark this as "Failure"?

Done.

[commit-bot: I haz the power, 2014-01-29 00:22:46]

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

[commit-bot: I haz the power, 2014-01-29 03:11:53]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-29 03:12:32]

CQ bit was unchecked on CL. Ignoring.

[pdr., 2014-01-29 03:15:15]

On 2014/01/29 03:12:32, I haz the power (commit-bot) wrote:
> CQ bit was unchecked on CL. Ignoring.

Darn, I think the fast/text-autosizing test needs to be [ ImageOnlyFailure ].

[commit-bot: I haz the power, 2014-01-29 17:30:58]

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

[commit-bot: I haz the power, 2014-01-29 20:04:48]

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

[commit-bot: I haz the power, 2014-01-29 23:06:39]

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

[commit-bot: I haz the power, 2014-01-29 23:15:57]

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

[commit-bot: I haz the power, 2014-01-30 00:13:11]

Change committed as 166068

[commit-bot: I haz the power, 2014-01-30 00:17:50]

CQ bit was unchecked on CL. Ignoring.