Bug 130274 - [Mac] Rewrite WebHTMLConverter::_computedStringForNode in C++
Summary: [Mac] Rewrite WebHTMLConverter::_computedStringForNode in C++
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 130227 130284
  Show dependency treegraph
 
Reported: 2014-03-14 16:54 PDT by Ryosuke Niwa
Modified: 2014-03-17 15:05 PDT (History)
5 users (show)

See Also:


Attachments
Cleanup (17.08 KB, patch)
2014-03-14 17:05 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed the bug (17.10 KB, patch)
2014-03-17 14:00 PDT, Ryosuke Niwa
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>