Bug 33944

Summary: [Android] bindings/v8/V8DOMWrapper.h[cpp] are missing guards for SVG, XPATH and XSLT features
Product: WebKit Reporter: Andrei Popescu <andreip>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: android-webkit-unforking
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Attachments:
Description Flags
Add missing guards. levin: review+, abarth: commit-queue-

Description Andrei Popescu 2010-01-21 05:10:20 PST
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.
Comment 1 Andrei Popescu 2010-01-21 05:17:54 PST
Created attachment 47110 [details]
Add missing guards.
Comment 2 David Levin 2010-01-21 09:14:15 PST
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
Comment 3 Andrei Popescu 2010-01-21 10:09:18 PST
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 4 Adam Barth 2010-01-22 16:00:38 PST
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.
Comment 5 Andrei Popescu 2010-01-23 05:00:26 PST
(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.