Bug 121111

Created attachment 211237 [details] Patch

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

Created attachment 211239 [details] Patch

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.

Comment on attachment 211239 [details] Patch r=me with previous comments addressed.

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.

Comment on attachment 211239 [details] Patch Didn't mean to clear the r+.

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

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

(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.

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

(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.

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

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.

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