33944 2010-01-21 05:10:20 -0800 [Android] bindings/v8/V8DOMWrapper.h[cpp] are missing guards for SVG, XPATH and XSLT features 2010-01-23 05:00:26 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Android Android RESOLVED FIXED P2 Normal --- 0 andreip webkit-unassigned android-webkit-unforking oldest_to_newest 183167 0 andreip 2010-01-21 05:10:20 -0800 Iniside V8DOMWrapper.h[cpp], the code for SVG, XPATH and XSLT features is not guarded by the appropriate #if ENABLE(feature) macros. V8DOMWrapper.cpp includes ChromiumBridge.h for no reason. 183169 1 47110 andreip 2010-01-21 05:17:54 -0800 Created attachment 47110 Add missing guards. 183216 2 47110 levin 2010-01-21 09:14:15 -0800 Comment on attachment 47110 Add missing guards. Just a few minor things to address. > Index: WebCore/ChangeLog > +2010-01-21 Andrei Popescu <andreip@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [Android] bindings/v8/V8DOMWrapper.h[cpp] are missing guards for SVG, XPATH and XSLT features > + https://bugs.webkit.org/show_bug.cgi?id=33944 > + > + Iniside V8DOMWrapper.h[cpp], the code for SVG, XPATH and XSLT features is not guarded typo: Iniside Please adjust the comment appropriately when you remove the if ENABLE(SVG) for the SVG headers (see comment below). > Index: WebCore/bindings/v8/V8DOMWrapper.cpp > #include "Notification.h" > -#include "SVGElementInstance.h" Please leave this here and add #include "SVGPathSeg.h" (see comments below). > #include "ScriptController.h" > #include "V8AbstractEventListener.h" > #include "V8Binding.h" > @@ -63,6 +61,7 @@ > #include "WorkerContextExecutionProxy.h" > > #if ENABLE(SVG) > +#include "SVGElementInstance.h" SVGElementInstance.h correctly has #if ENABLE(SVG) inside of it so this is unnecessary. (If something is wrong with the way the header does the if ENABLE, please fix that.) > #include "SVGPathSeg.h" SVGPathSeg.h also has #if ENABLE(SVG) inside of it. So the whole if ENABLE(SVG) that is guarding these includes should go away and these headers should just be sorted with the other headers (as SVGElementInstance.h was before). > #endif 183237 3 andreip 2010-01-21 10:09:18 -0800 Thanks a lot David! (In reply to comment #2) > (From update of attachment 47110 [details]) > Just a few minor things to address. > > > Index: WebCore/ChangeLog > > +2010-01-21 Andrei Popescu <andreip@google.com> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + [Android] bindings/v8/V8DOMWrapper.h[cpp] are missing guards for SVG, XPATH and XSLT features > > + https://bugs.webkit.org/show_bug.cgi?id=33944 > > + > > + Iniside V8DOMWrapper.h[cpp], the code for SVG, XPATH and XSLT features is not guarded > > typo: Iniside > Fixed. > Please adjust the comment appropriately when you remove the if ENABLE(SVG) for > the SVG headers (see comment below). > > > Index: WebCore/bindings/v8/V8DOMWrapper.cpp > > #include "Notification.h" > > -#include "SVGElementInstance.h" > > Please leave this here and add #include "SVGPathSeg.h" (see comments below). > Done. > > #include "ScriptController.h" > > #include "V8AbstractEventListener.h" > > #include "V8Binding.h" > > @@ -63,6 +61,7 @@ > > #include "WorkerContextExecutionProxy.h" > > > > #if ENABLE(SVG) > > +#include "SVGElementInstance.h" > > SVGElementInstance.h correctly has #if ENABLE(SVG) inside of it so this is > unnecessary. (If something is wrong with the way the header does the if ENABLE, > please fix that.) > I see, I will do that if I find a header with such a problem. > > #include "SVGPathSeg.h" > > SVGPathSeg.h also has #if ENABLE(SVG) inside of it. So the whole if > ENABLE(SVG) that is guarding these includes should go away and these headers > should just be sorted with the other headers (as SVGElementInstance.h was > before). > Done. Steve Block was kind enough to commit a new patch that has the changes you requested. Thanks, Andrei 183859 4 47110 abarth 2010-01-22 16:00:38 -0800 Comment on attachment 47110 Add missing guards. This patch does not apply cleanly to the most recent version. I haven't looked whether the merge is trivial. 183983 5 andreip 2010-01-23 05:00:26 -0800 (In reply to comment #4) > (From update of attachment 47110 [details]) > This patch does not apply cleanly to the most recent version. I haven't looked > whether the merge is trivial. That would be because it's landed already: http://trac.webkit.org/changeset/53630 I think Steve simply forgot to update the bug after he landed the patch. It is my fault for rushing him. 47110 2010-01-21 05:17:54 -0800 2010-01-22 16:00:38 -0800 Add missing guards. 33944.patch text/plain 3211 andreip SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MzYyMikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjAgQEAKKzIwMTAtMDEtMjEgIEFuZHJlaSBQb3Blc2N1ICA8YW5kcmVpcEBnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IFtBbmRyb2lkXSBiaW5kaW5ncy92OC9WOERPTVdyYXBwZXIuaFtjcHBdIGFyZSBtaXNzaW5nIGd1 YXJkcyBmb3IgU1ZHLCBYUEFUSCBhbmQgWFNMVCBmZWF0dXJlcworICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzM5NDQKKworICAgICAgICBJbmlzaWRlIFY4 RE9NV3JhcHBlci5oW2NwcF0sIHRoZSBjb2RlIGZvciBTVkcsIFhQQVRIIGFuZCBYU0xUIGZlYXR1 cmVzIGlzIG5vdCBndWFyZGVkCisgICAgICAgIGJ5IHRoZSBhcHByb3ByaWF0ZSAjaWYgRU5BQkxF KGZlYXR1cmUpIG1hY3Jvcy4gQWRkIHRoZSBtaXNzaW5nIGd1YXJkcy4KKyAgICAgICAgVjhET01X cmFwcGVyLmNwcCBpbmNsdWRlcyBDaHJvbWl1bUJyaWRnZS5oIGZvciBubyByZWFzb24uIFJlbW92 ZSB0aGUgaW5jbHVkZS4KKworICAgICAgICBObyBuZXcgdGVzdHMgbmVlZGVkLCBmdW5jdGlvbmFs aXR5IG5vdCBjaGFuZ2VkLgorCisgICAgICAgICogYmluZGluZ3MvdjgvVjhET01XcmFwcGVyLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OlY4RE9NV3JhcHBlcjo6Z2V0VGVtcGxhdGUpOgorICAgICAg ICAqIGJpbmRpbmdzL3Y4L1Y4RE9NV3JhcHBlci5oOgorCiAyMDEwLTAxLTIxICBTdGV2ZSBCbG9j ayAgPHN0ZXZlYmxvY2tAZ29vZ2xlLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkIHNwZWN1bGF0 aXZlIGJ1aWxkIGZpeCBmb3IgV2luZG93cy4KSW5kZXg6IFdlYkNvcmUvYmluZGluZ3MvdjgvVjhE T01XcmFwcGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9NV3Jh cHBlci5jcHAJKHJldmlzaW9uIDUzNjIxKQorKysgV2ViQ29yZS9iaW5kaW5ncy92OC9WOERPTVdy YXBwZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zMiwxMiArMzIsMTAgQEAKICNpbmNsdWRlICJW OERPTVdyYXBwZXIuaCIKIAogI2luY2x1ZGUgIkNTU011dGFibGVTdHlsZURlY2xhcmF0aW9uLmgi Ci0jaW5jbHVkZSAiQ2hyb21pdW1CcmlkZ2UuaCIKICNpbmNsdWRlICJET01PYmplY3RzSW5jbHVk ZS5oIgogI2luY2x1ZGUgIkRvY3VtZW50TG9hZGVyLmgiCiAjaW5jbHVkZSAiRnJhbWVMb2FkZXJD bGllbnQuaCIKICNpbmNsdWRlICJOb3RpZmljYXRpb24uaCIKLSNpbmNsdWRlICJTVkdFbGVtZW50 SW5zdGFuY2UuaCIKICNpbmNsdWRlICJTY3JpcHRDb250cm9sbGVyLmgiCiAjaW5jbHVkZSAiVjhB YnN0cmFjdEV2ZW50TGlzdGVuZXIuaCIKICNpbmNsdWRlICJWOEJpbmRpbmcuaCIKQEAgLTYzLDYg KzYxLDcgQEAKICNpbmNsdWRlICJXb3JrZXJDb250ZXh0RXhlY3V0aW9uUHJveHkuaCIKIAogI2lm IEVOQUJMRShTVkcpCisjaW5jbHVkZSAiU1ZHRWxlbWVudEluc3RhbmNlLmgiCiAjaW5jbHVkZSAi U1ZHUGF0aFNlZy5oIgogI2VuZGlmCiAKQEAgLTMyNiwxMiArMzI1LDE2IEBAIHY4OjpQZXJzaXN0 ZW50PHY4OjpGdW5jdGlvblRlbXBsYXRlPiBWOEQKICAgICAgICAgZGVzY3JpcHRvci0+U2V0Q2Fs bEhhbmRsZXIoVVNFX0NBTExCQUNLKFhNTEh0dHBSZXF1ZXN0Q29uc3RydWN0b3IpKTsKICAgICAg ICAgYnJlYWs7CiAgICAgfQorI2lmIEVOQUJMRShYUEFUSCkKICAgICBjYXNlIFY4Q2xhc3NJbmRl eDo6WFBBVEhFVkFMVUFUT1I6CiAgICAgICAgIGRlc2NyaXB0b3ItPlNldENhbGxIYW5kbGVyKFVT RV9DQUxMQkFDSyhYUGF0aEV2YWx1YXRvckNvbnN0cnVjdG9yKSk7CiAgICAgICAgIGJyZWFrOwor I2VuZGlmCisjaWYgRU5BQkxFKFhTTFQpCiAgICAgY2FzZSBWOENsYXNzSW5kZXg6OlhTTFRQUk9D RVNTT1I6CiAgICAgICAgIGRlc2NyaXB0b3ItPlNldENhbGxIYW5kbGVyKFVTRV9DQUxMQkFDSyhY U0xUUHJvY2Vzc29yQ29uc3RydWN0b3IpKTsKICAgICAgICAgYnJlYWs7CisjZW5kaWYKICAgICBk ZWZhdWx0OgogICAgICAgICBicmVhazsKICAgICB9CkluZGV4OiBXZWJDb3JlL2JpbmRpbmdzL3Y4 L1Y4RE9NV3JhcHBlci5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvYmluZGluZ3MvdjgvVjhET01X cmFwcGVyLmgJKHJldmlzaW9uIDUzNjIxKQorKysgV2ViQ29yZS9iaW5kaW5ncy92OC9WOERPTVdy YXBwZXIuaAkod29ya2luZyBjb3B5KQpAQCAtMTk0LDcgKzE5NCw3IEBAIG5hbWVzcGFjZSBXZWJD b3JlIHsKIAogICAgICAgICBzdGF0aWMgUGFzc1JlZlB0cjxFdmVudExpc3RlbmVyPiBnZXRFdmVu dExpc3RlbmVyKFY4UHJveHkqIHByb3h5LCB2ODo6TG9jYWw8djg6OlZhbHVlPiB2YWx1ZSwgYm9v bCBpc0F0dHJpYnV0ZSwgTGlzdGVuZXJMb29rdXBUeXBlIGxvb2t1cCk7CiAKLQorI2lmIEVOQUJM RShYUEFUSCkKICAgICAgICAgLy8gWFBhdGgtcmVsYXRlZCB1dGlsaXRpZXMKICAgICAgICAgc3Rh dGljIFJlZlB0cjxYUGF0aE5TUmVzb2x2ZXI+IGdldFhQYXRoTlNSZXNvbHZlcih2ODo6SGFuZGxl PHY4OjpWYWx1ZT4gdmFsdWUsIFY4UHJveHkqIHByb3h5ID0gMCkKICAgICAgICAgewpAQCAtMjA1 LDcgKzIwNSw3IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKICAgICAgICAgICAgICAgICByZXNvbHZl ciA9IFY4Q3VzdG9tWFBhdGhOU1Jlc29sdmVyOjpjcmVhdGUocHJveHksIHZhbHVlLT5Ub09iamVj dCgpKTsKICAgICAgICAgICAgIHJldHVybiByZXNvbHZlcjsKICAgICAgICAgfQotCisjZW5kaWYK ICAgICAgICAgLy8gRE9NSW1wbGVtZW50YXRpb24gaXMgYSBzaW5nbGV0b24gYW5kIGl0IGlzIGhh bmRsZWQgaW4gYSBzcGVjaWFsCiAgICAgICAgIC8vIHdheS4gQSB3cmFwcGVyIGlzIGdlbmVyYXRl ZCBwZXIgZG9jdW1lbnQgYW5kIHN0b3JlZCBpbiBhbgogICAgICAgICAvLyBpbnRlcm5hbCBmaWVs ZCBvZiB0aGUgZG9jdW1lbnQuCg==