Bug 34812 – [Android] SVG code in V8 bindings is not properly guarded

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

[Steve Block]

SVG code in V8 bindings are not properly guarded

This includes generated headers and method call sites which need to be guarded by ENABLE(SVG)
These were introduced in http://trac.webkit.org/changeset/54305

[Steve Block]

Created attachment 48518 [details]
Patch

[Steve Block]

See also http://trac.webkit.org/changeset/54150 and http://trac.webkit.org/changeset/54153

[David Levin]

Comment on attachment 48518 [details]
Patch

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

[Steve Block]

>   #if ENABLE(SVG) 
> to exclude headers should be removed. The headers themselves should have the if
> inside of them.
Is that also the case for generated headers? Having the guard around the include site means that generated headers for unsupported features don't need to be built.

[David Levin]

(In reply to comment #4)
> >   #if ENABLE(SVG) 
> > to exclude headers should be removed. The headers themselves should have the if
> > inside of them.
> Is that also the case for generated headers? Having the guard around the
> include site means that generated headers for unsupported features don't need
> to be built.

I see what you are saying. I though the idl typically had the correct guard specified, so the header is generated with the appropriate if ENABLE.

But you're right that this means the header is generated when it doesn't need to be.  If at all possible, I think that is better than adding extra clutter to the files.

[Steve Block]

> I see what you are saying. I though the idl typically had the correct guard
> specified, so the header is generated with the appropriate if ENABLE.
Yes, I think they should be generated with the appropriate guard.

> But you're right that this means the header is generated when it doesn't need
> to be.  If at all possible, I think that is better than adding extra clutter to
> the files.
OK, I'll update the patch to not guard the include sites, and will make sure the generated headers include the guards.

[Steve Block]

Landed updated patch as http://trac.webkit.org/changeset/54650

closing bug as resolved fixed.

[Jeremy Orlow]

(In reply to comment #5)
> (In reply to comment #4)
> > >   #if ENABLE(SVG) 
> > > to exclude headers should be removed. The headers themselves should have the if
> > > inside of them.
> > Is that also the case for generated headers? Having the guard around the
> > include site means that generated headers for unsupported features don't need
> > to be built.
> 
> I see what you are saying. I though the idl typically had the correct guard
> specified, so the header is generated with the appropriate if ENABLE.
> 
> But you're right that this means the header is generated when it doesn't need
> to be.  If at all possible, I think that is better than adding extra clutter to
> the files.

For the record, I'm not sure I agree.  Making the build logic more complicated in order to save a couple lines of pre-processor here and there really seems like a net loss.  Especially since any WebKit developer understands the "#if ENABLE" logic but not necessarily the GYP/Makefile logic.

https://bugs.webkit.org/show_bug.cgi?id=34814 and
https://bugs.webkit.org/show_bug.cgi?id=34815

seem a lot worse than simply adding these guards whenever we're including files that are generated.

[David Levin]

(In reply to comment #8)
> For the record, I'm not sure I agree.  Making the build logic more complicated

> in order to save a couple lines of pre-processor here and there really seems
> like a net loss.  Especially since any WebKit developer understands the "#if
> ENABLE" logic but not necessarily the GYP/Makefile logic.

You're saying that developers don't know how to add a file to the build? If so, that is really broken.

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

This looks like a really trivial one line change and much simpler that adding if ENABLE's  in lots of different filfes to exclude some header file, so I don't get why this is worse.
 
> https://bugs.webkit.org/show_bug.cgi?id=34815

This seems like standard boiler plate in the makefile for idl files. It is a shame that it requires so much boilerplate but that's the way these particular build files are.

> seem a lot worse than simply adding these guards whenever we're including files
> that are generated.

fwiw, the guidelines against doing if ENABLE's for headers is a common recommendation in WebKit reviews. It is more confusing if folks need to remember don't do this except when the included file is autogenerated through some process (every additional rule/exception is more mental overhead).

Lastly, we should strive to make source files as clean and easy to read as possible and not having unnecessary if ENABLEs fits nicely into that goal. (And they are unnecessary with the simple makefile additions.)

[David Levin]

(In reply to comment #8)
> Especially since any WebKit developer understands the "#if
> ENABLE" logic but not necessarily the GYP/Makefile logic.

btw, if you look at https://bugs.webkit.org/attachment.cgi?id=48518&action=prettypatch , you'll see that a file was incorrectly included in the if ENABLE.

Also if you look at https://bugs.webkit.org/attachment.cgi?id=48520&action=prettypatch , you'll see that the if ENABLE was done incorrectly.

The point being that the if ENABLE stuff can be hard to get right for including the headers as well.

[Jeremy Orlow]

(In reply to comment #9)
> (In reply to comment #8)
> > For the record, I'm not sure I agree.  Making the build logic more complicated
> 
> > in order to save a couple lines of pre-processor here and there really seems
> > like a net loss.  Especially since any WebKit developer understands the "#if
> > ENABLE" logic but not necessarily the GYP/Makefile logic.
> 
> You're saying that developers don't know how to add a file to the build? If so,
> that is really broken.

I don't know how to add a file to the android build, no.  Bindings are especially mysterious across ports.

> > https://bugs.webkit.org/show_bug.cgi?id=34814
> 
> This looks like a really trivial one line change and much simpler that adding
> if ENABLE's  in lots of different filfes to exclude some header file, so I
> don't get why this is worse.
> 
> > https://bugs.webkit.org/show_bug.cgi?id=34815
> 
> This seems like standard boiler plate in the makefile for idl files. It is a
> shame that it requires so much boilerplate but that's the way these particular
> build files are.
> 
> > seem a lot worse than simply adding these guards whenever we're including files
> > that are generated.
> 
> fwiw, the guidelines against doing if ENABLE's for headers is a common
> recommendation in WebKit reviews. It is more confusing if folks need to
> remember don't do this except when the included file is autogenerated through
> some process (every additional rule/exception is more mental overhead).
> 
> Lastly, we should strive to make source files as clean and easy to read as
> possible and not having unnecessary if ENABLEs fits nicely into that goal. (And
> they are unnecessary with the simple makefile additions.)

I agree that we should avoid it when dealing with files that are not auto-generated in a way that's conditional because there we can simply put the #if ENABLE in the file.  But if the file might not exist, making the build system more complicated and having to do the change in multiple places is not worth it.

And really. In the top of a hand full of files like that, do you really find a few #if ENABLE so distracting?  I'm not sure I believe that.

I think we're taking a good rule of thumb to the extreme here.

[David Levin]

(In reply to comment #11)
> I don't know how to add a file to the android build, no.  Bindings are
> especially mysterious across ports.

Well that is not the case here because Steve Block does know but it is a much bigger problem. (Also, when these items were added, they weren't ifdef'ed anyway.)

> #if ENABLE in the file.  But if the file might not exist, making the build
> system more complicated and having to do the change in multiple places is not
> worth it.

The build isn't more complicated. In one change he added a *single* line. That is certainly not more complicated than one he did before. In the other case, he copied some boilerplate. Is that complicated boilerplate? Maybe, but it is just boilerplate not some complicated addition to the build system. (Perhaps some effort could be put into making the boilerplate simpler if Android finds this to be a problem.)

> And really. In the top of a hand full of files like that, do you really find a
> few #if ENABLE so distracting?  I'm not sure I believe that.

Readability is the result of one thing but a lot of little things. Sure this is a little thing we could compromise on something that makes the file less readable. Do this enough and your file becomes a lot less readable.

Lastly, you're ignoring the mental effort from keeping exception in mind for developers. "Don't do if ENABLE's for headers unless the if ENABLE is for a file that is autogenerated. Is this header file autogenerated?" Instead of simply "Don't do if ENABLE's for headers."

[Jeremy Orlow]

(In reply to comment #12)
> (In reply to comment #11)
> > I don't know how to add a file to the android build, no.  Bindings are
> > especially mysterious across ports.
> 
> Well that is not the case here because Steve Block does know but it is a much
> bigger problem. (Also, when these items were added, they weren't ifdef'ed
> anyway.)
> 
> > #if ENABLE in the file.  But if the file might not exist, making the build
> > system more complicated and having to do the change in multiple places is not
> > worth it.
> 
> The build isn't more complicated. In one change he added a *single* line. That
> is certainly not more complicated than one he did before. In the other case, he
> copied some boilerplate. Is that complicated boilerplate? Maybe, but it is just
> boilerplate not some complicated addition to the build system. (Perhaps some
> effort could be put into making the boilerplate simpler if Android finds this
> to be a problem.)

The build is complicated in that only people on that particular port usually fully understand what's going on.  That means, with the model you're supporting, each port needs to fix their own build whenever a new conditional IDL file is added.  Or the developer, if they're nice, need to learn all the build systems enough to do the fix themselves.  This can often lead to them doing things subtly incorrectly though, and causing even more headache.

On the other hand, any WebKit committer should understand the basics of #if ENABLEs.
 
> > And really. In the top of a hand full of files like that, do you really find a
> > few #if ENABLE so distracting?  I'm not sure I believe that.
> 
> Readability is the result of one thing but a lot of little things. Sure this is
> a little thing we could compromise on something that makes the file less
> readable. Do this enough and your file becomes a lot less readable.

This is a pretty unique corner case I think.

> Lastly, you're ignoring the mental effort from keeping exception in mind for
> developers. "Don't do if ENABLE's for headers unless the if ENABLE is for a
> file that is autogenerated. Is this header file autogenerated?" Instead of
> simply "Don't do if ENABLE's for headers."

Ports already end up needing to fix things themselves when their build fixes.  Why not let them fix up the source (which fixes other ports that might have the same problem...including the original port if they ever opt to turn the feature off) rather than just putting a fix into their own build system.

[David Levin]

(In reply to comment #13)
> Ports already end up needing to fix things themselves when their build fixes. 
> Why not let them fix up the source (which fixes other ports that might have the
> same problem...including the original port if they ever opt to turn the feature
> off) rather than just putting a fix into their own build system.

Well, the "original" port doesn't need to be fixed because it already has the file being generated.

Anyway, I don't think any thing new is being said from either of us, and nothing is changing either of our minds so far, so off to do useful work :)

[Jeremy Orlow]

(In reply to comment #14)
> (In reply to comment #13)
> > Ports already end up needing to fix things themselves when their build fixes. 
> > Why not let them fix up the source (which fixes other ports that might have the
> > same problem...including the original port if they ever opt to turn the feature
> > off) rather than just putting a fix into their own build system.
> 
> Well, the "original" port doesn't need to be fixed because it already has the
> file being generated.
> 
> Anyway, I don't think any thing new is being said from either of us, and
> nothing is changing either of our minds so far, so off to do useful work :)

K.  I'll go along with it, but I just wanted to officially say I think it's silly.