Use OwnPtr in the RenderLayerFilterInfo map
Created attachment 210329 [details] Patch
Comment on attachment 210329 [details] Patch Attachment 210329 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1690163
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 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 on attachment 210329 [details] Patch Attachment 210329 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1692176
Comment on attachment 210329 [details] Patch Attachment 210329 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1695047
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 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.
Created attachment 210427 [details] Patch
My second try makes more changes, eliminating some of the stranger things about the old code.
Comment on attachment 210427 [details] Patch Attachment 210427 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1687510
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
Created attachment 210428 [details] Patch
I have an even better version where I turn RenderLayerFilterInfo into RenderLayer::FilterInfo.
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
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
Created attachment 210431 [details] Patch
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 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.
Committed r155047: <http://trac.webkit.org/changeset/155047>
(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.