Bug 33675 - [Android] DOMWrapperMap::Visitor needs virtual destructor
Summary: [Android] DOMWrapperMap::Visitor needs virtual destructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Ben Murdoch
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-14 07:51 PST by Andrei Popescu
Modified: 2010-01-18 04:54 PST (History)
3 users (show)

See Also:


Attachments
Add the virtual destructor to DOMWrapperMap::Visitor. (1.12 KB, patch)
2010-01-14 07:56 PST, Andrei Popescu
eric: review-
Details | Formatted Diff | Diff
Add protected virtual destructor to DOMWrapperMap::Visitor. (1.23 KB, patch)
2010-01-15 04:29 PST, Andrei Popescu
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Popescu 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.
Comment 1 Andrei Popescu 2010-01-14 07:56:52 PST
Created attachment 46568 [details]
Add the virtual destructor to  DOMWrapperMap::Visitor.
Comment 2 Adam Barth 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?
Comment 3 Eric Seidel (no email) 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.
Comment 4 Andrei Popescu 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.
Comment 5 Andrei Popescu 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.
Comment 6 Adam Barth 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.  :)
Comment 7 Adam Barth 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".
Comment 8 Andrei Popescu 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.
Comment 9 Ben Murdoch 2010-01-18 04:47:19 PST
Will update Changelog as requested and land manually.
Comment 10 Ben Murdoch 2010-01-18 04:54:54 PST
Landed as r53401.