Bug 33675

Summary:

[Android] DOMWrapperMap::Visitor needs virtual destructor

Product:

WebKit

Reporter:

Andrei Popescu <andreip>

Component:

WebCore Misc.

Assignee:

Ben Murdoch <benm>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, android-webkit-unforking, benm

Priority:

Version:

528+ (Nightly build)

Hardware:

Android

OS:

Android

Attachments:

Description	Flags
Add the virtual destructor to DOMWrapperMap::Visitor.	eric: review-
Add protected virtual destructor to DOMWrapperMap::Visitor.	abarth: review+, abarth: commit-queue-

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.