Bug 146550 - build-webkit --no-indexed-database fails due to missing guards
Summary: build-webkit --no-indexed-database fails due to missing guards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-02 09:58 PDT by Emanuele Aina
Modified: 2015-09-02 04:59 PDT (History)
20 users (show)

See Also:


Attachments
Patch (18.82 KB, patch)
2015-07-02 10:12 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (18.81 KB, patch)
2015-07-22 00:44 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emanuele Aina 2015-07-02 09:58:50 PDT
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);

[...]
Comment 1 Emanuele Aina 2015-07-02 10:12:44 PDT
Created attachment 256014 [details]
Patch
Comment 2 Csaba Osztrogonác 2015-07-03 08:12:21 PDT
Is it useful for anything to enable DatabaseProcess without IndexedDB?
If no, it would be better to unify these guards.
Comment 3 Emanuele Aina 2015-07-03 08:20:23 PDT
> 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. :)
Comment 4 Emanuele Aina 2015-07-22 00:44:04 PDT
Created attachment 257252 [details]
Patch

Rebased on current trunk (r187148)
Comment 5 Csaba Osztrogonác 2015-08-03 05:57:08 PDT
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.
Comment 6 Csaba Osztrogonác 2015-08-03 06:16:01 PDT
(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.
Comment 7 Csaba Osztrogonác 2015-08-06 04:45:44 PDT
(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?
Comment 8 Sam Weinig 2015-08-06 09:33:39 PDT
(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.
Comment 9 Csaba Osztrogonác 2015-08-24 09:02:16 PDT
(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?
Comment 10 Csaba Osztrogonác 2015-08-25 02:43:57 PDT
ping?
Comment 11 Csaba Osztrogonác 2015-09-01 02:41:57 PDT
(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?
Comment 12 Michael Catanzaro 2015-09-01 08:47:24 PDT
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....
Comment 13 WebKit Commit Bot 2015-09-02 04:45:30 PDT
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 'commit-queue@webkit.org': 
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
Comment 14 Csaba Osztrogonác 2015-09-02 04:59:02 PDT
Comment on attachment 257252 [details]
Patch

Clearing flags on attachment: 257252

Committed r189244: <http://trac.webkit.org/changeset/189244>
Comment 15 Csaba Osztrogonác 2015-09-02 04:59:12 PDT
All reviewed patches have been landed.  Closing bug.