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-

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-	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
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.