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.
Created attachment 47110 [details] Add missing guards.
Comment on attachment 47110 [details] 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
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
Comment on attachment 47110 [details] Add missing guards. This patch does not apply cleanly to the most recent version. I haven't looked whether the merge is trivial.
(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.