[webkit-reviews] review canceled: [Bug 97025] Text Autosizing: Cluster text at flow roots, for consistency and to avoid autosizing headers/footers. : [Attachment 164951] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled John Mellor
<johnme at chromium.org>'s request for review:
Bug 97025: Text Autosizing: Cluster text at flow roots, for consistency and to
avoid autosizing headers/footers.
https://bugs.webkit.org/show_bug.cgi?id=97025

Attachment 164951: Patch
https://bugs.webkit.org/attachment.cgi?id=164951&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=164951&action=review


The new patch is starting to look good. More comments.

> Source/WebCore/rendering/TextAutosizer.cpp:85
> +{

ASSERT(isAutosizingCluster(cluster));

> Source/WebCore/rendering/TextAutosizer.cpp:93
> +    // FIXME: Make commonAncestorWidth for the RenderView independent of
whether the window has a vertical
> +    // scrollbar, to avoid having to change the multiplier if the document
height changes.
> +    float commonAncestorWidth = lowestCommonAncestor->contentLogicalWidth();


That should be easy to do by adding back the right scrollbar width. You can add
an helper on RenderBox to do that.

> Source/WebCore/rendering/TextAutosizer.cpp:98
> +	   int logicalWindowWidth = cluster->isHorizontalWritingMode() ?
m_windowSize.width() : m_windowSize.height();
> +	   int logicalLayoutWidth = cluster->isHorizontalWritingMode() ?
m_minLayoutSize.width() : m_minLayoutSize.height();

Nit: This could be an helper function on IntSize akin to
FractionalLayoutBoxExtent

> Source/WebCore/rendering/TextAutosizer.cpp:126
>  {

ASSERT(isAutosizingContainer(container));

> Source/WebCore/rendering/TextAutosizer.cpp:136
>	       // FIXME: Increase list marker size proportionately.

Does this comment still applies (cf comment in isAutosizingContainer)?

> Source/WebCore/rendering/TextAutosizer.cpp:142
> +		   processCluster(descendantBlock, descendantBlock,
descendant);
> +	       else
> +		   processContainer(multiplier, descendantBlock, descendant);

Let's use descendantBlock everywhere here instead of mixing descendant and
descendantBlock which makes it look like we are passing different pointers.

> Source/WebCore/rendering/TextAutosizer.cpp:230
> +{

ASSERT(isAutosizingCluster(lowestCommonAncestor);

> Source/WebCore/rendering/TextAutosizer.cpp:234
> +    // than that, so we're overestimating their contribution to the
linecount).

Would be good to explain 'why' you are not auto-sizing anything less than 4
lines here instead of in the ChangeLog.

> Source/WebCore/rendering/TextAutosizer.h:77
> +
> +    // These two sizes are calculated by processSubtree and cached here.
> +    IntSize m_windowSize;
> +    IntSize m_minLayoutSize;

Those 2 would be better in a structure that we pass around. As you don't need
them after a |processSubtree| phase, it would ensure that no one tries to query
them outside it. I wonder if we could throw in the other arguments too (like
the |container| and the |multiplier|), feel free to push back on this idea.

> LayoutTests/fast/text-autosizing/clusters-sufficient-width-expected.html:32
> +<div style="-webkit-writing-mode: vertical-rl; height: 440px">
> +    This text should be autosized to 22px computed font size (16 * 440/320),
since the perpendicular writing-mode compared to its containing block causes
this to be a new cluster. Unfortunately due to <a
href="https://bugs.webkit.org/show_bug.cgi?id=96557">http://webkit.org/b/96557<
/a> the height:440px is incorrectly interpreted as constraining the
logicalHeight, so it doesn't get autosized.
> +</div>

You forgot to set the font-size here.