Bug 33944 – [Android] bindings/v8/V8DOMWrapper.h[cpp] are missing guards for SVG, XPATH and XSLT features

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andrei Popescu]

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.

[Andrei Popescu]

Created attachment 47110 [details]
Add missing guards.

[David Levin]

Comment on attachment 47110 [details]
Add missing guards.

Just a few minor things to address.

> Index: WebCore/ChangeLog
> [phone]  Andrei Popescu  <[e-mail]>
> +
> +        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

[Andrei Popescu]

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
> > [phone]  Andrei Popescu  <[e-mail]>
> > +
> > +        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

[Adam Barth]

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.

[Andrei Popescu]

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