Bug 103627 – Text Autosizing: containers wider than their enclosing clusters should be autosized as separate clusters

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Anton Vayvod]

Text Autosizing: text nodes wider than their ancestor clusters should be autosized as separate clusters

[Anton Vayvod]

Created attachment 176705 [details]
Patch

Some blocks of texts might be wider than their parent clusters and need to be autosized separately.

[John Mellor]

Comment on attachment 176705 [details]
Patch

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

Some initial comments. Haven't completely finished going through this.

> Source/WebCore/ChangeLog:3
> +        Text Autosizing: text nodes wider than their ancestor clusters should be autosized as separate clusters

Nit: s/text nodes/containers/

> Source/WebCore/ChangeLog:8
> +        isAutosizingCluster() is checking for the width of the container being greater than the width of its

Nit: s/is checking/now checks/

> Source/WebCore/ChangeLog:9
> +        descendant cluster (more precisely, its text nodes least common ancestor). This ancestor RenderBlock is

s/descendant/enclosing/
s/its text nodes least common ancestor/the lowest common ancestor of the text nodes of its enclosing cluster/
s/ancestor/enclosing/

> Source/WebCore/ChangeLog:11
> +        Algorithm that searches for text nodes least common ancestor had to be modified to avoid circular dependency

s/least/lowest/ (these sound similar but are opposites!)
Nit: s/Algorithm/The algorithm/

> Source/WebCore/rendering/TextAutosizer.cpp:91
> +    processCluster(cluster, container, cluster, layoutRoot, windowInfo);

Shouldn't this pass 0 instead of cluster as the stayWithin?

> Source/WebCore/rendering/TextAutosizer.cpp:95
> +void TextAutosizer::processCluster(RenderBlock* cluster, RenderBlock* container, const RenderBlock* stayWithin, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)

It's somewhat confusing calling this stayWithin when there's already a subtreeRoot. And it's not really a stayWithin, since its purpose isn't to prevent the recursion from going higher than the cluster. How about: parentClusterLCA? Or parentTextWrapper? Or parentBlockContainingAllText?

> Source/WebCore/rendering/TextAutosizer.cpp:142
> +void TextAutosizer::processContainer(float multiplier, RenderBlock* container, const RenderBlock* stayWithin, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)

Ditto, how about: clusterLca? Or clusterTextWrapper? Or clusterBlockContainingAllText?

> Source/WebCore/rendering/TextAutosizer.cpp:214
> +bool TextAutosizer::isAutosizingCluster(const RenderBlock* renderer, const RenderBlock* stayWithin)

Ditto rename stayWithin.

> Source/WebCore/rendering/TextAutosizer.cpp:267
> +    measureDescendantTextWidth(lowestCommonAncestor, lowestCommonAncestor, minTextWidth, textWidth);

You renamed lowestCommonAncestor to deepestBlockContainingAllText above. I don't mind what its called, but it would be nice to be consistent.

> Source/WebCore/rendering/TextAutosizer.cpp:273
> +void TextAutosizer::measureDescendantTextWidth(const RenderBlock* stayWithin, const RenderBlock* container, float minTextWidth, float& textWidth)

Ditto rename stayWithin.

> Source/WebCore/rendering/TextAutosizer.cpp:308
> +const RenderBlock* TextAutosizer::findDeepestBlockContainingAllText(const RenderBlock* subtreeRoot)

Why did you rename this parameter? You still pass in a cluster when you call this function. And it's confusing to call it subtreeRoot as that means something different in process cluster.

> Source/WebCore/rendering/TextAutosizer.cpp:-308
> -    ASSERT(isAutosizingCluster(cluster));

And presumably you can keep this assert.

> Source/WebCore/rendering/TextAutosizer.cpp:311
> +    const RenderObject* firstTextLeaf = findFirstTextLeafNotInCluster(FirstToLast, firstPath);

Now that you have the paths, you never actually use firstTextLeaf and lastTextLeaf except a couple of times to check if they're null. Could you get rid of these completely, and just do some check based on the length of the path?

> Source/WebCore/rendering/TextAutosizer.cpp:412
> +bool TextAutosizer::isObjectAutosizingCluster(const RenderObject* object, const RenderBlock* stayWithin)

I'd rather this was called isAutosizingContainerAndCluster for consistency with isAutosizingContainer/Cluster. And could you put this next to those two methods? Finally, you could probably use this in processSubtree too.

> Source/WebCore/rendering/TextAutosizer.h:74
> +    // Returns true if the specified autosizing container is also a cluster. If |stayWithin| is specified and |container| is wider,

Nit: s/wider/wider than |stayWithin|/

> Source/WebCore/rendering/TextAutosizer.h:78
> +    static bool clusterShouldBeAutosized(const RenderBlock* cluster, float clusterWidth);

s/\bcluster\b/deepestBlockContainingAllText/ ? That's what you seem to pass in. Also please make sure the argument names here match those in the .cpp.

> Source/WebCore/rendering/TextAutosizer.h:101
> +    static const RenderObject* firstChildNotCluster(TraversalDirection, const RenderObject*);

I'm not so keen on the fact that this change introduces 7 new methods, 3 of which (firstChildNotCluster, nextSiblingNotCluster and objectFirstChild) are only used once. It makes it a bit harder to follow what Text Autosizing is doing, as the header file is getting quite overwhelming...

[Anton Vayvod]

Created attachment 177270 [details]
Patch

[Anton Vayvod]

Comment on attachment 176705 [details]
Patch

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

>> Source/WebCore/rendering/TextAutosizer.cpp:91
>> +    processCluster(cluster, container, cluster, layoutRoot, windowInfo);
> 
> Shouldn't this pass 0 instead of cluster as the stayWithin?

Changed. Actually it doesn't matter as long as the assertion within processCluster doesn't fail.

>> Source/WebCore/rendering/TextAutosizer.cpp:95
>> +void TextAutosizer::processCluster(RenderBlock* cluster, RenderBlock* container, const RenderBlock* stayWithin, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)
> 
> It's somewhat confusing calling this stayWithin when there's already a subtreeRoot. And it's not really a stayWithin, since its purpose isn't to prevent the recursion from going higher than the cluster. How about: parentClusterLCA? Or parentTextWrapper? Or parentBlockContainingAllText?

blockContainingAllText

>> Source/WebCore/rendering/TextAutosizer.cpp:308
>> +const RenderBlock* TextAutosizer::findDeepestBlockContainingAllText(const RenderBlock* subtreeRoot)
> 
> Why did you rename this parameter? You still pass in a cluster when you call this function. And it's confusing to call it subtreeRoot as that means something different in process cluster.

Was work-in-progress rename. Reverted.

>> Source/WebCore/rendering/TextAutosizer.cpp:-308
>> -    ASSERT(isAutosizingCluster(cluster));
> 
> And presumably you can keep this assert.

Yes.

>> Source/WebCore/rendering/TextAutosizer.cpp:311
>> +    const RenderObject* firstTextLeaf = findFirstTextLeafNotInCluster(FirstToLast, firstPath);
> 
> Now that you have the paths, you never actually use firstTextLeaf and lastTextLeaf except a couple of times to check if they're null. Could you get rid of these completely, and just do some check based on the length of the path?

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:412
>> +bool TextAutosizer::isObjectAutosizingCluster(const RenderObject* object, const RenderBlock* stayWithin)
> 
> I'd rather this was called isAutosizingContainerAndCluster for consistency with isAutosizingContainer/Cluster. And could you put this next to those two methods? Finally, you could probably use this in processSubtree too.

I think this one should actually be isAutosizingCluster since it checks it for any object.
Old isAutosizingCluster checks only containers (or fails) so I renamed it to isContainerAutosizingCluster.
How does it look?

>> Source/WebCore/rendering/TextAutosizer.h:101
>> +    static const RenderObject* firstChildNotCluster(TraversalDirection, const RenderObject*);
> 
> I'm not so keen on the fact that this change introduces 7 new methods, 3 of which (firstChildNotCluster, nextSiblingNotCluster and objectFirstChild) are only used once. It makes it a bit harder to follow what Text Autosizing is doing, as the header file is getting quite overwhelming...

Combined firstChildNotCluster and nextSiblingNotCluster since there was much common code.
Since firstChild and nextSibling are very similar here (both mean the next node in traversal) it seems not very consistent to me to inline some logic about one while moving the same logic about another to a function.
As for the header getting bigger, I'd happily move all private static functions to anonymous namespace in .cpp.

[John Mellor]

Comment on attachment 177270 [details]
Patch

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

A few more comments:

> Source/WebCore/ChangeLog:9
> +        common ancestor of the text nodes of the enclosing cluster). The enclosing RenderBlock is passed throughout

Error: mismatched bracket :)

Also, perhaps replace "The enclosing ... traversal methods." with "This blockContainingAllText is passed to all the tree traversal methods." ?

> Source/WebCore/ChangeLog:11
> +        The algorithm that searches for text nodes lowest common ancestor had to be modified to avoid circular dependency

Some minor grammar nits:
  s/text nodes lowest common ancestor/the lowest common ancestor of descendant text nodes/
  s/circular dependency/a circular dependency/
  s/finding LCA/finding the LCA/
  s/if container/whether a given container/
  s/initial LCA/the initial LCA/
  s/of text leafs'/of the text leafs'/
  s/new LCA/a new LCA/
  s/in paths/in the paths/
  s/from LCA/from the LCA/

> Source/WebCore/rendering/TextAutosizer.cpp:235
> +    // If a container is wider than its |blockContainingAllText| parent, it becomes a cluster.

Nit: please wrap this to match the rest of the comment.

> Source/WebCore/rendering/TextAutosizer.cpp:289
> +                measureDescendantTextWidth(blockContainingAllText, descendantBlock, minTextWidth, textWidth);

Nit: isContainerAutosizingCluster takes container then blockContainingAllText parameters, whereas measureDescendantTextWidth takes blockContainingAllText then container. It would be nice if this were consistent, e.g. swap the first two parameters to measureDescendantTextWidth.

> Source/WebCore/rendering/TextAutosizer.cpp:297
> +RenderObject* TextAutosizer::nextInPreOrderSkippingDescendantsOfContainers(const RenderObject* current, const RenderObject* blockContainingAllText)

This should still be called stayWithin. I suspect you grep'd overzealously :)

> Source/WebCore/rendering/TextAutosizer.cpp:315
> +    ASSERT(isContainerAutosizingCluster(cluster));

Isn't this missing a parent blockContainingAllText parameter?

> Source/WebCore/rendering/TextAutosizer.cpp:338
> +        // Check if any of text leafs parents are going to be clusters due to being wider than the block just found.

Minor nit: s/text leafs/the text leafs'/

> Source/WebCore/rendering/TextAutosizer.cpp:368
> +void TextAutosizer::findFirstTextLeafNotInCluster(TraversalDirection direction, RenderObjects& path)

This method used to be easy to follow, but has gotten rather more complicated. findFirstTextLeafNotInCluster now calls findNextTextLeafNotInCluster which in turn calls back to findFirstTextLeafNotInCluster, so there are nested while loops of alternating different kinds. I wonder if this could be made easier. For example unless I'm missing something, findFirstTextLeafNotInCluster could be written as follows with no new dependencies (based heavily on the existing implementation of findFirstTextLeafNotInCluster added by webkit.org/b/102562, with the ++depth and --depth replaced by pushing and popping children):

void TextAutosizer::findFirstTextLeafNotInCluster(TraversalDirection direction, RenderObjects& path)
{
    const RenderObject* child = (direction == FirstToLast) ? path.last()->firstChild() : path.last()->lastChild();
    while (child) {
        if (!isAutosizingCluster(child, 0)) {
            path.append(child);
            findFirstTextLeafNotInCluster(direction, path);
            if (path.last()->isText())
                return;
            path.removeLast();
        }
        child = (direction == FirstToLast) ? child->nextSibling() : child->previousSibling();
    }
    // Leave path empty if search failed.
    if (path.size() == 1)
        path.clear();
}

What do you think?

> Source/WebCore/rendering/TextAutosizer.cpp:375
> +            return findNextTextLeafNotInCluster(direction, path);

Nit: Since this is a void method, I'd prefer you write this on 2 lines:

    findNextTextLeafNotInCluster(direction, path);
    return;

Or in this case:

    if (!current->isText())
        findNextTextLeafNotInCluster(direction, path);
    return;

Ditto twice below.

[Anton Vayvod]

Created attachment 177841 [details]
Patch

[Anton Vayvod]

Comment on attachment 177270 [details]
Patch

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

I reduced the number of methods and then moved private static methods over to .cpp file wrapped in anonymous namespace.

>> Source/WebCore/ChangeLog:9
>> +        common ancestor of the text nodes of the enclosing cluster). The enclosing RenderBlock is passed throughout
> 
> Error: mismatched bracket :)
> 
> Also, perhaps replace "The enclosing ... traversal methods." with "This blockContainingAllText is passed to all the tree traversal methods." ?

Done.

>> Source/WebCore/ChangeLog:11
>> +        The algorithm that searches for text nodes lowest common ancestor had to be modified to avoid circular dependency
> 
> Some minor grammar nits:
>   s/text nodes lowest common ancestor/the lowest common ancestor of descendant text nodes/
>   s/circular dependency/a circular dependency/
>   s/finding LCA/finding the LCA/
>   s/if container/whether a given container/
>   s/initial LCA/the initial LCA/
>   s/of text leafs'/of the text leafs'/
>   s/new LCA/a new LCA/
>   s/in paths/in the paths/
>   s/from LCA/from the LCA/

Done. Thanks for fixing the articles.

>> Source/WebCore/rendering/TextAutosizer.cpp:235
>> +    // If a container is wider than its |blockContainingAllText| parent, it becomes a cluster.
> 
> Nit: please wrap this to match the rest of the comment.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:289
>> +                measureDescendantTextWidth(blockContainingAllText, descendantBlock, minTextWidth, textWidth);
> 
> Nit: isContainerAutosizingCluster takes container then blockContainingAllText parameters, whereas measureDescendantTextWidth takes blockContainingAllText then container. It would be nice if this were consistent, e.g. swap the first two parameters to measureDescendantTextWidth.

Good catch, I actually mixed it up here, it's the other way around in the header.

>> Source/WebCore/rendering/TextAutosizer.cpp:297
>> +RenderObject* TextAutosizer::nextInPreOrderSkippingDescendantsOfContainers(const RenderObject* current, const RenderObject* blockContainingAllText)
> 
> This should still be called stayWithin. I suspect you grep'd overzealously :)

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:315
>> +    ASSERT(isContainerAutosizingCluster(cluster));
> 
> Isn't this missing a parent blockContainingAllText parameter?

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:338
>> +        // Check if any of text leafs parents are going to be clusters due to being wider than the block just found.
> 
> Minor nit: s/text leafs/the text leafs'/

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:368
>> +void TextAutosizer::findFirstTextLeafNotInCluster(TraversalDirection direction, RenderObjects& path)
> 
> This method used to be easy to follow, but has gotten rather more complicated. findFirstTextLeafNotInCluster now calls findNextTextLeafNotInCluster which in turn calls back to findFirstTextLeafNotInCluster, so there are nested while loops of alternating different kinds. I wonder if this could be made easier. For example unless I'm missing something, findFirstTextLeafNotInCluster could be written as follows with no new dependencies (based heavily on the existing implementation of findFirstTextLeafNotInCluster added by webkit.org/b/102562, with the ++depth and --depth replaced by pushing and popping children):
> 
> void TextAutosizer::findFirstTextLeafNotInCluster(TraversalDirection direction, RenderObjects& path)
> {
>     const RenderObject* child = (direction == FirstToLast) ? path.last()->firstChild() : path.last()->lastChild();
>     while (child) {
>         if (!isAutosizingCluster(child, 0)) {
>             path.append(child);
>             findFirstTextLeafNotInCluster(direction, path);
>             if (path.last()->isText())
>                 return;
>             path.removeLast();
>         }
>         child = (direction == FirstToLast) ? child->nextSibling() : child->previousSibling();
>     }
>     // Leave path empty if search failed.
>     if (path.size() == 1)
>         path.clear();
> }
> 
> What do you think?

I figured out how to use just one (hopefully still easy to follow) method.

>> Source/WebCore/rendering/TextAutosizer.cpp:375
>> +            return findNextTextLeafNotInCluster(direction, path);
> 
> Nit: Since this is a void method, I'd prefer you write this on 2 lines:
> 
>     findNextTextLeafNotInCluster(direction, path);
>     return;
> 
> Or in this case:
> 
>     if (!current->isText())
>         findNextTextLeafNotInCluster(direction, path);
>     return;
> 
> Ditto twice below.

Done.

[John Mellor]

(In reply to comment #7)
> (From update of attachment 177270 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177270&action=review
> 
> I reduced the number of methods and then moved private static methods over to .cpp file wrapped in anonymous namespace.

I'm finding it really hard to review this patch, as the diff just shows that the whole file has changed so I can't see what's changed within the bits that have moved around.

Would you mind re-uploading this without moving things around, and then doing the moving as a follow-up patch (which only moves things, without changing any of the code)? Thanks!

[Anton Vayvod]

Created attachment 177998 [details]
Patch

[Anton Vayvod]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 177270 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177270&action=review
> > 
> > I reduced the number of methods and then moved private static methods over to .cpp file wrapped in anonymous namespace.
> 
> I'm finding it really hard to review this patch, as the diff just shows that the whole file has changed so I can't see what's changed within the bits that have moved around.
> 
> Would you mind re-uploading this without moving things around, and then doing the moving as a follow-up patch (which only moves things, without changing any of the code)? Thanks!

Done.

[John Mellor]

Comment on attachment 177998 [details]
Patch

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

Thanks, that's much clearer now!

> Source/WebCore/rendering/TextAutosizer.cpp:239
> +    // If a container is wider than its |parentBlockContainingAllText| parent, it

Nit - might be slightly clearer as: "Additionally, any containers that are wider than the |blockContainingAllText| of their enclosing cluster also become clusters, since they need special treatment due to their width."

> Source/WebCore/rendering/TextAutosizer.cpp:303
> +// Use to traverse the tree of descendants, excluding descendants of containers (but returning the containers themselves).

This comment is already in the .h file?

> Source/WebCore/rendering/TextAutosizer.cpp:381
> +bool TextAutosizer::correctFirstTextLeafIfInCluster(TraversalDirection direction, RenderObjects& path, size_t startFrom, const RenderBlock* parentBlockContainingAllText)

Cool, these are a lot easier to follow now - thanks!

> Source/WebCore/rendering/TextAutosizer.cpp:395
> +    findFirstTextLeafNotInClusterStartingFrom(direction, path, objectNextSibling(direction, cluster));

What happens if the cluster doesn't have a next sibling? objectNextSibling will return 0, hence findFirstTextLeafNotInClusterStartingFrom will just pop the last entry of path. But startFrom could have other descendants that are text leaves, and that we are ignoring...

Incidentally, if the tests didn't catch this, might want to add another test ;)

> Source/WebCore/rendering/TextAutosizer.h:33
>  #include <wtf/PassRefPtr.h>

While you're at it, looks like you can remove this include?

> Source/WebCore/rendering/TextAutosizer.h:57
> +    typedef Vector<const RenderObject*> RenderObjects;

It seems you only use "RenderObjects&" in the .h file. Would it be possible to move this typedef to the .cpp and forward declare it in the .h file, such that you can also move the Vector include to the .cpp file? Though if you're going to move all of these to the .cpp file anyway, it might not be worth spending too long on this :)

> Source/WebCore/rendering/TextAutosizer.h:66
> +    void processContainer(float multiplier, RenderBlock* container, const RenderBlock* parentBlockContainingAllText, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&);

Please sync up your parameter names. In the header file both processCluster and processContainer take a parentBlockContainingAllText, but in the cpp file they both take a blockContainingAllText. I would suggest that instead processCluster takes a parentBlockContainingAllText (in both) and processContainer takes a blockContainingAllText (in both), since conceptually processContainer is passed the blockContainingAllText of the current cluster (rather than of the parent cluster).

This would also allow you to rename deepestBlockContainingAllText in processCluster to blockContainingAllText, which might be more consistent.

> Source/WebCore/rendering/TextAutosizer.h:86
> +    // returns the lowest common ancestor for these text nodes. Otherwise, returns the cluster itself.

Tiny nit: s/for/of/

> Source/WebCore/rendering/TextAutosizer.h:90
> +    // the given child of the last object in the path. Follows the given tree traversal direction. If such text leaf is not found,

It's not clear what happens if the search succeeds. How about replacing the last sentence with: "If such a text leaf is found, the path will be replaced with the path to the text leaf; otherwise the last object will be removed from the path." ?

> Source/WebCore/rendering/TextAutosizer.h:95
> +    // If cluster is not found, returns false. Otherwise, removes every node in the path including the found cluster and continues

"removes every node in the path including the found cluster" suggests that it completely clears the path, but it only removes the cluster and entries after (with greater index than) the cluster. How about "If no cluster is found, this returns false. Otherwise, it removes the found cluster and all subsequent nodes from the path, then continues traversing the tree to find the next text leaf in the given direction, and finally returns true."

> Source/WebCore/rendering/TextAutosizer.h:97
> +    static bool correctFirstTextLeafIfInCluster(TraversalDirection, RenderObjects& path, size_t startFrom, const RenderBlock* parentBlockContainingAllText);

Please mention this method in the ChangeLog. Also, "adjust" might be clearer than "correct", as correct has multiple meanings and people might interpret it as asking a question, "is this the correct first text leaf" or something... :)

[Anton Vayvod]

Created attachment 178210 [details]
Patch

[Anton Vayvod]

Comment on attachment 177998 [details]
Patch

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

>> Source/WebCore/rendering/TextAutosizer.cpp:239
>> +    // If a container is wider than its |parentBlockContainingAllText| parent, it
> 
> Nit - might be slightly clearer as: "Additionally, any containers that are wider than the |blockContainingAllText| of their enclosing cluster also become clusters, since they need special treatment due to their width."

Done

>> Source/WebCore/rendering/TextAutosizer.cpp:303
>> +// Use to traverse the tree of descendants, excluding descendants of containers (but returning the containers themselves).
> 
> This comment is already in the .h file?

Done

>> Source/WebCore/rendering/TextAutosizer.cpp:381
>> +bool TextAutosizer::correctFirstTextLeafIfInCluster(TraversalDirection direction, RenderObjects& path, size_t startFrom, const RenderBlock* parentBlockContainingAllText)
> 
> Cool, these are a lot easier to follow now - thanks!

Great!

>> Source/WebCore/rendering/TextAutosizer.cpp:395
>> +    findFirstTextLeafNotInClusterStartingFrom(direction, path, objectNextSibling(direction, cluster));
> 
> What happens if the cluster doesn't have a next sibling? objectNextSibling will return 0, hence findFirstTextLeafNotInClusterStartingFrom will just pop the last entry of path. But startFrom could have other descendants that are text leaves, and that we are ignoring...
> 
> Incidentally, if the tests didn't catch this, might want to add another test ;)

Fixed. I don't think LayoutTests are suited best to test tree traversal. I'd look into unittesting it.

>> Source/WebCore/rendering/TextAutosizer.h:33
>>  #include <wtf/PassRefPtr.h>
> 
> While you're at it, looks like you can remove this include?

Done

>> Source/WebCore/rendering/TextAutosizer.h:57
>> +    typedef Vector<const RenderObject*> RenderObjects;
> 
> It seems you only use "RenderObjects&" in the .h file. Would it be possible to move this typedef to the .cpp and forward declare it in the .h file, such that you can also move the Vector include to the .cpp file? Though if you're going to move all of these to the .cpp file anyway, it might not be worth spending too long on this :)

To "forward declare" a typedef means to forward declare used classes (so namespace WTF { template<typename T,size_t capacity> Vector; }) and then make the same typedef in .cpp file like class TextAutosizer { private: typedef Vector<RenderObject> RenderObjects; };
I don't think it's worth it (unless there's another way I'm not aware of)

>> Source/WebCore/rendering/TextAutosizer.h:66
>> +    void processContainer(float multiplier, RenderBlock* container, const RenderBlock* parentBlockContainingAllText, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&);
> 
> Please sync up your parameter names. In the header file both processCluster and processContainer take a parentBlockContainingAllText, but in the cpp file they both take a blockContainingAllText. I would suggest that instead processCluster takes a parentBlockContainingAllText (in both) and processContainer takes a blockContainingAllText (in both), since conceptually processContainer is passed the blockContainingAllText of the current cluster (rather than of the parent cluster).
> 
> This would also allow you to rename deepestBlockContainingAllText in processCluster to blockContainingAllText, which might be more consistent.

Done.

>> Source/WebCore/rendering/TextAutosizer.h:86
>> +    // returns the lowest common ancestor for these text nodes. Otherwise, returns the cluster itself.
> 
> Tiny nit: s/for/of/

Done

>> Source/WebCore/rendering/TextAutosizer.h:90
>> +    // the given child of the last object in the path. Follows the given tree traversal direction. If such text leaf is not found,
> 
> It's not clear what happens if the search succeeds. How about replacing the last sentence with: "If such a text leaf is found, the path will be replaced with the path to the text leaf; otherwise the last object will be removed from the path." ?

Done

>> Source/WebCore/rendering/TextAutosizer.h:95
>> +    // If cluster is not found, returns false. Otherwise, removes every node in the path including the found cluster and continues
> 
> "removes every node in the path including the found cluster" suggests that it completely clears the path, but it only removes the cluster and entries after (with greater index than) the cluster. How about "If no cluster is found, this returns false. Otherwise, it removes the found cluster and all subsequent nodes from the path, then continues traversing the tree to find the next text leaf in the given direction, and finally returns true."

Done

>> Source/WebCore/rendering/TextAutosizer.h:97
>> +    static bool correctFirstTextLeafIfInCluster(TraversalDirection, RenderObjects& path, size_t startFrom, const RenderBlock* parentBlockContainingAllText);
> 
> Please mention this method in the ChangeLog. Also, "adjust" might be clearer than "correct", as correct has multiple meanings and people might interpret it as asking a question, "is this the correct first text leaf" or something... :)

Done

[John Mellor]

Comment on attachment 178210 [details]
Patch

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

Looking good. I like how you merged findFirst and findNext into nextInPreOrderSkippingClusters, and the parallel to nextInPreOrderSkippingDescendantsOfContainers makes it easy to follow.

> Source/WebCore/ChangeLog:27
> +        (WebCore::TextAutosizer::nextInInOrderSkippingClusters): finds the next renderer that is not an autosizing cluster.

s/nextInInOrderSkippingClusters/nextInPreOrderSkippingClusters/

> Source/WebCore/ChangeLog:29
> +        (WebCore::TextAutosizer::nextTextLeafInInOrderSkippingClusters): finds the next text leaf that doesn't belong to any cluster.

s/nextTextLeafInInOrderSkippingClusters/nextTextLeafInPreOrderSkippingClusters/

> Source/WebCore/rendering/TextAutosizer.cpp:320
> +const RenderObject* TextAutosizer::nextInPreOrderSkippingClusters(TraversalDirection direction, const RenderObject* current, const RenderObject* stayWithin, const RenderBlock* parentBlockContainingAllText)

Nit: I think in this context I'd call it blockContainingAllText not parentBlockContainingAllText, as it is the LCA block of the current cluster, not the parent cluster. Ditto for nextTextLeafInPreOrderSkippingClusters and adjustFirstTextLeafIfInCluster below.

> Source/WebCore/rendering/TextAutosizer.cpp:379
> +void TextAutosizer::nextTextLeafInPreOrderSkippingClusters(TraversalDirection direction, RenderObjects& path, const RenderBlock* parentBlockContainingAllText)

This looks like an instance of 'Precede getters that return values through out arguments with the word "get".'
http://www.webkit.org/coding/coding-style.html#names-out-argument

> Source/WebCore/rendering/TextAutosizer.cpp:385
> +    const RenderObject* next = nextInPreOrderSkippingClusters(direction, current, stayWithin, parentBlockContainingAllText);

I think you could avoid this extra call to nextInPreOrderSkippingClusters if you wrote this as:

    const RenderObject* next = current;
    for (;;) {
        next = nextInPreOrderSkippingClusters(direction, next, ...);
        if (!next) {
            path.clear();
            return;
        }
        if (next->isText()) {
            ...
            return;
        }
    }

But I don't mind if you think the current approach is clearer.

> Source/WebCore/rendering/TextAutosizer.cpp:388
> +            ASSERT(next->isEmpty());

What's this ASSERT for? None of your code seems to depend on RenderTexts having no children.

> Source/WebCore/rendering/TextAutosizer.cpp:409
> +    for (size_t i = startFrom; i < path.size(); ++i)

I think WebKit typically uses braces whenever the body of a loop is multiple lines, even if it happens to be a single statement.

> Source/WebCore/rendering/TextAutosizer.h:84
> +    // Use to traverse the tree of descendants, excluding clusters with their descendants (the current object and |stayWithin|

Nit: s/with/and/

> LayoutTests/ChangeLog:8
> +        isAutosizingCluster() now checks for the width of the container being greater than the width of the lowest

This comment doesn't need to repeat the explanation of what the patch does; instead it should explain how the patch is being tested.

> LayoutTests/fast/text-autosizing/cluster-wide-in-narrow.html:30
> +        This text should be autosized to 30px computed font-size (16 * 600/320), since because of its width:600px it becomes a cluster.<br>

Slightly more explanation would be good, e.g. "since its width:600px makes it wider than the lowest common ancestor of its enclosing cluster, hence it will itself become a cluster.

> LayoutTests/fast/text-autosizing/cluster-wide-in-narrow.html:33
>  

You need to change the explanation for the div below, as it no longer behaves "similarly" :)

> LayoutTests/fast/text-autosizing/wide-child.html:27
> +    	This text should be autosized to 40px computed font size (16 * 800/320) since it's wider than its cluster so it becomes a cluster itself.<br>

Technically it's wider than the LCA of its enclosing cluster, not wider than its enclosing cluster (the RenderView).

> LayoutTests/fast/text-autosizing/wide-in-narrow-overflow-scroll.html:26
> +        This text should be autosized to 40px computed font size (16 * 800/320), since it's wider than the containing cluster and therefore becomes a cluster itself.<br>

Ditto wider than LCA not cluster. This case is also interesting as only 400px of this div will be visible at any one time, even though it is 800px wide. So really we should be using 400px as the width for this div. But that's not the fault of this patch - I filed bug 104897.

[Anton Vayvod]

Created attachment 179291 [details]
Patch

[Anton Vayvod]

Created attachment 179297 [details]
Patch

[John Mellor]

Comment on attachment 179297 [details]
Patch

Looks good to me. Kenneth/Julien, what do you think?

[John Mellor]

(In reply to comment #17)
> Looks good to me. Kenneth/Julien, what do you think?

By the way we tested this on 1000 popular homepages and this improved quite a few of them, without causing any significant regressions.

[Julien Chaffraix]

Comment on attachment 179297 [details]
Patch

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

Overall comment: this patch is 37k long, webkit-patch should have warned you that it's too big for any reviewer to safely review. I barely scratched the surface on this review so if you want your patch to be properly reviewed, split it up. If not, I can guarantee that it will painful for both of us.

> Source/WebCore/ChangeLog:16
> +

What is missing here would be some context: what are you trying to achieve and why?

> Source/WebCore/ChangeLog:20
> +        (WebCore::TextAutosizer::processSubtree): calls processCluster with new number of arguments.

Nit: Sentences starts with a capitalized letter. Also going to the next line give me space to avoid wrapping on the right (the reviewing tool had to wrap some of your massive lines).

> Source/WebCore/ChangeLog:21
> +        (WebCore::TextAutosizer::processCluster): parentBlockContainingAllText parameter (for assertion only), finds new block containing all text nodes and passes it into processContainer.

I don't think it's a good argument to pass in an extra argument just for the sake of ASSERTing.

> Source/WebCore/ChangeLog:34
> +        (TextAutosizer): removed unused declarations, added a typedef for a vector of RenderObjects.

We usually remove the (TextAutosizer): part as it has no value (script snafu).

> Source/WebCore/rendering/TextAutosizer.cpp:340
> +const RenderBlock* TextAutosizer::findDeepestBlockContainingAllText(const RenderBlock* cluster, const RenderBlock* parentBlockContainingAllText)
> +{
> +    ASSERT_UNUSED(parentBlockContainingAllText, isContainerAutosizingCluster(cluster, parentBlockContainingAllText));

There is only one call site for this function and the only reason you pass |parentBlockContainingAllText| is for the ASSERT so the parameter should be removed and if you follow the caller, the ASSERT is already there.

> Source/WebCore/rendering/TextAutosizer.cpp:400
> +            RenderObjects nextPath;
> +            while (next != stayWithin) {
> +                nextPath.append(next);
> +                next = next->parent();
> +            }
> +            nextPath.append(stayWithin);
> +            nextPath.reverse();
>  
> -    return containingBlock;
> +            path.swap(nextPath);
> +            return;

I don't see the need for an extra variable here as you are swapping the Vector anyway.

[Anton Vayvod]

Created attachment 179489 [details]
Patch

[Anton Vayvod]

Comment on attachment 179297 [details]
Patch

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

I've split the patch as you suggested. Hopefully this one is not very big now.

>> Source/WebCore/ChangeLog:16
>> +
> 
> What is missing here would be some context: what are you trying to achieve and why?

Added.

>> Source/WebCore/ChangeLog:20
>> +        (WebCore::TextAutosizer::processSubtree): calls processCluster with new number of arguments.
> 
> Nit: Sentences starts with a capitalized letter. Also going to the next line give me space to avoid wrapping on the right (the reviewing tool had to wrap some of your massive lines).

Done. Sorry about wrapping.

>> Source/WebCore/ChangeLog:21
>> +        (WebCore::TextAutosizer::processCluster): parentBlockContainingAllText parameter (for assertion only), finds new block containing all text nodes and passes it into processContainer.
> 
> I don't think it's a good argument to pass in an extra argument just for the sake of ASSERTing.

Agreed. Removed it.

>> Source/WebCore/ChangeLog:34
>> +        (TextAutosizer): removed unused declarations, added a typedef for a vector of RenderObjects.
> 
> We usually remove the (TextAutosizer): part as it has no value (script snafu).

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:340
>> +    ASSERT_UNUSED(parentBlockContainingAllText, isContainerAutosizingCluster(cluster, parentBlockContainingAllText));
> 
> There is only one call site for this function and the only reason you pass |parentBlockContainingAllText| is for the ASSERT so the parameter should be removed and if you follow the caller, the ASSERT is already there.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:400
>> +            return;
> 
> I don't see the need for an extra variable here as you are swapping the Vector anyway.

Acknowledged. It will be part of the follow-up patch.

[Anton Vayvod]

Created attachment 179497 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179497 [details]
Patch

Attachment 179497 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15317723

[Anton Vayvod]

Created attachment 179499 [details]
Patch

[Anton Vayvod]

Created attachment 179506 [details]
Patch

[Julien Chaffraix]

Comment on attachment 179506 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by Julien Chaffraix.

Mhhh, please be careful with that. It shouldn't be filled as the patch was never r+

> Source/WebCore/rendering/TextAutosizer.cpp:-98
> -    ASSERT(isAutosizingCluster(cluster));
> -

Why do you remove this ASSERT when the callers didn't change?

> Source/WebCore/rendering/TextAutosizer.cpp:237
> +bool TextAutosizer::isAutosizingCluster(const RenderObject* object, const RenderBlock* blockContainingAllText)

This should take a RenderBlock as you only ever call it on RenderBlock's.

> Source/WebCore/rendering/TextAutosizer.cpp:-322
> -    ASSERT(isAutosizingCluster(cluster));
> -

Why do you remove this ASSERT when the callers didn't change?

> Source/WebCore/rendering/TextAutosizer.h:70
> +    // Returns true if the given autosizing container is an autosizing cluster. Checks if the container is wider than
> +    // |parentBlockContainingAllText| if the pointer is not null.

Please remove these comments as they are not really helpful.

> Source/WebCore/rendering/TextAutosizer.h:73
> +    // Returns true if the given renderer is an autosizing cluster. Checks if the renderer is wider than
> +    // |parentBlockContainingAllText| if the pointer is not null.

Ditto.

> Source/WebCore/rendering/TextAutosizer.h:74
> +    static bool isAutosizingCluster(const RenderObject*, const RenderBlock* parentBlockContainingAllText);

I would make the second argument optional to avoid confusing call sites: isAutosizingCluster(renderer, 0)

> Source/WebCore/rendering/TextAutosizer.h:84
> +    // If there are any descendant text nodes of the given cluster that are not descendants of any descendant cluster,
> +    // returns the lowest common ancestor of these text nodes. Otherwise, returns the cluster itself.

That's why we usually don't put comments next to functions as:
* they get stale super easily.
* different people have probably different level of verbosity or different ways of describing the same code.
* the naming should reflect that and now doesn't.

If you think this comment is unclear, remove it and update the function name to be clearer. As is, this change doesn't add much apart from confusing the blaming tool (there is no code change in this function).

[John Mellor]

Comment on attachment 179506 [details]
Patch

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

>> Source/WebCore/rendering/TextAutosizer.cpp:-98
>> -
> 
> Why do you remove this ASSERT when the callers didn't change?

Because isAutosizingCluster(cluster, 0) may return false here even though this is a valid cluster, since we're not passing in the parentBlockContainingAllText argument (which the caller of this function knows, but which this function doesn't have access to since we deemed it wasn't worth passing an extra argument just for this assert). Ditto below.

>> Source/WebCore/rendering/TextAutosizer.h:74
>> +    static bool isAutosizingCluster(const RenderObject*, const RenderBlock* parentBlockContainingAllText);
> 
> I would make the second argument optional to avoid confusing call sites: isAutosizingCluster(renderer, 0)

It's nice to emphasize that the result of this function depends on the blockContainingAllText you pass in (and that if you pass in 0, you'll get an incomplete answer). Though I notice that both of the callers of this function pass in 0. So perhaps we could remove the second argument completely (always pass in 0 to isContainerAutosizingCluster), and rename the function to, e.g. "isAutosizingClusterIgnoringWidth"?

[John Mellor]

Comment on attachment 179506 [details]
Patch

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

> LayoutTests/fast/text-autosizing/cluster-wide-in-narrow-expected.html:26
> +        This text should be autosized to 20px computed font-size (16 * 400 / 320), since it's not an autosizing cluster itself and its parent element is the enclosing autosizing cluster 400px wide.<br>

Nit: s/cluster 400px/cluster which is 400px/

[Anton Vayvod]

Created attachment 179785 [details]
Patch

[Anton Vayvod]

Comment on attachment 179506 [details]
Patch

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

>> Source/WebCore/ChangeLog:6
>> +        Reviewed by Julien Chaffraix.
> 
> Mhhh, please be careful with that. It shouldn't be filled as the patch was never r+

Yes, sorry. It was supposed to go to another patch (submitted last Friday).

>> Source/WebCore/rendering/TextAutosizer.cpp:237
>> +bool TextAutosizer::isAutosizingCluster(const RenderObject* object, const RenderBlock* blockContainingAllText)
> 
> This should take a RenderBlock as you only ever call it on RenderBlock's.

It is called on RenderObject* in findFirstTextLeafNotInCluster().

>> Source/WebCore/rendering/TextAutosizer.h:70
>> +    // |parentBlockContainingAllText| if the pointer is not null.
> 
> Please remove these comments as they are not really helpful.

Done.

>> Source/WebCore/rendering/TextAutosizer.h:73
>> +    // |parentBlockContainingAllText| if the pointer is not null.
> 
> Ditto.

Done.

>>> Source/WebCore/rendering/TextAutosizer.h:74
>>> +    static bool isAutosizingCluster(const RenderObject*, const RenderBlock* parentBlockContainingAllText);
>> 
>> I would make the second argument optional to avoid confusing call sites: isAutosizingCluster(renderer, 0)
> 
> It's nice to emphasize that the result of this function depends on the blockContainingAllText you pass in (and that if you pass in 0, you'll get an incomplete answer). Though I notice that both of the callers of this function pass in 0. So perhaps we could remove the second argument completely (always pass in 0 to isContainerAutosizingCluster), and rename the function to, e.g. "isAutosizingClusterIgnoringWidth"?

Renamed to isAutosizingClusterIgnoringParentCluster (as width seems to be too specific).

>> Source/WebCore/rendering/TextAutosizer.h:84
>> +    // returns the lowest common ancestor of these text nodes. Otherwise, returns the cluster itself.
> 
> That's why we usually don't put comments next to functions as:
> * they get stale super easily.
> * different people have probably different level of verbosity or different ways of describing the same code.
> * the naming should reflect that and now doesn't.
> 
> If you think this comment is unclear, remove it and update the function name to be clearer. As is, this change doesn't add much apart from confusing the blaming tool (there is no code change in this function).

Agree, removed the comment.

>> LayoutTests/fast/text-autosizing/cluster-wide-in-narrow-expected.html:26
>> +        This text should be autosized to 20px computed font-size (16 * 400 / 320), since it's not an autosizing cluster itself and its parent element is the enclosing autosizing cluster 400px wide.<br>
> 
> Nit: s/cluster 400px/cluster which is 400px/

Done.

[Anton Vayvod]

Created attachment 179797 [details]
Patch

[Julien Chaffraix]

Comment on attachment 179797 [details]
Patch

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

r=me but I would like to see the updated patch before it lands.

> Source/WebCore/rendering/TextAutosizer.cpp:-97
> -    ASSERT(isAutosizingCluster(cluster));

Please add a comment in the ChangeLog about *why* we are removing these ASSERTs.

> Source/WebCore/rendering/TextAutosizer.cpp:240
> +bool TextAutosizer::isAutosizingClusterIgnoringParentCluster(const RenderObject* object)

This new naming is pretty confusing I have to say as you don't really check your parent anywhere (it's implied as you define any auto-sizing cluster as independent from its parent cluster). Here is some alternative names that convey the idea better:
* isIndependentAutosizingCluster
* rendererIsAutosizingCluster

> Source/WebCore/rendering/TextAutosizer.cpp:265
> +

Extra line.

> Source/WebCore/rendering/TextAutosizer.h:81
> -    // Depending on the traversal direction specified, finds the first or the last leaf text node child that doesn't
> -    // belong to any cluster.
> -    static const RenderObject* findFirstTextLeafNotInCluster(const RenderObject*, size_t& depth, TraversalDirection);
> +    // Returns the first text leaf in the given direction and its depth starting from the given parent.
> +    static const RenderObject* findFirstTextLeafNotInCluster(const RenderObject* parent, size_t& depth, TraversalDirection);

Same comment as previously, this change doesn't add much as you don't touch this function. The extra parameter name is not super helpful either so I would leave this code alone.

[Anton Vayvod]

Created attachment 180020 [details]
Patch

[Julien Chaffraix]

Comment on attachment 180020 [details]
Patch

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

> Source/WebCore/rendering/TextAutosizer.h:69
> -    static bool isAutosizingCluster(const RenderBlock*);
> +    static bool isContainerAutosizingCluster(const RenderBlock*, const RenderBlock* parentBlockContainingAllText);
> +    static bool isIndependentAutosizingCluster(const RenderObject*);

Looking again at the change, it would be a lot more readable to just have 2 overloaded functions:

static bool isAutosizingCluster(const RenderBlock*, const RenderBlock* parentBlockContainingAllText);
static bool isAutosizingCluster(const RenderObject*);

Those 2 functions really returns the same information and having the same name will convey this information a lot better. The original renaming doesn't add much IMHO but I could be convinced otherwise.

[Anton Vayvod]

Created attachment 180042 [details]
Patch

[John Mellor]

(In reply to comment #34)
> Looking again at the change, it would be a lot more readable to just have 2 overloaded functions:
> 
> static bool isAutosizingCluster(const RenderBlock*, const RenderBlock* parentBlockContainingAllText);
> static bool isAutosizingCluster(const RenderObject*);
> 
> Those 2 functions really returns the same information and having the same name will convey this information a lot better. The original renaming doesn't add much IMHO but I could be convinced otherwise.

I'm not a great fan of giving them the same name. The 2nd function only covers part of the reasons that something can be a cluster, and will return false for things that are actually clusters. It seems misleading to give it the same name without some kind of disclaimer. For example it wasn't obvious that ASSERT(isAutosizingCluster(renderer)) at the start of processCluster could fail, whereas it would be clearer that a function named isAutosizingClusterIgnoringParentClusterTextWidth (or something like that) might fail.

[WebKit Review Bot]

Comment on attachment 180042 [details]
Patch

Clearing flags on attachment: 180042

Committed r138111: <http://trac.webkit.org/changeset/138111>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

(In reply to comment #36)
> (In reply to comment #34)
> > Looking again at the change, it would be a lot more readable to just have 2 overloaded functions:
> > 
> > static bool isAutosizingCluster(const RenderBlock*, const RenderBlock* parentBlockContainingAllText);
> > static bool isAutosizingCluster(const RenderObject*);
> > 
> > Those 2 functions really returns the same information and having the same name will convey this information a lot better. The original renaming doesn't add much IMHO but I could be convinced otherwise.
> 
> I'm not a great fan of giving them the same name. The 2nd function only covers part of the reasons that something can be a cluster, and will return false for things that are actually clusters. It seems misleading to give it the same name without some kind of disclaimer. For example it wasn't obvious that ASSERT(isAutosizingCluster(renderer)) at the start of processCluster could fail, whereas it would be clearer that a function named isAutosizingClusterIgnoringParentClusterTextWidth (or something like that) might fail.

If you passed in the text block container, they would be strictly equivalent but we don't. Feel free to post a patch about that, I don't feel strongly about either way (just that the direction we were heading to with a completely unrelated name was bad).