Bug 25616 – Add more ENABLE_DATABASE guards.

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

[Ben Murdoch]

Further to the patch for https://bugs.webkit.org/show_bug.cgi?id=24776 (landed as r43283) I have found further places in the source where the ENABLE_DATABASE macro guard is appropriate. Inserting these guards helps reduce the overall library size when the database api is disabled. Please see attached patch.

[Ben Murdoch]

Created attachment 30100 [details]
Adds guards

[Darin Adler]

Comment on attachment 30100 [details]
Adds guards

Seems like a fine thing to do.

> Index: WebCore/storage/Database.cpp
> ===================================================================
> --- WebCore/storage/Database.cpp	(revision 43341)
> +++ WebCore/storage/Database.cpp	(working copy)
> @@ -49,9 +49,10 @@
>  #include "SQLiteStatement.h"
>  #include "SQLResultSet.h"
>  #include <wtf/MainThread.h>
> -#include <wtf/StdLibExtras.h>
>  #endif
>  
> +#include <wtf/StdLibExtras.h>
> +
>  #if USE(JSC)
>  #include "JSDOMWindow.h"
>  #include <runtime/InitializeThreading.h>

Unconditional includes should go *before* any conditional includes in the first paragraph. Not between two paragraphs of conditional includes.

> Index: WebCore/storage/DatabaseDetails.h
> ===================================================================
> --- WebCore/storage/DatabaseDetails.h	(revision 43341)
> +++ WebCore/storage/DatabaseDetails.h	(working copy)
> @@ -29,6 +29,8 @@
>  #ifndef DatabaseDetails_h
>  #define DatabaseDetails_h
>  
> +#if ENABLE(DATABASE)
> +
>  #include "PlatformString.h"
>  
>  namespace WebCore {

It's not correct to use the ENABLE macro without first including the Platform.h header. Doing so changes this header so it can't be included first, which we don't want. I see the same problem in a few other headers in this patch.

review- since we should get such minor details right in a patch that's entirely about adding conditionals

[Ben Murdoch]

Thanks for the comments Darin!

> Unconditional includes should go *before* any conditional includes in the first
> paragraph. Not between two paragraphs of conditional includes.

OK, will fix and send a new patch.

> It's not correct to use the ENABLE macro without first including the Platform.h
> header. Doing so changes this header so it can't be included first, which we
> don't want. I see the same problem in a few other headers in this patch.

OK, but just to be clear -- I only need to add the platform header to those headers that don't have a corresponding implementation file i.e.

DatabaseDetails.h
SQLError.h
SQLStatementCallback.h
SQLStatementErrorCallback.h
SQLTransactionCallback.h
SQLTransactionErrorCallback.h

as the others with an implementation file include config.h which pulls in platform.h? Is it preferable in those headers above to pull in platform.h or config.h?

Thanks, Ben

[Darin Adler]

(In reply to comment #3)
> OK, but just to be clear -- I only need to add the platform header to those
> headers that don't have a corresponding implementation file i.e.

No, that's not it.

I didn't realize <wtf/Platform.h> is in "config.h". Given that it is, my comment was wrong.

It's the responsibility of .cpp files to include "config.h" and WebCore header files can safely assume it has already been included.

[Ben Murdoch]

Created attachment 30128 [details]
Fixes include as per comments below.

[Ben Murdoch]

Ping ... :)

[Ben Murdoch]

Created attachment 30272 [details]
Adds guards to V8 bindings as well.

This patch also applies the changes to V8 bindings. CC'ing dglazkov for the V8 changes.

[Darin Adler]

Comment on attachment 30272 [details]
Adds guards to V8 bindings as well.

> +        Reviewed by NOBODY (OOPS!).
> +        
> +        Reviewed by NOBODY (OOPS!).

You should not have that in the patch you post.

> +        WARNING: NO TEST CASES ADDED OR CHANGED

You should also remove this.

Otherwise this looks fine, r=me

[Gustavo Noronha (kov)]

Landed as r43699.