Bug 126251 - Add an owning smart pointer for RenderObjects and start using it.
Summary: Add an owning smart pointer for RenderObjects and start using it.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-26 21:15 PST by Andreas Kling
Modified: 2013-12-28 17:32 PST (History)
8 users (show)

See Also:


Attachments
Patch (13.90 KB, patch)
2013-12-26 21:42 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-12-26 21:15:37 PST
Add an owning smart pointer for RenderObjects and start using it.
Comment 1 Andreas Kling 2013-12-26 21:42:30 PST
Created attachment 220046 [details]
Patch
Comment 2 WebKit Commit Bot 2013-12-26 21:43:14 PST
Attachment 220046 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/dom/PseudoElement.cpp', u'Source/WebCore/rendering/RenderPtr.h', u'Source/WebCore/rendering/style/ContentData.cpp', u'Source/WebCore/rendering/style/ContentData.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/rendering/style/ContentData.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderPtr.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderPtr.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/RenderPtr.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/RenderPtr.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Antti Koivisto 2013-12-28 03:48:30 PST
It would be nice if we could eventually just use unique_ptr and split the destroy() stuff between subtree removal (invoked when that happens) and actual destruction (in destructor).
Comment 4 Zan Dobersek 2013-12-28 04:15:49 PST
Comment on attachment 220046 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This patch adds a RenderPtr pointer, essentially an OwnPtr for
> +        RenderObjects. The difference is that RenderPtr destroys the object
> +        by calling destroy() on it.

If this is the only difference you could switch to std::unique_ptr, with an std::default_delete<RenderObject> specialization that would call destroy().
Comment 5 Andreas Kling 2013-12-28 10:21:00 PST
(In reply to comment #4)
> (From update of attachment 220046 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220046&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        This patch adds a RenderPtr pointer, essentially an OwnPtr for
> > +        RenderObjects. The difference is that RenderPtr destroys the object
> > +        by calling destroy() on it.
> 
> If this is the only difference you could switch to std::unique_ptr, with an std::default_delete<RenderObject> specialization that would call destroy().

Wouldn't that require me to specialize separately for every single RenderObject subclass, or e.g std::unique_ptr<RenderBlock>'s destructor would just delete without calling destroy()?
Comment 6 Zan Dobersek 2013-12-28 12:17:06 PST
Yes, unfortunately I don't think there's a way to partially specialize std::default_delete for both the base class and its derivatives.

The subclass specializations could just inherit from the base class specialization, but that's still not very scalable:
template<> std::default_delete<RenderBlock> : std::default_delete<RenderObject> { };
Comment 7 WebKit Commit Bot 2013-12-28 17:32:43 PST
Comment on attachment 220046 [details]
Patch

Clearing flags on attachment: 220046

Committed r161115: <http://trac.webkit.org/changeset/161115>
Comment 8 WebKit Commit Bot 2013-12-28 17:32:45 PST
All reviewed patches have been landed.  Closing bug.