[webkit-reviews] review requested: [Bug 121111] [iOS] Upstream text autosizing : [Attachment 211239] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has asked  for review:
Bug 121111: [iOS] Upstream text autosizing
https://bugs.webkit.org/show_bug.cgi?id=121111

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
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.