Bug 24776 – Disabling HTML5 Database breaks the build

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Murdoch]

Disabling ENABLE_DATABASE causes the build to fail when compiling Database.cpp as this calls WebCore::Document::setHasOpenDatabases and others which have been compiled out by an #ifdef ENABLE_DATABASE.

I have a patch ready to correct the ifdef's.

[Ben Murdoch]

Created attachment 28891 [details]
Proposed fix for #ifdefs in Document and InspectorController

[Sam Weinig]

Comment on attachment 28891 [details]
Proposed fix for #ifdefs in Document and InspectorController

Why not just put an #ifdef around Database.cpp so it doesn't compile?

> +    void Document::setHasOpenDatabases();
> +    bool Document::hasOpenDatabases();

Also, this will break some builds, r-.

[Ben Murdoch]

(In reply to comment #2)
> (From update of attachment 28891 [details] [review])
> Why not just put an #ifdef around Database.cpp so it doesn't compile?
> 
> > +    void Document::setHasOpenDatabases();
> > +    bool Document::hasOpenDatabases();
> 
> Also, this will break some builds, r-.
> 

Hi Sam,

Database.cpp is needed in the build for DatabaseAuthorizer.cpp, which is used  in the SQLite code for other features like the Icon Database so I don't think just #ifdef'ing out the file will work. I guess the other alternative is to #ifdef out the parts of Database.cpp that call into the Document but changing the ifdef's in Document.cpp|h and the InspectorController seemed like the clearer and simpler solution to me.

I can fix the syntax in Document.h and upload a new patch if you think this is worth pursuing. Please let me know your thoughts.

Thanks, Ben

[Ben Murdoch]

Created attachment 28894 [details]
Proposed fix for #ifdefs in Document and InspectorController

[Darin Adler]

Comment on attachment 28894 [details]
Proposed fix for #ifdefs in Document and InspectorController

It’s not a good idea to change inlining just to fix compiles like this. The setHasOpenDatabases and hasOpendatabases functions can remain inlined in the header, but just be after the class definition and separate.

We compile with unused argument settings, so you’ll need to use the UNUSED_PARAM macro in the #else cases of these various features.

This does *not* seem like quite the right approach. I’m not sure how much you’ve done to try other things first. A function like stopDatabases() makes sense even if you don’t have databases enabled. But a feature like addOpenDatabase doesn’t. And databaseThread makes even less sense.

I’m not going to set review- because perhaps the others who have been working with you on earlier versions of this patch have a different view.

[Ben Murdoch]

Good point on the inlining and thanks for the UNUSED_PARAM tip, I will upload a new version of the patch that takes your comments into account.

Regarding the strategy for fixing the build break, I'm not sure I was very clear in explaining the problem I'm trying to fix. If you disable HTML5 Database support in the build by defining ENABLE_DATABASE to 0, some database related functions are completely compiled out of the Document class. These functions are called by Database methods in Database.cpp and so with ENABLE_DATABASE set to 0 the compilation of Database.cpp fails as the functions are no longer present in the Document class. I don't think it's correct to remove Database.cpp from the build or #ifdef out the file as it seems it is used by the SQLite classes which in turn are used by other parts of WebCore - the Icon Database, Application Cache, etc.

I can think of a couple of ways to fix the build break:
1) Leave Database.cpp as it is and #if ENABLE(DATABASE) the definitions rather than declarations of the database functions in the Document class. This requires the fewest code changes and in my opinion is clearest and simplest. I have sent a patch that does this. I see that a similar technique of compiling out the function body is used for the ENABLE_TEXT_CARET macro in page/Frame.cpp.
2) Leave Document.cpp|h as they are and #if ENABLE(DATABASE) each of the callsites for functions compiled out of the Document class in Database.cpp. There are a lot of these calls and this strikes me as a messier approach.

I'm certainly open to alternative solutions if anyone can think of a better way to fix this.

Thanks, Ben

[Ben Murdoch]

Created attachment 28942 [details]
Proposed fix for #ifdefs in Document and InspectorController

[Sam Weinig]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 28891 [details] [review] [review])
> > Why not just put an #ifdef around Database.cpp so it doesn't compile?
> > 
> > > +    void Document::setHasOpenDatabases();
> > > +    bool Document::hasOpenDatabases();
> > 
> > Also, this will break some builds, r-.
> > 
> 
> Hi Sam,
> 
> Database.cpp is needed in the build for DatabaseAuthorizer.cpp, which is used 
> in the SQLite code for other features like the Icon Database so I don't think
> just #ifdef'ing out the file will work. 

This is the issue then.  We need to detangle DatabaseAutorizer from SQLite code, indeed, that is a layering violation.

[Ben Murdoch]

I had a look in the platform/sql code and particularly the SQLiteAuthorizer - it seems that the SQLiteAuthorizer was replaced by DatabaseAuthorizer in changeset 36054. On inspecting further however it looks like the only dependency in DatabaseAuthorizer.cpp on the rest of the Database code is Database::databaseInfoTableName. What do you think about moving this dependency into Database.h (it's straightforward and static) and #if ENABLE(DATABASE) out the other database cpp files? Or we could leave this function in Database.cpp and #if ENABLE(DATABASE) around the rest of the code? I think that either of these means we can leave Document.cpp|h as they are. I'll give these alternatives a try and upload an alternative patch. Let me know your thoughts.

[Sam Weinig]

Perhaps we should instead consider reverting r36054.

[Ben Murdoch]

I've been working on a different patch that compiles out the unneeded database code except for the function that the Authoriser needs. I've got WebCore built but I've come accross a problem with creating the WebKit library. As I've compiled out classes like DatabaseTracker from WebCore, WebKitDatabaseManager no longer compiles. I think it makes sense to also remove WebKitDatabaseManager as we are building with database support off. However on Windows (I'm not sure about the other platforms but I guess there might be similar problems) when I remove that class, I get problems with WebKitClassFactory as it expects WebKitDatabaseManager to exist in the FOR_EACH_COCLASS macro (see webkit\win\ForEachCoClass.h:35)

Can anyone give me some advice on how best to work around this issue? I'm not able to put an #if inside the FOR_EACH_COCLASS macro (compiler errors) and creating another FOR_EACH_COCLASS macro for when databases are disabled seems like a maintenance nightmare in the making. Another solution would be to compile out the bodies of the functions in WebKitDatabaseManager and have them return E_NOINTERFACE or similar, but then the class still exists when it seems like it shouldn't and I'm not clear what similar code changes will be needed on the other platforms. Any advice would be appreciated!

Many thanks, Ben 

(In reply to comment #9)
> I had a look in the platform/sql code and particularly the SQLiteAuthorizer -
> it seems that the SQLiteAuthorizer was replaced by DatabaseAuthorizer in
> changeset 36054. On inspecting further however it looks like the only
> dependency in DatabaseAuthorizer.cpp on the rest of the Database code is
> Database::databaseInfoTableName. What do you think about moving this dependency
> into Database.h (it's straightforward and static) and #if ENABLE(DATABASE) out
> the other database cpp files? Or we could leave this function in Database.cpp
> and #if ENABLE(DATABASE) around the rest of the code? I think that either of
> these means we can leave Document.cpp|h as they are. I'll give these
> alternatives a try and upload an alternative patch. Let me know your thoughts.
> 

[Ben Murdoch]

I've worked out an alternative patch that compiles out HTML5 Database code more sensibly should the ENABLE_DATABASE macro be set to 0. This touches more files than my previous patch, but removes (as far as I can tell) all the Database code that the rest of WebCore doesn't need. I think this is a better approach, so I've made my previous patch obsolete.

Please let me know if you have any feedback!

Thanks, Ben

[Ben Murdoch]

Created attachment 29126 [details]
Fix build break with HTML5 Database disbled

[Eric Seidel (no email)]

Comment on attachment 29126 [details]
Fix build break with HTML5 Database disbled

In general this looks fine except for the missing newlines at end of files.

r- for the missing newlines (I think the commit will fail w/o them).

[Ben Murdoch]

(In reply to comment #14)
> (From update of attachment 29126 [details] [review])
> In general this looks fine except for the missing newlines at end of files.
> 
> r- for the missing newlines (I think the commit will fail w/o them).
> 

Thanks Eric - will fix up the newlines and upload a new patch.

Cheers, Ben

[Ben Murdoch]

Created attachment 29939 [details]
Fixes newlines and adds other platforms

Fixes newline problem with the previous patch version and also adds the necessary edits to platforms other than windows (mac, qt, gtk, wx).

Please note: I do not have a GTK, Qt or WX setup to test this patch with. Although the edits on those platforms are straightforward and should be fine, someone with the a build setup for those platforms may want to test the patch before it lands to avoid any surprises :)

[Eric Seidel (no email)]

Comment on attachment 29939 [details]
Fixes newlines and adds other platforms

The FOR_EACH_COCLASS change strikes me as a little strange.  But in general this looks great!

[Eric Seidel (no email)]

Landed as http://trac.webkit.org/changeset/43283.  Thanks!