[FastTextAutosizer] Refactor for understandability

[FastTextAutosizer] Refactor for understandability

This patch is a followup to [1] to cleanup the dependencies on
the old text autosizer. The largest change in this patch is to
remove the "container" concept and rely only on clusters and
blocks in the FastTextAutosizer.

The big changes in this patch include:
* formInputTags vector -> isFormInput(const Element*)
    Passing around a static vector was unnecessarily confusing. This
    patch refactors the formInput statics into a single function:
    isFormInput.
* isAutosizingContainer -> !isExemptFromAutosizer
    The logic of isAutosizingContainer has been switched in
    isExemptFromAutosizer and I think the new name better reflects
    what this function does.
* containerIsRowOfLinks -> blockContainsRowOfLinks
* contentHeightIsConstrained -> blockHeightConstrained
* containerContainsOneOfTags -> blockContainsFormInput
* containerShouldBeAutosized -> !blockPreventedFromAutosizing
    The logic of containerShouldBeAutosized has been switched in
    blockPreventedFromAutosizing. I think this better reflects what
    this function does.
* blockMightBecomeAutosizingCluster has been added.
* isFingerprintingCandidate has been removed in favor of
    blockMightBecomeAutosizingCluster

[1] https://codereview.chromium.org/200603002

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

[pdr., 2014-03-16 00:40:12]

Ready for an initial review. It may be best to split this up.

[skobes, 2014-03-17 19:20:44]

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

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:76: static bool
isExemptFromAutosizer(const RenderObject* renderer)
If this returns false, it doesn't mean the block isn't autosized, just that it
cannot be a cluster root.  So I'd call this isPotentialClusterRoot or similar. 
(But not "isClusterRoot", since if it returns true, it still might not be one.)

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:144: static bool
blockContainsRowOfLinks(const RenderBlock* block)
blockIsRowOfLinks (since if it contains links and other things as well, we
return false).

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:218: static bool
blockPreventedFromAutosizing(const RenderBlock* block)
blockSuppressesAutosizing (since autosizing is disabled not just on the block
itself, but also on descendant blocks in the cluster).

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:421: RenderObject* descendant =
block->nextInPreOrder();
I'm not sure what the changes in inflate() are doing.  Is it just refactoring
the existing logic for clarity?

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:514: // FIXME: These checks do not
seem right but our tests are particularly
Clarify what specifically seems wrong?

[pdr., 2014-03-18 21:55:00]

Thanks for the review. PTAL

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

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:76: static bool
isExemptFromAutosizer(const RenderObject* renderer)
On 2014/03/17 19:20:44, skobes wrote:
> If this returns false, it doesn't mean the block isn't autosized, just that it
> cannot be a cluster root.  So I'd call this isPotentialClusterRoot or similar.

> (But not "isClusterRoot", since if it returns true, it still might not be
one.)

This took me a while to grok but you're definitely right. This also makes the
code much cleaner, I think.

I left the code broken into separate if statements. I think this is easier to
understand, but I don't feel strongly about it.

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:144: static bool
blockContainsRowOfLinks(const RenderBlock* block)
On 2014/03/17 19:20:44, skobes wrote:
> blockIsRowOfLinks (since if it contains links and other things as well, we
> return false).

Done.

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:218: static bool
blockPreventedFromAutosizing(const RenderBlock* block)
On 2014/03/17 19:20:44, skobes wrote:
> blockSuppressesAutosizing (since autosizing is disabled not just on the block
> itself, but also on descendant blocks in the cluster).

Done.

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:421: RenderObject* descendant =
block->nextInPreOrder();
On 2014/03/17 19:20:44, skobes wrote:
> I'm not sure what the changes in inflate() are doing.  Is it just refactoring
> the existing logic for clarity?

This is mostly removing "nextChildSkippingChildrenOfBlocks" because this is the
only caller. I think the comment here ("Skip block descendants because they will
be inflate()'d on their own.") makes it easier to understand what's going on. I
don't feel too strongly about this though.

https://codereview.chromium.org/197883013/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:514: // FIXME: These checks do not
seem right but our tests are particularly
On 2014/03/17 19:20:44, skobes wrote:
> Clarify what specifically seems wrong?

I updated this comment. In general I think this really wants to be
blockMightBecomeAutosizingCluster which checks:
isIndependentDescendant(block) || mightBeWiderOrNarrowerDescendant(block) ||
block->isTable()

Instead, it's a hodgepodge of checks :(

[skobes, 2014-03-19 00:04:33]

https://codereview.chromium.org/197883013/diff/20001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/197883013/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:259: if (!block->isRenderView() &&
!blockMightBecomeAutosizingCluster(block))
It shouldn't be necessary to check isRenderView here, since
isIndependentDescendant returns true for the RenderView.

https://codereview.chromium.org/197883013/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:494: // FIXME: This check wants to
be blockMightBecomeAutosizingCluster but our tests
I think the code is correct as written, because
blockMightBecomeAutosizingCluster is really testing for a potential
*independent* cluster, but this check is trying to account for both independent
and multiplier-inheriting clusters.

[skobes, 2014-03-19 00:09:58]

https://codereview.chromium.org/197883013/diff/20001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/197883013/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:494: // FIXME: This check wants to
be blockMightBecomeAutosizingCluster but our tests
On 2014/03/19 00:04:33, skobes wrote:
> I think the code is correct as written, because
> blockMightBecomeAutosizingCluster is really testing for a potential
> *independent* cluster, but this check is trying to account for both
independent
> and multiplier-inheriting clusters.

(it will be clearer once we address http://crbug.com/339213)

[pdr., 2014-03-19 00:14:00]

PTAL

https://codereview.chromium.org/197883013/diff/20001/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/197883013/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:259: if (!block->isRenderView() &&
!blockMightBecomeAutosizingCluster(block))
On 2014/03/19 00:04:33, skobes wrote:
> It shouldn't be necessary to check isRenderView here, since
> isIndependentDescendant returns true for the RenderView.

Good catch. I also removed a similar call in maybeCreateCluster

https://codereview.chromium.org/197883013/diff/20001/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:494: // FIXME: This check wants to
be blockMightBecomeAutosizingCluster but our tests
On 2014/03/19 00:04:33, skobes wrote:
> I think the code is correct as written, because
> blockMightBecomeAutosizingCluster is really testing for a potential
> *independent* cluster, but this check is trying to account for both
independent
> and multiplier-inheriting clusters.

That is what it's doing, though it's not clear to me why the width check doesn't
matter. You're right that this isn't a fixme though, so I've returned this
comment to its original.

[skobes, 2014-03-19 00:23:32]

lgtm

The extra isRenderView() is probably left over from when we allowed the subtree
layout root to be at the top of the cluster stack.

[pdr., 2014-03-19 00:25:56]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-19 00:26:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/197883013/30001

[commit-bot: I haz the power, 2014-03-19 02:54:06]

Change committed as 169511