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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33944





--- Comment #3 from Andrei Popescu <andreip at google.com>  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 at 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.