Bug 120619 – Use OwnPtr in the RenderLayerFilterInfo map

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Darin Adler]

Use OwnPtr in the RenderLayerFilterInfo map

[Darin Adler]

Created attachment 210329 [details]
Patch

[Early Warning System Bot]

Comment on attachment 210329 [details]
Patch

Attachment 210329 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1690163

[Early Warning System Bot]

Comment on attachment 210329 [details]
Patch

Attachment 210329 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1693155

[Build Bot]

Comment on attachment 210329 [details]
Patch

Attachment 210329 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1689212

[Build Bot]

Comment on attachment 210329 [details]
Patch

Attachment 210329 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1692176

[Build Bot]

Comment on attachment 210329 [details]
Patch

Attachment 210329 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1695047

[Anders Carlsson]

Comment on attachment 210329 [details]
Patch

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

I wonder if the optimization to delete the map if it's empty is actually useful here.

> Source/WebCore/rendering/RenderLayerFilterInfo.cpp:57
> +static OwnPtr<RenderLayerFilterInfoMap>& filterMap()

I wish we had a better name for these types of global smart pointer variable slots.

[Darin Adler]

Comment on attachment 210329 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerFilterInfo.cpp:94
> +            filterMap() = nullptr;

If we didn’t need to destroy the map here, then we would not have to have filterMap() return an OwnPtr& at all. It could just return a pointer. It actually seems dumb to destroy this global map.

[Darin Adler]

Created attachment 210427 [details]
Patch

[Darin Adler]

My second try makes more changes, eliminating some of the stranger things about the old code.

[Early Warning System Bot]

Comment on attachment 210427 [details]
Patch

Attachment 210427 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1687510

[Early Warning System Bot]

Comment on attachment 210427 [details]
Patch

Attachment 210427 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1694441

[Darin Adler]

Created attachment 210428 [details]
Patch

[Darin Adler]

I have an even better version where I turn RenderLayerFilterInfo into RenderLayer::FilterInfo.

[Build Bot]

Comment on attachment 210428 [details]
Patch

Attachment 210428 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1692446

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 210430 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Darin Adler]

Created attachment 210431 [details]
Patch

[Anders Carlsson]

Comment on attachment 210431 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerFilterInfo.cpp:50
> +    static Map& map = *new Map;
> +    return map;

I think you can still use NeverDestroyed here.

> Source/WebCore/rendering/RenderLayerFilterInfo.h:51
> +class RenderLayer::FilterInfo
> +#if ENABLE(CSS_SHADERS) && ENABLE(SVG)
> +    FINAL : public CustomFilterProgramClient, public CachedSVGDocumentClient
> +#elif ENABLE(CSS_SHADERS)
> +    FINAL : public CustomFilterProgramClient
> +#else
> +    FINAL : public CachedSVGDocumentClient

I'd just put the FINAL on the previous line instead of duplicating it three times.

> Source/WebCore/rendering/RenderLayerFilterInfo.h:91
> +    typedef HashMap<const RenderLayer*, OwnPtr<FilterInfo>> Map;
> +    static Map& map();

I think you should just use the HashMap type directly instead of the typedef.

[Darin Adler]

Comment on attachment 210431 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayerFilterInfo.cpp:50
>> +    return map;
> 
> I think you can still use NeverDestroyed here.

Oh, good idea!

>> Source/WebCore/rendering/RenderLayerFilterInfo.h:51
>> +    FINAL : public CachedSVGDocumentClient
> 
> I'd just put the FINAL on the previous line instead of duplicating it three times.

Oops, got the condition wrong. The last one is supposed to be:

    #elif ENABLE(SVG)

That’s why FINAL is in the duplicate lines, because it can’t be used in the fourth case, when there is no base class at all.

>> Source/WebCore/rendering/RenderLayerFilterInfo.h:91
>> +    static Map& map();
> 
> I think you should just use the HashMap type directly instead of the typedef.

I like the typedef for the implementation of the map() function, to avoid repeating the type two more times. But I can repeat it three times.

[Darin Adler]

Committed r155047: <http://trac.webkit.org/changeset/155047>

[Anders Carlsson]

(In reply to comment #19)
> > I think you should just use the HashMap type directly instead of the typedef.
> 
> I like the typedef for the implementation of the map() function, to avoid repeating the type two more times. But I can repeat it three times.

I don’t like having to go hunting for the real type whenever I need to know what the memory semantics for a collection is.