[webkit-reviews] review granted: [Bug 130226] [Mac] Avoid creating DOMCSSStyleDeclaration in WebHTMLConverter : [Attachment 226655] Extracted HTMLConverterCaches

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 130226: [Mac] Avoid creating DOMCSSStyleDeclaration in WebHTMLConverter
https://bugs.webkit.org/show_bug.cgi?id=130226

Attachment 226655: Extracted HTMLConverterCaches
https://bugs.webkit.org/attachment.cgi?id=226655&action=review

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226655&action=review


r=me

> Source/WebCore/platform/mac/HTMLConverter.mm:568
> +    auto result = m_computedStyles.add(element, nullptr);
> +    if (result.isNewEntry)
> +	   result.iterator->value =
CSSComputedStyleDeclaration::create(element, true);
> +    CSSStyleDeclaration* declaration = result.iterator->value.get();

This would be more efficient using ComputedStyleExtractor directly. Then we
wouldn't have to build a CSSComputedStyleDeclaration object at all.

> Source/WebCore/platform/mac/HTMLConverter.mm:569
> +    return declaration ? declaration->getPropertyCSSValue(propertyName) : 0;


0 -> nullptr

> Source/WebCore/platform/mac/HTMLConverter.mm:577
> +    auto result = m_inlineStyles.add(element, nullptr);
> +    if (result.isNewEntry)
> +	   result.iterator->value = element->style();
> +    CSSStyleDeclaration* declaration = result.iterator->value.get();

This code would be more efficient if it accessed the StyleProperties directly
via Element::inlineStyle() instead of instantiating a CSSOM wrapper via
style().

> Source/WebCore/platform/mac/HTMLConverter.mm:578
> +    return declaration ? declaration->getPropertyCSSValue(propertyName) : 0;


0 -> nullptr

> Source/WebCore/platform/mac/HTMLConverter.mm:785
> +	   Element* element = toElement(coreNode);

I'd make this Element&.

> Source/WebCore/platform/mac/HTMLConverter.mm:1005
> +	   Element* element = toElement(coreNode);

Same here.

> Source/WebCore/platform/mac/HTMLConverter.mm:2424
> +    _caches = nullptr;

Is this actually necessary?