[Webkit-unassigned] [Bug 34812] [Android] SVG code in V8 bindings is not properly guarded

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Jeremy Orlow <jorlow at chromium.org>  2010-02-11 08:22:58 PST ---
(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.

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