Bug 102562 – Text Autosizing: Clusters should use width of LCA of their text nodes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[John Mellor]

Text Autosizing divides the page into clusters (see bug 97025). The contents of each cluster is autosized according to the width of the cluster. But sometimes the actual text within a cluster only occupies part of the width of the cluster. A particularly frequent example, is that the RenderView itself is often the cluster that contains much of the text on a page; yet many pages have borders or a max-width assigned, hence the actual text content within that page is significantly narrower than the RenderView. Thus you can zoom in on just the text content without needing to pan horizontally, so there is no need to multiply the font size by as much.

To solve this, when calculating the width of a cluster, we should first determine the "lowest common ancestor" of the first and last descendant text node of the cluster (i.e. the deepest wrapper block that contains all the text), and use its width instead of the width of the actual cluster node.

[Anton Vayvod]

Created attachment 176221 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 176221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review

Please fix style first, so style comments for now

> Source/WebCore/rendering/TextAutosizer.cpp:345
> +        blockWithAllText = lower->containingBlock();
> +    ASSERT(blockWithAllText->isDescendantOf(cluster));
> +    return blockWithAllText;

I would add a newline after the if-else block and one before return

> Source/WebCore/rendering/TextAutosizer.cpp:351
> +    return findFinalTextLeafNotInCluster(
> +        parent, depth, &RenderObject::firstChild, &RenderObject::nextSibling);

put that on one line, following normal coding style

> Source/WebCore/rendering/TextAutosizer.cpp:357
> +    return findFinalTextLeafNotInCluster(
> +        parent, depth, &RenderObject::lastChild, &RenderObject::previousSibling);

same

> Source/WebCore/rendering/TextAutosizer.cpp:364
> +RenderObject* TextAutosizer::findFinalTextLeafNotInCluster(
> +    RenderObject* parent,
> +    size_t& depth,
> +    RenderObject* (RenderObject::*getFirstChildMethod)() const,
> +    RenderObject* (RenderObject::*getNextSiblingMethod)() const)

this HAS to be on one line according to style

> Source/WebCore/rendering/TextAutosizer.cpp:375
> +        RenderObject* leaf = findFinalTextLeafNotInCluster(
> +            child, depth, getFirstChildMethod, getNextSiblingMethod);

same

> Source/WebCore/rendering/TextAutosizer.h:88
> +    static RenderObject* findFirstTextLeafNotInCluster(
> +        RenderObject* parent, size_t& depth);
> +    // Finds the last leaf text node child that doesn't belong to any cluster.
> +    static RenderObject* findLastTextLeafNotInCluster(
> +        RenderObject* parent, size_t& depth);
> +    static RenderObject* findFinalTextLeafNotInCluster(
> +        RenderObject* parent,
> +        size_t& depth,
> +        RenderObject* (RenderObject::*getFirstChildMethod)() const,
> +        RenderObject* (RenderObject::*getNextSiblingMethod)() const);

Wrong coding style. These should be on one line.

[Anton Vayvod]

Created attachment 176226 [details]
Patch

[Anton Vayvod]

Comment on attachment 176221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review

Fixed.

>> Source/WebCore/rendering/TextAutosizer.cpp:345
>> +    return blockWithAllText;
> 
> I would add a newline after the if-else block and one before return

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:351
>> +        parent, depth, &RenderObject::firstChild, &RenderObject::nextSibling);
> 
> put that on one line, following normal coding style

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:357
>> +        parent, depth, &RenderObject::lastChild, &RenderObject::previousSibling);
> 
> same

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:364
>> +    RenderObject* (RenderObject::*getNextSiblingMethod)() const)
> 
> this HAS to be on one line according to style

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:375
>> +            child, depth, getFirstChildMethod, getNextSiblingMethod);
> 
> same

Done.

>> Source/WebCore/rendering/TextAutosizer.h:88
>> +        RenderObject* (RenderObject::*getNextSiblingMethod)() const);
> 
> Wrong coding style. These should be on one line.

Done.

[Kenneth Rohde Christiansen]

Comment on attachment 176221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review

> Source/WebCore/rendering/TextAutosizer.cpp:302
> +RenderBlock* TextAutosizer::findDeepestBlockWithAllText(RenderBlock* cluster)

ContainingAllText ?

> Source/WebCore/rendering/TextAutosizer.cpp:319
> +    // Determine which node is lower.
> +    RenderObject* lower = firstTextLeaf;
> +    RenderObject* higher = lastTextLeaf;

I am not sure the lower,higher terminology is that clear

[Anton Vayvod]

Created attachment 176242 [details]
Patch

[Anton Vayvod]

Comment on attachment 176221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review

>> Source/WebCore/rendering/TextAutosizer.cpp:302
>> +RenderBlock* TextAutosizer::findDeepestBlockWithAllText(RenderBlock* cluster)
> 
> ContainingAllText ?

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:319
>> +    RenderObject* higher = lastTextLeaf;
> 
> I am not sure the lower,higher terminology is that clear

Changed to deep/shallow

[Kenneth Rohde Christiansen]

did John look at this yet?

[John Mellor]

(In reply to comment #8)
> did John look at this yet?

I discussed this several times during implementation but I'm still reviewing the code itself. Thanks for responding quickly as always!

[John Mellor]

Comment on attachment 176242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176242&action=review

General approach looks great; got some minor nits on the code. I haven't yet reviewed the tests - I'm doing that now.

> Source/WebCore/rendering/TextAutosizer.cpp:98
> +    // RenderView,instead of just taking the width of |cluster| we should find

Nits:
    s/RenderView,instead/RenderView, instead/
    s/we should find/we find/

> Source/WebCore/rendering/TextAutosizer.cpp:-105
> -    if (clusterShouldBeAutosized(lowestCommonAncestor, commonAncestorWidth)) {

Why did you change this? Presumably all the text within the cluster will be in the subtree descending from the LCA, so when we traverse the cluster looking for text we may as well only traverse that subtree?

> Source/WebCore/rendering/TextAutosizer.cpp:-229
> -    ASSERT(isAutosizingContainer(renderer));

What was the reason for changing this to a check? If this is just for the sake of findFinalTextLeafNotInCluster, I've left a comment below showing how you could tweak that so this ASSERT remains true.

> Source/WebCore/rendering/TextAutosizer.cpp:305
> +    if (cluster->isEmpty())

This isEmpty check seems unnecessary, since the next thing you do is call findFirstTextLeafNotInCluster which immediately returns 0 anyway (also causing you to return the cluster).

> Source/WebCore/rendering/TextAutosizer.cpp:322
> +    if (firstDepth < lastDepth) {

Rather than determining which node is deeper, it might be simpler to just repeat the equalizing depths code, e.g.:

// Equalize the depths if necessary.
while (firstDepth > lastDepth) {
    firstTextLeaf = firstTextLeaf->parent();
    --firstDepth;
}
while (firstDepth < lastDepth) {
    lastTextLeaf = lastTextLeaf->parent();
    --lastDepth;
}

You'd save 6 lines of code (and a dependency on std::swap) ;)
If you prefer not to do this, would you mind adding a #include <algorithm> in the spirit of http://code.google.com/p/include-what-you-use/wiki/WhyIWYU ?

> Source/WebCore/rendering/TextAutosizer.cpp:345
> +    ASSERT(blockContainingAllText->isDescendantOf(cluster));

Ultra minor nit: you could save a few lines by reordering this section:

ASSERT(deepNode->isDescendantOf(cluster));
if (deepNode->isRenderBlock())
    return toRenderBlock(deepNode);
else
    return deepNode->containingBlock();

But I don't mind if you think it was clearer the other way...

> Source/WebCore/rendering/TextAutosizer.cpp:360
> +const RenderObject* TextAutosizer::findFinalTextLeafNotInCluster(const RenderObject* parent, size_t& depth, RenderObject* (RenderObject::*getFirstChildMethod)() const, RenderObject* (RenderObject::*getNextSiblingMethod)() const)

The function pointers are a neat solution. But I wonder if it might be easier to read without them. Something like:

const RenderObject* TextAutosizer::findFirstTextLeafNotInCluster(const RenderObject* parent, size_t& depth, bool firstNotLast)
{
    ...
    const RenderObject* child = firstNotLast ? parent->firstChild() : parent->lastChild();
    while (child) {
        ...
        child = firstNotLast ? child->nextSibling() : child->previousSibling()
    }
    ....
}

> Source/WebCore/rendering/TextAutosizer.cpp:367
> +        if (child->isRenderBlock() && isAutosizingCluster(toRenderBlock(child)))

You can use:

if (isAutosizingContainer(child) && isAutosizingCluster(toRenderBlock(child)))

to avoid changing the ASSERT in isAutosizingCluster.

> LayoutTests/ChangeLog:9
> +        RenderView,instead of just taking the width of |cluster| we should find

Nit: s/RenderView,instead/RenderView, instead/

[Anton Vayvod]

Created attachment 176278 [details]
Patch

[Anton Vayvod]

Comment on attachment 176242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176242&action=review

>> Source/WebCore/rendering/TextAutosizer.cpp:98
>> +    // RenderView,instead of just taking the width of |cluster| we should find
> 
> Nits:
>     s/RenderView,instead/RenderView, instead/
>     s/we should find/we find/

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:-105
>> -    if (clusterShouldBeAutosized(lowestCommonAncestor, commonAncestorWidth)) {
> 
> Why did you change this? Presumably all the text within the cluster will be in the subtree descending from the LCA, so when we traverse the cluster looking for text we may as well only traverse that subtree?

I misinterpreted the code here.

>> Source/WebCore/rendering/TextAutosizer.cpp:-229
>> -    ASSERT(isAutosizingContainer(renderer));
> 
> What was the reason for changing this to a check? If this is just for the sake of findFinalTextLeafNotInCluster, I've left a comment below showing how you could tweak that so this ASSERT remains true.

The reason was also for the sake of less coupling (one always have to check for autosizing container before checking for cluster).
Since in some places it actually saves a check for container, changed it back.

>> Source/WebCore/rendering/TextAutosizer.cpp:305
>> +    if (cluster->isEmpty())
> 
> This isEmpty check seems unnecessary, since the next thing you do is call findFirstTextLeafNotInCluster which immediately returns 0 anyway (also causing you to return the cluster).

Removed.

>> Source/WebCore/rendering/TextAutosizer.cpp:322
>> +    if (firstDepth < lastDepth) {
> 
> Rather than determining which node is deeper, it might be simpler to just repeat the equalizing depths code, e.g.:
> 
> // Equalize the depths if necessary.
> while (firstDepth > lastDepth) {
>     firstTextLeaf = firstTextLeaf->parent();
>     --firstDepth;
> }
> while (firstDepth < lastDepth) {
>     lastTextLeaf = lastTextLeaf->parent();
>     --lastDepth;
> }
> 
> You'd save 6 lines of code (and a dependency on std::swap) ;)
> If you prefer not to do this, would you mind adding a #include <algorithm> in the spirit of http://code.google.com/p/include-what-you-use/wiki/WhyIWYU ?

Makes things shorter though less obvious that only one loop will actually execute (added a comment for that).
<algorithm> still needed since std::min and std::max are used.

>> Source/WebCore/rendering/TextAutosizer.cpp:345
>> +    ASSERT(blockContainingAllText->isDescendantOf(cluster));
> 
> Ultra minor nit: you could save a few lines by reordering this section:
> 
> ASSERT(deepNode->isDescendantOf(cluster));
> if (deepNode->isRenderBlock())
>     return toRenderBlock(deepNode);
> else
>     return deepNode->containingBlock();
> 
> But I don't mind if you think it was clearer the other way...

The assertion was to catch an case when containingBlock goes beyond the cluster, hence it's called after we may use this method.
I removed it completely (else return then triggered a warning so else went away as well).

>> Source/WebCore/rendering/TextAutosizer.cpp:360
>> +const RenderObject* TextAutosizer::findFinalTextLeafNotInCluster(const RenderObject* parent, size_t& depth, RenderObject* (RenderObject::*getFirstChildMethod)() const, RenderObject* (RenderObject::*getNextSiblingMethod)() const)
> 
> The function pointers are a neat solution. But I wonder if it might be easier to read without them. Something like:
> 
> const RenderObject* TextAutosizer::findFirstTextLeafNotInCluster(const RenderObject* parent, size_t& depth, bool firstNotLast)
> {
>     ...
>     const RenderObject* child = firstNotLast ? parent->firstChild() : parent->lastChild();
>     while (child) {
>         ...
>         child = firstNotLast ? child->nextSibling() : child->previousSibling()
>     }
>     ....
> }

Done. I used an enum instead of bool in an attempt to follow the style-guide:
http://www.webkit.org/coding/coding-style.html#names-enum-to-bool
I also used wrapper functions instead of ternary operator.

>> Source/WebCore/rendering/TextAutosizer.cpp:367
>> +        if (child->isRenderBlock() && isAutosizingCluster(toRenderBlock(child)))
> 
> You can use:
> 
> if (isAutosizingContainer(child) && isAutosizingCluster(toRenderBlock(child)))
> 
> to avoid changing the ASSERT in isAutosizingCluster.

Done.

>> LayoutTests/ChangeLog:9
>> +        RenderView,instead of just taking the width of |cluster| we should find
> 
> Nit: s/RenderView,instead/RenderView, instead/

Done.

[Anton Vayvod]

Created attachment 176280 [details]
Patch

[Anton Vayvod]

Created attachment 176288 [details]
Patch

[John Mellor]

View in context: https://bugs.webkit.org/attachment.cgi?id=176242&action=review

Some minor nits on the comments too. Thanks!

> LayoutTests/fast/text-autosizing/cluster-wide-in-narrow.html:29
> +    <div style="width: 600px">

I'm not quite sure why you're making this change now, since this patch doesn't affect this test (though I suspect your follow-on patch will :)

> LayoutTests/fast/text-autosizing/cluster-with-narrow-lca.html:23
> +<div style="width: 400px" >

Nit: s/" >/">/

> LayoutTests/fast/text-autosizing/cluster-with-narrow-lca.html:25
> +    This text should be autosized to just 20px computed font size (16 * 400/320), since the width of the least common ancestor is used for multiplier calculation.

It's not quite clear what least common ancestor refers to; perhaps "the width of the least common ancestor of the cluster's text descendants" would be clearer?

> LayoutTests/fast/text-autosizing/cluster-with-wide-lca.html:25
> +    This text should be autosized to 40px computed font size (16 * 800/320).

Please explain briefly why (ditto below).

> LayoutTests/fast/text-autosizing/em-margin-border-padding.html:24
> +    This text should get autosized to 35.2px computed font size (16 * 704/320), but the 1em margins, borders and padding should remain just 16px. While debatable, this generally looks more consistent than having them be scaled.<br>

Thanks for adding the equation here. It's mildly unfortunate that we have a fractional size though; if you can think of an easy way of tweaking the numbers so it comes out integral that would be nice (as it would reduce the likelihood of rounding bugs on different platforms). Though when changing this, please make sure the multiplier remains expressible in a small finite number of decimal places (not e.g. 2.3333333333).

> LayoutTests/fast/text-autosizing/wide-child-expected.html:15
> +	This text should not be autosized.

Nit: please add ", as this div is the lowest common ancestor of the root cluster, and this div is narrow" or similar. Also, a subtle gotcha with these tests is that Text Autosizing only autosizes clusters with enough text, so whenever you're asserting that something shouldn't be autosized, it's best for that something to have a decent amount of text so prevent passing the test for the wrong reason (i.e. accidentally passing the test because there wasn't enough text). So you might want to add a Lorem...aliqua line underneath this one (after a <br>).

> LayoutTests/fast/text-autosizing/wide-child-expected.html:17
> +    	This text should not be autosized as well since its width doesn't affect the width of the parent block.

Please explain why the parent block is significant.

> LayoutTests/fast/text-autosizing/wide-in-narrow-overflow-scroll.html:26
> +        This text should be autosized to 20px computed font size (16 * 400/320), since its scroll width is 400px.<br>

Could you clarify this. Something like "since this is part of the root cluster, whose text descendants are all contained within the 400px wide grandparent of this div."

[Anton Vayvod]

Since I failed to find these last comments in patch, answering in plain text:

> > LayoutTests/fast/text-autosizing/cluster-wide-in-narrow.html:29
> > +    <div style="width: 600px">
>
> I'm not quite sure why you're making this change now, since this patch doesn't affect this test (though I suspect your follow-on patch will :)

Reverted.

>
> > LayoutTests/fast/text-autosizing/cluster-with-narrow-lca.html:23
> > +<div style="width: 400px" >
>
> Nit: s/" >/">/

Done.

>
> > LayoutTests/fast/text-autosizing/cluster-with-narrow-lca.html:25
> > +    This text should be autosized to just 20px computed font size (16 * 400/320), since the width of the least common ancestor is used for multiplier calculation.
>
> It's not quite clear what least common ancestor refers to; perhaps "the width of the least common ancestor of the cluster's text descendants" would be clearer?

Agree. Done.

>
> > LayoutTests/fast/text-autosizing/cluster-with-wide-lca.html:25
> > +    This text should be autosized to 40px computed font size (16 * 800/320).
>
> Please explain briefly why (ditto below).

Done.

>
> > LayoutTests/fast/text-autosizing/em-margin-border-padding.html:24
> > +    This text should get autosized to 35.2px computed font size (16 * 704/320), but the 1em margins, borders and padding should remain just 16px. While debatable, this generally looks more consistent than having them be scaled.<br>
>
> Thanks for adding the equation here. It's mildly unfortunate that we have a fractional size though; if you can think of an easy way of tweaking the numbers so it comes out integral that would be nice (as it would reduce the likelihood of rounding bugs on different platforms). Though when changing this, please make sure the multiplier remains expressible in a small finite number of decimal places (not e.g. 2.3333333333).

Increased body width to 896 so that div width is 800.

>
> > LayoutTests/fast/text-autosizing/wide-child-expected.html:15
> > +     This text should not be autosized.
>
> Nit: please add ", as this div is the lowest common ancestor of the root cluster, and this div is narrow" or similar. Also, a subtle gotcha with these tests is that Text Autosizing only autosizes clusters with enough text, so whenever you're asserting that something shouldn't be autosized, it's best for that something to have a decent amount of text so prevent passing the test for the wrong reason (i.e. accidentally passing the test because there wasn't enough text). So you might want to add a Lorem...aliqua line underneath this one (after a <br>).

Done.
There actually was additional text, but after child div. Moved it to the front.
>
> > LayoutTests/fast/text-autosizing/wide-child-expected.html:17
> > +     This text should not be autosized as well since its width doesn't affect the width of the parent block.
>
> Please explain why the parent block is significant.

Done.
>
> > LayoutTests/fast/text-autosizing/wide-in-narrow-overflow-scroll.html:26
> > +        This text should be autosized to 20px computed font size (16 * 400/320), since its scroll width is 400px.<br>
>
> Could you clarify this. Something like "since this is part of the root cluster, whose text descendants are all contained within the 400px wide grandparent of this div."

Done.

[Anton Vayvod]

Created attachment 176342 [details]
Patch

[John Mellor]

Comment on attachment 176342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176342&action=review

Looking good. Final minor nits:

> Source/WebCore/ChangeLog:21
> +        (WebCore::TextAutosizer::isAutosizingCluster): replaced assertion for being autosizing container with a check

This comment is stale.

> Source/WebCore/rendering/TextAutosizer.cpp:337
> +    // We assume that containingBlock() can't skip the cluster, since as of now (11/27/2012) it skips

I just realized that this assumption isn't currently true - my bad for telling you it was earlier! For this to hold, you'll need to adjust isAutosizingContainer so that it returns true if renderer->isRenderBlock() && renderer->isOutOfFlowPositioned(), even if it is a list item.

Nit: I'd reword this comment to:

// containingBlock() should never leave the cluster, since it only skips ancestors when finding the
// container of position:absolute/fixed blocks, and those cannot exist within a cluster as
// isAutosizingCluster would have made them into their own independent cluster.

> Source/WebCore/rendering/TextAutosizer.cpp:374
> +const RenderObject* TextAutosizer::firstChild(const RenderObject* object, TraversalDirection direction)

These methods are confusingly named - it sounds like it returns the first child of the TextAutosizer (whatever that would mean).

Nit: They're also kinda verbose; it would probably be sufficient to do:

const RenderObject* child = direction == FirstToLast ? parent->firstChild() : parent->lastChild();

(since the enum lives in TextAutosizer.h, I wouldn't worry about people adding new entries to the enum without taking into account the uses in TextAutosizer.cpp)

[John Mellor]

Comment on attachment 176342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176342&action=review

> LayoutTests/fast/text-autosizing/cluster-with-narrow-lca-and-cluster-expected.html:14
> +<div style="width: 400px" >

Nit: please don't leave spaces between " and >, here and in your other test files (probably best to grep for /" >$/).

> LayoutTests/fast/text-autosizing/cluster-with-narrow-lca-and-cluster-expected.html:25
> +    Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.

Nit: please indent this the same as "This ...". And you probably want a <br> after "another cluster."

[Anton Vayvod]

Created attachment 176512 [details]
Patch

[Anton Vayvod]

Created attachment 176523 [details]
Patch

[Anton Vayvod]

Comment on attachment 176342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176342&action=review

>> Source/WebCore/ChangeLog:21
>> +        (WebCore::TextAutosizer::isAutosizingCluster): replaced assertion for being autosizing container with a check
> 
> This comment is stale.

Thanks! Fixed.

>> Source/WebCore/rendering/TextAutosizer.cpp:337
>> +    // We assume that containingBlock() can't skip the cluster, since as of now (11/27/2012) it skips
> 
> I just realized that this assumption isn't currently true - my bad for telling you it was earlier! For this to hold, you'll need to adjust isAutosizingContainer so that it returns true if renderer->isRenderBlock() && renderer->isOutOfFlowPositioned(), even if it is a list item.
> 
> Nit: I'd reword this comment to:
> 
> // containingBlock() should never leave the cluster, since it only skips ancestors when finding the
> // container of position:absolute/fixed blocks, and those cannot exist within a cluster as
> // isAutosizingCluster would have made them into their own independent cluster.

Fixed. Added a test for this case.

>> Source/WebCore/rendering/TextAutosizer.cpp:374
>> +const RenderObject* TextAutosizer::firstChild(const RenderObject* object, TraversalDirection direction)
> 
> These methods are confusingly named - it sounds like it returns the first child of the TextAutosizer (whatever that would mean).
> 
> Nit: They're also kinda verbose; it would probably be sufficient to do:
> 
> const RenderObject* child = direction == FirstToLast ? parent->firstChild() : parent->lastChild();
> 
> (since the enum lives in TextAutosizer.h, I wouldn't worry about people adding new entries to the enum without taking into account the uses in TextAutosizer.cpp)

I guess I kept trying to keep this optimized (i.e. with __builtin_unreachable or __assume).
Inlined them in the calling function.

>> LayoutTests/fast/text-autosizing/cluster-with-narrow-lca-and-cluster-expected.html:14
>> +<div style="width: 400px" >
> 
> Nit: please don't leave spaces between " and >, here and in your other test files (probably best to grep for /" >$/).

Done.

>> LayoutTests/fast/text-autosizing/cluster-with-narrow-lca-and-cluster-expected.html:25
>> +    Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
> 
> Nit: please indent this the same as "This ...". And you probably want a <br> after "another cluster."

Done.

[John Mellor]

Comment on attachment 176523 [details]
Patch

Patch looks good to me. Kenneth/Julien, is this ready for r+ ?

[Anton Vayvod]

Comment on attachment 176523 [details]
Patch

Could someone cq+ this, please?

[WebKit Review Bot]

Comment on attachment 176523 [details]
Patch

Clearing flags on attachment: 176523

Committed r136047: <http://trac.webkit.org/changeset/136047>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.