Bug 146550 – build-webkit --no-indexed-database fails due to missing guards

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Emanuele Aina]

At the moment IndexedDatabase is the only user of the DatabaseProcess, which means that the usefulness of enabling the latter without the former is very low. They are however protected by different guards, ENABLE(INDEXED_DATABASE) and ENABLE(DATABASE_PROCESS) respectively, while it is technically possible to end up in that situation it's likely that everyone either disable them both or enable them. In such a situation, inevitably the two guards grew intermingled, with different levels on consistency.

Unfortunately, turning off ENABLE(INDEXED_DATABASE) while keeping ENABLE(DATABASE_PROCESS) is exactly what WebKit ends up doing when specifying the --no-indexed-database flag on `build-webkit` invocation, leading to build failures like the one below:

$ Tools/Scripts/build-webkit --gtk --release --no-indexed-database

[...]

In file included from ../../Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:27:0:
../../Source/WebKit2/DatabaseProcess/DatabaseProcess.h:56:70: error: ‘UniqueIDBDatabaseIdentifier’ does not name a type
     PassRefPtr<UniqueIDBDatabase> getOrCreateUniqueIDBDatabase(const UniqueIDBDatabaseIdentifier&);
                                                                      ^
In file included from ../../Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:27:0:
../../Source/WebKit2/DatabaseProcess/DatabaseProcess.h:90:98: error: ‘SecurityOriginData’ was not declared in this scope
     void deleteWebsiteDataForOrigins(WebCore::SessionID, uint64_t websiteDataTypes, const Vector<SecurityOriginData>& origins, uint64_t callbackID);

[...]

[Emanuele Aina]

Created attachment 256014 [details]
Patch

[Csaba Osztrogonác]

Is it useful for anything to enable DatabaseProcess without IndexedDB?
If no, it would be better to unify these guards.

[Emanuele Aina]

> Is it useful for anything to enable DatabaseProcess without IndexedDB?
> If no, it would be better to unify these guards.

As far as I can tell, at the moment IndexedDB seems to be the only user of DatabaseProcess, so merging those guards would make sense as the split will likely bitrot again.

On the other hand the DatabaseProcess code seems to be structured to handle multiple users, so in future we could need to split them again, redoing some work which has already been done.

I just needed to make the whole thing build (at the moment I'm stuck to a GCC 4.8 toolchain and DatabaseProcess is the only thing that requires GCC 4.9) so both solutions work for me. :)

[Emanuele Aina]

Created attachment 257252 [details]
Patch

Rebased on current trunk (r187148)

[Csaba Osztrogonác]

Anders or Sam, what do you think about this patch? 

Do we need DatabaseProcess if IndexedDB feature is disabled?
- If yes, we need a patch like this to fix this build configuration.
- If no, we should unify the two guard or disable DatabaseProcess
automatically if IndexedDB is disabled.

[Csaba Osztrogonác]

(In reply to comment #5)
> Anders or Sam, what do you think about this patch? 
> 
> Do we need DatabaseProcess if IndexedDB feature is disabled?
> - If yes, we need a patch like this to fix this build configuration.
> - If no, we should unify the two guard or disable DatabaseProcess
> automatically if IndexedDB is disabled.

just a note: We need one of these changes to fix the --minimal build
configuration on EFL to be able to catch broken ifdef guards again.

[Csaba Osztrogonác]

(In reply to comment #5)
> Anders or Sam, what do you think about this patch? 
> 
> Do we need DatabaseProcess if IndexedDB feature is disabled?
> - If yes, we need a patch like this to fix this build configuration.
> - If no, we should unify the two guard or disable DatabaseProcess
> automatically if IndexedDB is disabled.

ping?

[Sam Weinig]

(In reply to comment #7)
> (In reply to comment #5)
> > Anders or Sam, what do you think about this patch? 
> > 
> > Do we need DatabaseProcess if IndexedDB feature is disabled?
> > - If yes, we need a patch like this to fix this build configuration.
> > - If no, we should unify the two guard or disable DatabaseProcess
> > automatically if IndexedDB is disabled.
> 
> ping?

The DatabaseProcess is only needed for IndexedDB right now. That may not always be the case however, so I prefer having two guards.

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Anders or Sam, what do you think about this patch? 
> > > 
> > > Do we need DatabaseProcess if IndexedDB feature is disabled?
> > > - If yes, we need a patch like this to fix this build configuration.
> > > - If no, we should unify the two guard or disable DatabaseProcess
> > > automatically if IndexedDB is disabled.
> > 
> > ping?
> 
> The DatabaseProcess is only needed for IndexedDB right now. That may not
> always be the case however, so I prefer having two guards.

The proposed patch leaves the two guards untouched 
and adds the missing guards to be able to build with
disabled indexedDB support and enabled DatabaseProcess.

So could you possibly review this patch?

[Csaba Osztrogonác]

ping?

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Anders or Sam, what do you think about this patch? 
> > > 
> > > Do we need DatabaseProcess if IndexedDB feature is disabled?
> > > - If yes, we need a patch like this to fix this build configuration.
> > > - If no, we should unify the two guard or disable DatabaseProcess
> > > automatically if IndexedDB is disabled.
> > 
> > ping?
> 
> The DatabaseProcess is only needed for IndexedDB right now. That may not
> always be the case however, so I prefer having two guards.

Could you review this patch?

[Michael Catanzaro]

I don't think we need owners to approve build fixes?

A good argument could be made that ENABLE_DATABASE_PROCESS should subsume the ENABLE_INDEXED_DATABASE option, but for the time being that's not the case, and it can always be changed later....

[WebKit Commit Bot]

Comment on attachment 257252 [details]
Patch

Rejecting attachment 257252 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 257252, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
Authentication realm: <http://svn.webkit.org:80> Mac OS Forge
Password for '[e-mail]': 
Authentication realm: <http://svn.webkit.org:80> Mac OS Forge
Username: error: git-svn died of signal 11

Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/132788

[Csaba Osztrogonác]

Comment on attachment 257252 [details]
Patch

Clearing flags on attachment: 257252

Committed r189244: <http://trac.webkit.org/changeset/189244>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.