Bug 130274 – [Mac] Rewrite WebHTMLConverter::_computedStringForNode in C++

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

_computedStringForNode should be rewritten in C++ for performance.

[Ryosuke Niwa]

Created attachment 226781 [details]
Cleanup

[Ryosuke Niwa]

This reduces the runtime from 25-26s to 21-22s (another 20% improvement).

[Andreas Kling]

Comment on attachment 226781 [details]
Cleanup

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

> Source/WebCore/platform/mac/HTMLConverter.mm:631
> +    if (RefPtr<CSSValue> value = computedStylePropertyForElement(element, propertyName)) {
> +        String result;
> +        if (stringFromCSSValue(*value, result))
> +            return result;
> +    }
> +
> +    if (RefPtr<CSSValue> value = computedStylePropertyForElement(element, propertyName)) {
> +        String result;
> +        if (value->isInheritedValue())
> +            inherit = true;
> +        else if (stringFromCSSValue(*value, result))
> +            return result;
> +    }

Why are we calling computedStylePropertyForElement(element, propertyName) twice here?

> Source/WebCore/platform/mac/HTMLConverter.mm:720
> +        if (Node* parent = node.parentNode())

This is really a ContainerNode*.

> Source/WebCore/platform/mac/HTMLConverter.mm:733
> +    if (!result.length())

I'd use isEmpty() here.

[Andreas Kling]

Comment on attachment 226781 [details]
Cleanup

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

> Source/WebCore/platform/mac/HTMLConverter.mm:631
> +    if (RefPtr<CSSValue> value = computedStylePropertyForElement(element, propertyName)) {
> +        String result;
> +        if (stringFromCSSValue(*value, result))
> +            return result;
> +    }
> +
> +    if (RefPtr<CSSValue> value = computedStylePropertyForElement(element, propertyName)) {
> +        String result;
> +        if (value->isInheritedValue())
> +            inherit = true;
> +        else if (stringFromCSSValue(*value, result))
> +            return result;
> +    }

Why are we calling computedStylePropertyForElement(element, propertyName) twice here?

> Source/WebCore/platform/mac/HTMLConverter.mm:720
> +        if (Node* parent = node.parentNode())

This is really a ContainerNode*.

> Source/WebCore/platform/mac/HTMLConverter.mm:733
> +    if (!result.length())

I'd use isEmpty() here.

[Ryosuke Niwa]

Comment on attachment 226781 [details]
Cleanup

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

>>> Source/WebCore/platform/mac/HTMLConverter.mm:631
>>> +    }
>> 
>> Why are we calling computedStylePropertyForElement(element, propertyName) twice here?
> 
> Why are we calling computedStylePropertyForElement(element, propertyName) twice here?

Oops, the second is supposed to be calling computedStylePropertyForElement.

[Ryosuke Niwa]

Created attachment 226959 [details]
Fixed the bug

[Ryosuke Niwa]

Committed r165761: <http://trac.webkit.org/changeset/165761>