Bug 120619 - Use OwnPtr in the RenderLayerFilterInfo map
Summary: Use OwnPtr in the RenderLayerFilterInfo map
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-03 00:01 PDT by Darin Adler
Modified: 2013-09-04 10:02 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.16 KB, patch)
2013-09-03 00:05 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (36.00 KB, patch)
2013-09-03 21:36 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (36.00 KB, patch)
2013-09-03 21:45 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (480.51 KB, application/zip)
2013-09-03 22:39 PDT, Build Bot
no flags Details
Patch (35.89 KB, patch)
2013-09-03 22:40 PDT, Darin Adler
andersca: review+
andersca: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2013-09-03 00:01:38 PDT
Use OwnPtr in the RenderLayerFilterInfo map
Comment 1 Darin Adler 2013-09-03 00:05:13 PDT
Created attachment 210329 [details]
Patch
Comment 2 Early Warning System Bot 2013-09-03 00:09:43 PDT
Comment on attachment 210329 [details]
Patch

Attachment 210329 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1690163
Comment 3 Early Warning System Bot 2013-09-03 00:10:09 PDT
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
Comment 4 Build Bot 2013-09-03 00:32:23 PDT
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
Comment 5 Build Bot 2013-09-03 00:44:02 PDT
Comment on attachment 210329 [details]
Patch

Attachment 210329 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1692176
Comment 6 Build Bot 2013-09-03 00:50:04 PDT
Comment on attachment 210329 [details]
Patch

Attachment 210329 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1695047
Comment 7 Anders Carlsson 2013-09-03 07:41:39 PDT
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.
Comment 8 Darin Adler 2013-09-03 09:06:49 PDT
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.
Comment 9 Darin Adler 2013-09-03 21:36:08 PDT
Created attachment 210427 [details]
Patch
Comment 10 Darin Adler 2013-09-03 21:36:49 PDT
My second try makes more changes, eliminating some of the stranger things about the old code.
Comment 11 Early Warning System Bot 2013-09-03 21:43:14 PDT
Comment on attachment 210427 [details]
Patch

Attachment 210427 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1687510
Comment 12 Early Warning System Bot 2013-09-03 21:45:16 PDT
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
Comment 13 Darin Adler 2013-09-03 21:45:23 PDT
Created attachment 210428 [details]
Patch
Comment 14 Darin Adler 2013-09-03 22:20:23 PDT
I have an even better version where I turn RenderLayerFilterInfo into RenderLayer::FilterInfo.
Comment 15 Build Bot 2013-09-03 22:39:25 PDT
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
Comment 16 Build Bot 2013-09-03 22:39:28 PDT
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
Comment 17 Darin Adler 2013-09-03 22:40:01 PDT
Created attachment 210431 [details]
Patch
Comment 18 Anders Carlsson 2013-09-04 06:18:37 PDT
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.
Comment 19 Darin Adler 2013-09-04 09:17:19 PDT
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.
Comment 20 Darin Adler 2013-09-04 09:42:02 PDT
Committed r155047: <http://trac.webkit.org/changeset/155047>
Comment 21 Anders Carlsson 2013-09-04 10:02:26 PDT
(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.