[webkit-reviews] review denied: [Bug 25616] Add more ENABLE_DATABASE guards. : [Attachment 30100] Adds guards

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 7 09:47:59 PDT 2009


Darin Adler <darin at apple.com> has denied Ben Murdoch <benm at google.com>'s
request for review:
Bug 25616: Add more ENABLE_DATABASE guards.
https://bugs.webkit.org/show_bug.cgi?id=25616

Attachment 30100: Adds guards
https://bugs.webkit.org/attachment.cgi?id=30100&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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


More information about the webkit-reviews mailing list