Bug 130274

Summary:

[Mac] Rewrite WebHTMLConverter::_computedStringForNode in C++

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, enrica, kling, koivisto, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

Unspecified

Bug Depends on:

Bug Blocks:

130227, 130284

Attachments:

Description	Flags
Cleanup	none
Fixed the bug	kling: review+

Description Ryosuke Niwa 2014-03-14 16:54:02 PDT

_computedStringForNode should be rewritten in C++ for performance.

Comment 1 Ryosuke Niwa 2014-03-14 17:05:54 PDT

Created attachment 226781 [details]
Cleanup

Comment 2 Ryosuke Niwa 2014-03-14 17:06:31 PDT

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

Comment 3 Andreas Kling 2014-03-15 14:46:04 PDT

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.

Comment 4 Andreas Kling 2014-03-15 14:46:05 PDT

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.

Comment 5 Ryosuke Niwa 2014-03-16 19:17:15 PDT

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.

Comment 6 Ryosuke Niwa 2014-03-17 14:00:12 PDT

Created attachment 226959 [details]
Fixed the bug

Comment 7 Ryosuke Niwa 2014-03-17 15:05:05 PDT

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