[webkit-reviews] review granted: [Bug 34812] [Android] SVG code in V8 bindings is not properly guarded : [Attachment 48518] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Steve Block
<steveblock at google.com>'s request for review:
Bug 34812: [Android] SVG code in V8 bindings is not properly guarded
https://bugs.webkit.org/show_bug.cgi?id=34812

Attachment 48518: Patch
https://bugs.webkit.org/attachment.cgi?id=48518&action=review

------- Additional Comments from David Levin <levin at chromium.org>
r=me (with the suggested changes done).


General note: The 
  #if ENABLE(SVG) 
in the code is fine, but the
  #if ENABLE(SVG) 
to exclude headers should be removed. The headers themselves should have the if
inside of them. (I spot checked them and every one that I looked at had this.)


> Index: WebCore/bindings/v8/V8DOMWrapper.cpp

This file should be reverted (since it is all about excluding header includes
using ENABLE).

> Index: WebCore/bindings/v8/V8Proxy.cpp

This file should be reverted (since it is all about excluding header includes
using ENABLE).

> Index: WebCore/bindings/v8/custom/V8CSSValueCustom.cpp
> ===================================================================
> --- WebCore/bindings/v8/custom/V8CSSValueCustom.cpp	(revision 54622)
> +++ WebCore/bindings/v8/custom/V8CSSValueCustom.cpp	(working copy)
> @@ -33,9 +33,12 @@
>  
>  #include "V8CSSPrimitiveValue.h"
>  #include "V8CSSValueList.h"
> +#include "V8WebKitCSSTransformValue.h"
> +
> +#if ENABLE(SVG)
>  #include "V8SVGColor.h"
>  #include "V8SVGPaint.h"
> -#include "V8WebKitCSSTransformValue.h"
> +#endif

This #if ENABLE should go away completely (and the headers sorted
appropriately).

> Index: WebCore/bindings/v8/custom/V8DocumentCustom.cpp
>  #include "V8IsolatedContext.h"
>  #include "V8Node.h"
>  #include "V8Proxy.h"
> -#include "V8SVGDocument.h"
>  #include "V8WebGLRenderingContext.h"
>  #include "V8XPathNSResolver.h"
>  #include "V8XPathResult.h"
>  
> +#if ENABLE(SVG)
> +#include "V8SVGDocument.h"
> +#endif

This part of the change should be undone.


> Index: WebCore/bindings/v8/custom/V8ElementCustom.cpp
> ===================================================================
> --- WebCore/bindings/v8/custom/V8ElementCustom.cpp	(revision 54622)
> +++ WebCore/bindings/v8/custom/V8ElementCustom.cpp	(working copy)
> @@ -45,7 +45,10 @@
>  #include "V8BindingState.h"
>  #include "V8HTMLElement.h"
>  #include "V8Proxy.h"
> +
> +#if ENABLE(SVG)
>  #include "V8SVGElement.h"
> +#endif

This part of the change should be undone.

> Index: WebCore/bindings/v8/custom/V8EventCustom.cpp

This file should be reverted (since it is all about excluding header includes
using ENABLE).