[webkit-reviews] review requested: [Bug 16669] autotools update and fixes : [Attachment 18172] Fix database inclusion and few fixes to the patch

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has asked  for review:
Bug 16669: autotools update and fixes
http://bugs.webkit.org/show_bug.cgi?id=16669

Attachment 18172: Fix database inclusion and few fixes to the patch
http://bugs.webkit.org/attachment.cgi?id=18172&action=edit

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>
Hi Darin

> +#if ENABLE(DATABASE)
> +#include "Database.h"
> +#endif
>
> This change is wrong. The file "Database.h" already has an #if 
> ENABLE(DATABASE) around its contents. We don't want to ifdef it in both 
> places.

Removed the ifdef in Database.h. See explanation below.

> +#if ENABLE(DATABASE)
> +#include "DatabaseTracker.h"
> +#endif
>
> We need to be consistent. The ENABLE #if statements are inside Database.h and

> should go inside DatabaseTracker.h too. Or we could go the other way and 
> change how we handle optional things like database.

I went the other way around to make Database consistent with the other
features, since Database ifdefs are already there, it's just a matter of adding
a guard before including Database.h, which pretty much what the other features
are doing (e.g. include xpath headers only if xpath is enabled). 

> ENABLE_XBL should not be in the makefile. XBL was an experiment done more
than
> two years ago. It doesn't make sense to have it commented-out code in there
> with a FIXME.

Removed XBL from the makefile/configure scripts.

Also, ideally we should be using DerivedSources.make but i had some issues with
the existing makefile (e.g., vpath mangling, some make constructs do not work
well with automake) as well as the risk of making untested changes that affects
the other platforms (i dont have a mac for example). But anyway, patches are
always welcome if somebody wants to work on it.

Thanks for the review!