RESOLVED FIXED33675
[Android] DOMWrapperMap::Visitor needs virtual destructor 
https://bugs.webkit.org/show_bug.cgi?id=33675
Summary [Android] DOMWrapperMap::Visitor needs virtual destructor
Andrei Popescu
Reported 2010-01-14 07:51:12 PST
For reasons similar to https://bugs.webkit.org/show_bug.cgi?id=33390 The DOMWrapperMap::Visitor class in V8DOMMap.h needs a virtual destructor.
Attachments
Add the virtual destructor to DOMWrapperMap::Visitor. (1.12 KB, patch)
2010-01-14 07:56 PST, Andrei Popescu 		eric: review-
DetailsFormatted DiffDiff
Add protected virtual destructor to DOMWrapperMap::Visitor. (1.23 KB, patch)
2010-01-15 04:29 PST, Andrei Popescu 		abarth: review+
abarth: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Andrei Popescu
Comment 1 2010-01-14 07:56:52 PST
Created attachment 46568 [details] Add the virtual destructor to DOMWrapperMap::Visitor.
Adam Barth
Comment 2 2010-01-14 16:54:05 PST
Comment on attachment 46568 [details] Add the virtual destructor to DOMWrapperMap::Visitor. This isn't right. Vistor objects are never deleted via the Vistor type. Can we make the destructor private instead?
Eric Seidel (no email)
Comment 3 2010-01-14 17:20:27 PST
Comment on attachment 46568 [details] Add the virtual destructor to DOMWrapperMap::Visitor. r- based on Adam's comment above.
Andrei Popescu
Comment 4 2010-01-15 04:24:04 PST
(In reply to comment #2) > (From update of attachment 46568 [details]) > This isn't right. Vistor objects are never deleted via the Vistor type. Can > we make the destructor private instead? Well, they're not deleted now but if someone else added code to do so, we'd have a disaster on our hand, right? Clearly, we can make it private either as derived classes would not be able to call it. What we can do is make it protected, instead, if you are sure that one is never meant to delete objects instantiated from derived types via a pointer to the base type.
Andrei Popescu
Comment 5 2010-01-15 04:29:52 PST
Created attachment 46671 [details] Add protected virtual destructor to DOMWrapperMap::Visitor. Made virtual dtor protected. Also fixed indentation of visitDOMWrapper.
Adam Barth
Comment 6 2010-01-16 15:43:13 PST
Comment on attachment 46671 [details] Add protected virtual destructor to DOMWrapperMap::Visitor. Looks good, but please update the ChangeLog to match what the patch actually does. :)
Adam Barth
Comment 7 2010-01-16 15:44:42 PST
Also, you patch does not apply properly to top of tree (according to the EWS). If you'd like this landed automatically, please either update your tree or create the patch using svn-create-patch or "webkit-patch upload".
Andrei Popescu
Comment 8 2010-01-18 04:44:11 PST
Thanks for the review Adam. (In reply to comment #7) > Also, you patch does not apply properly to top of tree (according to the EWS). > If you'd like this landed automatically, please either update your tree or > create the patch using svn-create-patch or "webkit-patch upload". Hmm, but svn-create-patch is exactly what I used to create the patch. Anyway, Ben Murdoch was kind enough to help land this manually.
Ben Murdoch
Comment 9 2010-01-18 04:47:19 PST
Will update Changelog as requested and land manually.
Ben Murdoch
Comment 10 2010-01-18 04:54:54 PST
Landed as r53401.
Note You need to log in before you can comment on or make changes to this bug.