Bug 73020 – CSSStyleDeclaration: Simplify removal and copying of multiple properties.

• Component: CSS
• Status: ASSIGNED

[Andreas Kling]

We currently have copyPropertiesInSet(int* properties, unsigned length) and removePropertiesInSet(int* properties, unsigned length).
If we change them into copyProperties(const ListHashSet<int>& propertyIDs) and removeProperties(const ListHashSet<int>& propertyIDs), we can tidy up a number of things.
Patch coming.

[Andreas Kling]

Created attachment 116358 [details]
Patch

[Antti Koivisto]

Comment on attachment 116358 [details]
Patch

A performance test would be nice. I'm not sure about the performance properties of ListHashSet though I would be suprised if it is a problem here.

[Andreas Kling]

Comment on attachment 116358 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:97
> +        // Inheritable editing properties.
> +        const ListHashSet<int>& inheritablePropertyIDs = inheritableEditingPropertyIDs();
> +        ListHashSet<int>::const_iterator end = inheritablePropertyIDs.end();
> +        for (ListHashSet<int>::const_iterator it = inheritablePropertyIDs.begin(); it != end; ++it)
> +            propertyIDs.add(*it);

We should just use the ListHashSet copy ctor instead of this code chunk.

[Andreas Kling]

Comment on attachment 116358 [details]
Patch

I'll return to this at some point..

[Alexis Menard (darktears)]

Created attachment 131315 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #5)
> Created an attachment (id=131315) [details]
> Patch

Rebased version but we do have some performance problems. ListHashSet is not as fast as we thought. It needs to be investigated.

[Alexis Menard (darktears)]

Created attachment 131953 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #7)
> Created an attachment (id=131953) [details]
> Patch

I changed ListHashSet with HashSet and it seems that if I use the benchmark of https://bugs.webkit.org/show_bug.cgi?id=81155 we don't regress performance wise, it seems that it is slightly improved.

[Benjamin Poulain]

> I changed ListHashSet with HashSet and it seems that if I use the benchmark of https://bugs.webkit.org/show_bug.cgi?id=81155 we don't regress performance wise, it seems that it is slightly improved.

If the performance does not improve, I wonder if the issue is not from the other loop. Have you tried replacing the property vector by a ListHashSet?

[WebKit Review Bot]

Comment on attachment 131953 [details]
Patch

Attachment 131953 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11956457

New failing tests:
editing/pasteboard/data-transfer-items.html
fast/events/ondrop-text-html.html
editing/pasteboard/onpaste-text-html.html

[Benjamin Poulain]

I just looked at the last patch.

This improves removeProperties() but make copyProperties() worse. I don't agree this simplifies the code due to copyProperties().

Do you observe any gains from the improvement of isEditingProperty() and removeProperties()?