AbstractWeakReferenceMap in V8DOMMap.h has virtual methods but no virtual destructor. This causes build errors on Android. This was added in http://trac.webkit.org/changeset/53944
Sorry about that. We need some automated way of catching these cases so we don't keep breaking the Android build for no reason. The ideal thing would be to set up an EWS bot to catch these errors. If you're interested in setting one up on your end, send me email and I'll tell you what's involved. (I would set one up in our current bot farm, but I don't know how to build Android.)
(In reply to comment #1) > Sorry about that. No problem > We need some automated way of catching these cases so we > don't keep breaking the Android build for no reason. The ideal thing would be > to set up an EWS bot to catch these errors. If you're interested in setting > one up on your end, send me email and I'll tell you what's involved. (I would > set one up in our current bot farm, but I don't know how to build Android.) Agreed, thanks for the offer of help. Android isn't yet fully upstreamed, so can't be built from webkit.org. We're working as fast as we can to fix that and will set up a bot as soon as possible.
Created attachment 47853 [details] Patch
Patch adds virtual destructors for AbstractWeakReferenceMap and AbstractWeakReferenceMap::Visitor Note that the destructor for AbstractWeakReferenceMap needs to be public as it's used by ScopedDOMDataStore, but the destructor for AbstractWeakReferenceMap::Visitor does not.
+ virtual ~AbstractWeakReferenceMap That's fine, but + virtual ~Visitor that's not correct. Objects are never deleted via Vistor pointers and inheriting from the Visitor interface shouldn't force an object to have a virtual destructor. I feel like we had that discussion on another bug and found another solution.
> Agreed, thanks for the offer of help. Android isn't yet fully upstreamed, so > can't be built from webkit.org. We're working as fast as we can to fix that and > will set up a bot as soon as possible. You can actually set up an EWS bot without the ability to build from webkit.org. You just need to fill in the update_webkit_command and the build_webkit_command methods in a AndroidPort class in this file: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/webkitport.py You can make them call arbitrary scripts, so as long as you have a machine that can update and build Android's WebKit port from the command line, you can set up an EWS bot.
> + virtual ~Visitor > > that's not correct. Objects are never deleted via Vistor pointers and > inheriting from the Visitor interface shouldn't force an object to have a > virtual destructor. I feel like we had that discussion on another bug and > found another solution. Are you thinking of Bug 33675 and http://trac.webkit.org/changeset/53401 ? This added a virtual destructor to WeakReferenceMap::Visitor. It was the last change to this file before http://trac.webkit.org/changeset/53944 removed WeakReferenceMap::Visitor and added AbstractWeakReferenceMap::Visitor. The outcome of the discussion was to make the destructor protected.
> You can actually set up an EWS bot without the ability to build from > webkit.org. You just need to fill in the update_webkit_command and the > build_webkit_command methods in a AndroidPort class in this file: > > http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/webkitport.py > > You can make them call arbitrary scripts, so as long as you have a machine that > can update and build Android's WebKit port from the command line, you can set > up an EWS bot. Ah, good to know, thanks. The problem still remains that because Android WebKit is forked from from webkit.org, there's a good chance that a patch won't apply, so we can't update the tree without manual conflict resolution. There would be lots of false positives, but it might be worth having anyway as it will give us a heads up as soon as a conflicting patch is landed. I'll look into it.
Comment on attachment 47853 [details] Patch Email from abarth: > Ah, that's right. r=me.
Comment on attachment 47853 [details] Patch Rejecting patch 47853 from review queue. steveblock@google.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed). Due to bug 30084 the commit-queue will require a restart after your change. Please contact eseidel@chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your reviewer rights.
Comment on attachment 47853 [details] Patch Will land manually
Landed manually as http://trac.webkit.org/changeset/54218 Closing bug as resolved fixed.
Sorry for not flipping the bit on the patch myself. I was away from my computer yesterday.
You can't r+ patches in bugzilla if you're not a reviewer. However you can still use the commit-queue. cq+'d patches will the review flag unset, will still be landed. But they'll fail to land if they have an OOPS in them, so you'd have to set the reviewer before hand or replace the reviewer line with some text about why review was not required. Adam Barth was working on some sort of "webkit-patch land-safely" command which used this feature of the commit-queue.