Bug 130226 – [Mac] Avoid creating DOMCSSStyleDeclaration in WebHTMLConverter

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

[Ryosuke Niwa]

Right now, we spend a significant amount of time creating Objective-C wrappers for CSSStyleDeclaration
and then looking up NSMutableDictionary.

[Ryosuke Niwa]

Created attachment 226655 [details]
Extracted HTMLConverterCaches

[Ryosuke Niwa]

Created attachment 226657 [details]
A/B test result

It's crazy.  This simple C++ rewrite is a 20% improvement.

[Andreas Kling]

Comment on attachment 226655 [details]
Extracted HTMLConverterCaches

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?

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 226655 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226655&action=review
>
> > 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.

Can I store ComputedStyleExtractor in the hash map and use it later?

[Andreas Kling]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 226655 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=226655&action=review
> >
> > > 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.
> 
> Can I store ComputedStyleExtractor in the hash map and use it later?

Totally.

[Ryosuke Niwa]

Committed r165635: <http://trac.webkit.org/changeset/165635>