Bug 25616

Summary: Add more ENABLE_DATABASE guards.
Product: WebKit Reporter: Ben Murdoch <benm>
Component: WebCore Misc.Assignee: Ben Murdoch <benm>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, laszlo.gombos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Adds guards
darin: review-
Fixes include as per comments below.
none
Adds guards to V8 bindings as well. darin: review+

Description Ben Murdoch 2009-05-07 09:05:33 PDT
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.
Comment 1 Ben Murdoch 2009-05-07 09:06:47 PDT
Created attachment 30100 [details]
Adds guards
Comment 2 Darin Adler 2009-05-07 09:47:59 PDT
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
Comment 3 Ben Murdoch 2009-05-07 10:09:19 PDT
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
Comment 4 Darin Adler 2009-05-07 11:16:21 PDT
(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.
Comment 5 Ben Murdoch 2009-05-08 02:33:26 PDT
Created attachment 30128 [details]
Fixes include as per comments below.
Comment 6 Ben Murdoch 2009-05-13 02:45:26 PDT
Ping ... :)
Comment 7 Ben Murdoch 2009-05-13 04:57:07 PDT
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.
Comment 8 Darin Adler 2009-05-13 06:26:27 PDT
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
Comment 9 Gustavo Noronha (kov) 2009-05-14 07:49:42 PDT
Landed as r43699.