Bug 121111 – [iOS] Upstream text autosizing

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Upstream the iOS-specific implementation of text autosizing.

[Daniel Bates]

Created attachment 211237 [details]
Patch

[Daniel Bates]

We should look to further clean up this code as well as look to merge this code with the existing ENABLE(TEXT_AUTOSIZING)-guarded code

[Daniel Bates]

Created attachment 211239 [details]
Patch

[Andy Estes]

Comment on attachment 211237 [details]
Patch

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

> Source/WebCore/ChangeLog:38
> +        resolved before properties that depend on them; make  See <rdar://problem/13522835>.

"make  See ..."

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1454
> +    static void applyValue(CSSPropertyID, StyleResolver* styleResolver, CSSValue* value)
> +    {
> +        if (!value->isPrimitiveValue())
> +            return;
> +
> +        CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +        Length lineHeight;
> +
> +        if (primitiveValue->getIdent() == CSSValueNormal)
> +            lineHeight = RenderStyle::initialLineHeight();
> +        else if (primitiveValue->isLength()) {
> +            double multiplier = styleResolver->style()->effectiveZoom();
> +            if (styleResolver->style()->textSizeAdjust().isNone()) {
> +                if (Frame* frame = styleResolver->document().frame())
> +                    multiplier *= frame->textZoomFactor();
> +            }
> +            lineHeight = primitiveValue->computeLength<Length>(styleResolver->style(), styleResolver->rootElementStyle(), multiplier);
> +            if (styleResolver->style()->textSizeAdjust().isPercentage())
> +                lineHeight = Length(lineHeight.value() * styleResolver->style()->textSizeAdjust().multiplier(), Fixed);
> +        } else if (primitiveValue->isPercentage()) {
> +            // FIXME: percentage should not be restricted to an integer here.
> +            lineHeight = Length((styleResolver->style()->fontSize() * primitiveValue->getIntValue()) / 100, Fixed);
> +        } else if (primitiveValue->isNumber()) {
> +            // FIXME: number and percentage values should produce the same type of Length (ie. Fixed or Percent).
> +            if (styleResolver->style()->textSizeAdjust().isPercentage())
> +                lineHeight = Length(primitiveValue->getDoubleValue() * styleResolver->style()->textSizeAdjust().multiplier() * 100.0, Percent);
> +            else
> +                lineHeight = Length(primitiveValue->getDoubleValue() * 100.0, Percent);
> +        } else if (primitiveValue->isViewportPercentageLength())
> +            lineHeight = primitiveValue->viewportPercentageLength();
> +        else
> +            return;
> +        styleResolver->style()->setLineHeight(lineHeight);
> +        styleResolver->style()->setSpecifiedLineHeight(lineHeight);
> +    }

This is a pretty long function. You could define it outside the class to reduce indentation.

> Source/WebCore/rendering/RenderBlock.cpp:8159
> +#define ParagraphMinWordCount 6
> +#define ParagraphMinLineCount 2
> +#define ParagraphMaxWidth     0.9

These don't seem to be used.

> Source/WebCore/rendering/style/TextSizeAdjustment.h:34
> +    float percentage() { return m_value; }
> +    float multiplier() { return m_value / 100; }

These functions could be const qualified.

> Source/WebKit/mac/WebView/WebFramePrivate.h:106
> +#if ENABLE(IOS_TEXT_AUTOSIZING)
> +- (void)resetTextAutosizingBeforeLayout;
> +- (void)_setVisibleSize:(CGSize)size;
> +- (void)_setTextAutosizingWidth:(CGFloat)width;
> +#endif

Since this is an SPI header I don't think it's safe to use WTF macros here. You can't rely on Platform.h being included in the translation unit.

> Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:164
> +#if ENABLE(IOS_TEXT_AUTOSIZING)
> +#define WebKitMinimumZoomFontSizePreferenceKey @"WebKitMinimumZoomFontSizePreferenceKey"
> +#endif

Ditto.

[Andy Estes]

Comment on attachment 211239 [details]
Patch

r=me with previous comments addressed.

[Sam Weinig]

Comment on attachment 211239 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:2782
> +        // We actually want to merge the following into TOT

This comment doesn't make sense anymore.

> Source/WebCore/css/StyleResolver.cpp:1455
> +    checkForTextSizeAdjust();

I think this might be a bit cleaner if it was passed the style like the functions below.

> Source/WebCore/dom/Document.cpp:4753
> +    if (m_textAutoSizedNodes.contains(key))
> +        value = m_textAutoSizedNodes.get(key);
> +    else {
> +        value = TextAutoSizingValue::create();
> +        m_textAutoSizedNodes.set(key, value);

This should probably do the add() trick to avoid the double hash lookup.

> Source/WebCore/dom/Document.cpp:4766
> +    TextAutoSizingMap::iterator end = m_textAutoSizedNodes.end();
> +    for (TextAutoSizingMap::iterator i = m_textAutoSizedNodes.begin(); i != end; ++i) {
> +        TextAutoSizingKey key = i->key;
> +        RefPtr<TextAutoSizingValue> value = i->value;

This should be using thew new auto hotness.

> Source/WebCore/dom/Document.cpp:4784
> +    TextAutoSizingMap::iterator end = m_textAutoSizedNodes.end();
> +    for (TextAutoSizingMap::iterator i = m_textAutoSizedNodes.begin(); i != end; ++i) {

This should be using thew new auto hotness.

> Source/WebCore/rendering/style/RenderStyle.cpp:369
> +    // FIXME: Not a very smart hash. Could be improved upon.
> +    uint32_t hash = 0;
> +    
> +    hash ^= rareNonInheritedData->m_appearance;
> +    hash ^= rareNonInheritedData->marginBeforeCollapse;
> +    hash ^= rareNonInheritedData->marginAfterCollapse;
> +    hash ^= rareNonInheritedData->lineClamp.value();
> +    hash ^= rareInheritedData->overflowWrap;
> +    hash ^= rareInheritedData->nbspMode;
> +    hash ^= rareInheritedData->lineBreak;
> +    hash ^= WTF::FloatHash<float>::hash(inherited->specifiedLineHeight.value());
> +    hash ^= computeFontHash(inherited->font);
> +    hash ^= inherited->horizontal_border_spacing;
> +    hash ^= inherited->vertical_border_spacing;
> +    hash ^= inherited_flags._box_direction;
> +    hash ^= inherited_flags.m_rtlOrdering;
> +    hash ^= noninherited_flags._position;
> +    hash ^= noninherited_flags._floating;
> +    hash ^= rareNonInheritedData->textOverflow;
> +    hash ^= rareInheritedData->textSecurity;
> +    return hash;

This is a terrible hash function.

[Sam Weinig]

Comment on attachment 211239 [details]
Patch

Didn't mean to clear the r+.

[Build Bot]

Comment on attachment 211239 [details]
Patch

Attachment 211239 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1738520

[Build Bot]

Comment on attachment 211239 [details]
Patch

Attachment 211239 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1734732

[Daniel Bates]

(In reply to comment #4)
> (From update of attachment 211237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211237&action=review
> 
> > Source/WebCore/ChangeLog:38
> > +        resolved before properties that depend on them; make  See <rdar://problem/13522835>.
> 
> "make  See ..."

Will fix.

> 
> > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1454
> > +    static void applyValue(CSSPropertyID, StyleResolver* styleResolver, CSSValue* value)
> > +    {
> > +        if (!value->isPrimitiveValue())
> > +            return;
> > +
> > +        CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> > +        Length lineHeight;
> > +
>> [...]
> > +        styleResolver->style()->setLineHeight(lineHeight);
> > +        styleResolver->style()->setSpecifiedLineHeight(lineHeight);
> > +    }
> 
> This is a pretty long function. You could define it outside the class to reduce indentation.
> 

With regards to the length of the function, we should look to extract the common code between this function and ApplyPropertyLineHeight::applyValue(). As you mentioned, we can define this function outside of the class to also reduce its level of indentation. I suggest we look into these cleanups in a separate patch.

> > Source/WebCore/rendering/RenderBlock.cpp:8159
> > +#define ParagraphMinWordCount 6
> > +#define ParagraphMinLineCount 2
> > +#define ParagraphMaxWidth     0.9
> 
> These don't seem to be used.
> 

Will remove.

> > Source/WebCore/rendering/style/TextSizeAdjustment.h:34
> > +    float percentage() { return m_value; }
> > +    float multiplier() { return m_value / 100; }
> 
> These functions could be const qualified.
> 

Will add qualifier const.

> > Source/WebKit/mac/WebView/WebFramePrivate.h:106
> > +#if ENABLE(IOS_TEXT_AUTOSIZING)
> > +- (void)resetTextAutosizingBeforeLayout;
> > +- (void)_setVisibleSize:(CGSize)size;
> > +- (void)_setTextAutosizingWidth:(CGFloat)width;
> > +#endif
> 
> Since this is an SPI header I don't think it's safe to use WTF macros here. You can't rely on Platform.h being included in the translation unit.
> 

Will remove ENABLE(IOS_TEXT_AUTOSIZING)-guard and define empty implementations of these functions when iOS test autosizing is disabled (say, on the Mac port). 

> > Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:164
> > +#if ENABLE(IOS_TEXT_AUTOSIZING)
> > +#define WebKitMinimumZoomFontSizePreferenceKey @"WebKitMinimumZoomFontSizePreferenceKey"
> > +#endif
> 
> Ditto.

Will remove ENABLE(IOS_TEXT_AUTOSIZING)-guard.

[Build Bot]

Comment on attachment 211239 [details]
Patch

Attachment 211239 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1738546

[Daniel Bates]

(In reply to comment #6)
> (From update of attachment 211239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211239&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:2782
> > +        // We actually want to merge the following into TOT
> 
> This comment doesn't make sense anymore.
> 

Will remove comment.

> > Source/WebCore/css/StyleResolver.cpp:1455
> > +    checkForTextSizeAdjust();
> 
> I think this might be a bit cleaner if it was passed the style like the functions below.

Will modify function to take argument of type RenderStyle*.

> 
> > Source/WebCore/dom/Document.cpp:4753
> > +    if (m_textAutoSizedNodes.contains(key))
> > +        value = m_textAutoSizedNodes.get(key);
> > +    else {
> > +        value = TextAutoSizingValue::create();
> > +        m_textAutoSizedNodes.set(key, value);
> 
> This should probably do the add() trick to avoid the double hash lookup.
> 

Will write in terms of HashMap::add().

> > Source/WebCore/dom/Document.cpp:4766
> > +    TextAutoSizingMap::iterator end = m_textAutoSizedNodes.end();
> > +    for (TextAutoSizingMap::iterator i = m_textAutoSizedNodes.begin(); i != end; ++i) {
> > +        TextAutoSizingKey key = i->key;
> > +        RefPtr<TextAutoSizingValue> value = i->value;
> 
> This should be using thew new auto hotness.
> 

Will write in terms of keyword auto.

> > Source/WebCore/dom/Document.cpp:4784
> > +    TextAutoSizingMap::iterator end = m_textAutoSizedNodes.end();
> > +    for (TextAutoSizingMap::iterator i = m_textAutoSizedNodes.begin(); i != end; ++i) {
> 
> This should be using thew new auto hotness.
> 

Ditto.

> > Source/WebCore/rendering/style/RenderStyle.cpp:369
> > +    // FIXME: Not a very smart hash. Could be improved upon.
> > +    uint32_t hash = 0;
> > +    
> > +    hash ^= rareNonInheritedData->m_appearance;
> > +    hash ^= rareNonInheritedData->marginBeforeCollapse;
> > +    hash ^= rareNonInheritedData->marginAfterCollapse;
> > +    hash ^= rareNonInheritedData->lineClamp.value();
> > +    hash ^= rareInheritedData->overflowWrap;
> > +    hash ^= rareInheritedData->nbspMode;
> > +    hash ^= rareInheritedData->lineBreak;
> > +    hash ^= WTF::FloatHash<float>::hash(inherited->specifiedLineHeight.value());
> > +    hash ^= computeFontHash(inherited->font);
> > +    hash ^= inherited->horizontal_border_spacing;
> > +    hash ^= inherited->vertical_border_spacing;
> > +    hash ^= inherited_flags._box_direction;
> > +    hash ^= inherited_flags.m_rtlOrdering;
> > +    hash ^= noninherited_flags._position;
> > +    hash ^= noninherited_flags._floating;
> > +    hash ^= rareNonInheritedData->textOverflow;
> > +    hash ^= rareInheritedData->textSecurity;
> > +    return hash;
> 
> This is a terrible hash function.

Filed bug #121131 to improve the hash function.

[Daniel Bates]

Committed r155496: <http://trac.webkit.org/changeset/155496>

[Darin Adler]

Some comments after the fact:

- I prefer conditional code in a separate paragraph rather than sorted alphabetically in with everything else. That applies in a few places in the patch.
- seems that StyleResolver::checkForTextSizeAdjust could be optimized so it doesn’t call setFontDescription when the font description doesn’t change; I wonder if that’s a real or common case
- StyleResolver::checkForTextSizeAdjust should take a reference rather than a pointer
- TextAutoSizingKey::deletedKeyDoc is not needed, because TextAutoSizingKey::deletedKeyStyle is enough on its own, and there is no need to even look at the document, which can just be nullptr
- addAutoSizingNode should probably take an element rather than a node, and maybe even a more specific type, and a reference rather than a pointer
- code iterating m_textAutoSizedNodes should not churn reference counts by putting the value into a RefPtr
- TextAutoSizingTraits could derive from just HashTraits instead of from WTF::GenericHashTraits
- textAutosizingWidth and setTextAutosizingWidth should not be functions on Frame; we are working so hard to remove functions from Frame and it's a serious bummer to add more instead of finding one of the Frame sub objects to put it on
- the code in FrameView uses the unpleasant variable name "visWidth" (is that visibleWidth, or is there a better name) and has a pointless local variable for a boolean
- resizeTextPermitted is not a good function name, because it sounds like a verb phrase, a function that will do a resize
- some of the new code uses the variable name "obj"
- the comment before oneLineTextMultiplier states something that is demonstrably untrue if you read the code below it
- the LineCount enum values are ALL_CAPS for no good reason
- RenderObject::traverseNext is a copy of an already-existing function, RenderObject::nextInPreOrder
- the fancy versions of RenderObject::traverseNext should just call the non-fancy version in a loop instead of sprinkling logic inside the iteration
- the depth stack inside RenderObject::adjustComputedFontSizesOnBlocks should have some inline capacity to avoid doing memory allocation for common cases
- it's easy to make a good hash instead of a horrible one in RenderStyle::hashForTextAutosizing, and we should
- the TextSizeAdjustmentType enum is not really an enum, just some magic values for m_value in TextSizeAdjustment, and the class could encapsulate those magic values much better than that with isXXX functions and the like
- [WebFrame resetTextAutosizingBeforeLayout] has an extra local variable it doesn't need, as does _setTextAutosizingWidth:

I’d like to fix some of those things.

[Daniel Bates]

*** Bug 73546 has been marked as a duplicate of this bug. ***