| Summary: | Use OwnPtr in the RenderLayerFilterInfo map | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | Darin Adler <darin> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | andersca, buildbot, commit-queue, dino, esprehn+autocc, glenn, kondapallykalyan, rniwa, simon.fraser, webkit-ews | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Darin Adler
2013-09-03 00:01:38 PDT
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. |