Bug 37186 - [Chromium] Register Chromium's SQLite VFS even when running in single-process mode
Summary: [Chromium] Register Chromium's SQLite VFS even when running in single-process...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-06 20:38 PDT by Dumitru Daniliuc
Modified: 2010-04-07 18:19 PDT (History)
5 users (show)

See Also:


Attachments
patch (3.18 KB, patch)
2010-04-06 20:44 PDT, Dumitru Daniliuc
dglazkov: review+
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (8.40 KB, patch)
2010-04-07 15:21 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (7.69 KB, patch)
2010-04-07 15:47 PDT, Dumitru Daniliuc
jorlow: review-
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (9.92 KB, patch)
2010-04-07 17:14 PDT, Dumitru Daniliuc
jorlow: review+
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2010-04-06 20:38:21 PDT
Chromium's SQLite VFS has not been the default VFS for a while now. We should register it even when !ChromiumBridge::sandboxEnabled() (which is equivalent to running in single-process mode). This will make DBs work correctly with --single-process too.
Comment 1 Dumitru Daniliuc 2010-04-06 20:44:45 PDT
Created attachment 52698 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2010-04-06 20:51:31 PDT
Comment on attachment 52698 [details]
patch

ok.
Comment 3 Michael Nordman 2010-04-07 11:58:55 PDT
like we talked about, this looks like it breaks DOMStorage in chrome?
Comment 4 Dumitru Daniliuc 2010-04-07 15:21:26 PDT
Created attachment 52788 [details]
patch

Make WebSQLDatabases happy while not breaking other features.
Comment 5 Dumitru Daniliuc 2010-04-07 15:47:51 PDT
Created attachment 52795 [details]
patch

Same patch, minus DatabaseTracker changes.
Comment 6 Jeremy Orlow 2010-04-07 16:40:58 PDT
I remember we considered this originally and decided against it, but I can't remember why.  + darin in case he remembers.  If not, I don't see any problem with this other than it seeming a bit ugly.
Comment 7 Dumitru Daniliuc 2010-04-07 16:44:57 PDT
(In reply to comment #6)
> I remember we considered this originally and decided against it, but I can't
> remember why.  + darin in case he remembers.  If not, I don't see any problem
> with this other than it seeming a bit ugly.

I think when we previously talked about this, the Chromium VFS was registered as the default VFS. It _is_ a big ugly, but I don't see any other way of doing this.
Comment 8 Jeremy Orlow 2010-04-07 16:50:05 PDT
Comment on attachment 52795 [details]
patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 57235)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,29 @@
> +2010-04-07  Dumitru Daniliuc  <dumi@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Always register and use Chromium's SQLite VFS for
> +        WebSQLDatabases. Keep using the default VFS in all other
> +        cases. This change should allow Chromium to support
> +        WebSQLDatabases in --single-process mode.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=37186.

Please follow the standard format for change logs.

Title
url

description

Also, please describe the "full mutex" part of the change.


> Index: WebCore/platform/sql/SQLiteFileSystem.h
> ===================================================================
> --- WebCore/platform/sql/SQLiteFileSystem.h	(revision 57230)
> +++ WebCore/platform/sql/SQLiteFileSystem.h	(working copy)
> @@ -52,7 +52,8 @@ public:
>      // fileName - The name of the database file.
>      // database - The SQLite structure that represents the database stored
>      //            in the given file.
> -    static int openDatabase(const String& fileName, sqlite3** database);
> +    // forWebSQLDatabase - True, if and only if we're opening a Web SQL Database file.

Probably you should explain why this is necessary.  (Chromium needs this info.)
Comment 9 Dumitru Daniliuc 2010-04-07 17:14:56 PDT
Created attachment 52809 [details]
patch

Done. Please take another look.
Comment 10 Jeremy Orlow 2010-04-07 17:49:58 PDT
Comment on attachment 52809 [details]
patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 57235)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,35 @@
> +2010-04-07  Dumitru Daniliuc  <dumi@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix Chromium's HTML5 DB support in --single-process mode.
> +

No newline here.

> +        https://bugs.webkit.org/show_bug.cgi?id=37186.
> +
> +        Always register and use Chromium's SQLite VFS for
> +        WebSQLDatabases. Keep using the default VFS in all other
> +        cases. This change should allow Chromium to support
> +        WebSQLDatabases in --single-process mode.
> +
> +        Also, cleaning up a big SQLiteFileSystemChromium and getting rid

This sentence doesn't parse in my head.....a big what?


r=me, provided Michael signs off on it as well and you double check that LocalStorage still works under --single-process.
Comment 11 Dumitru Daniliuc 2010-04-07 17:58:58 PDT
(In reply to comment #10)
> (From update of attachment 52809 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 57235)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,35 @@
> > +2010-04-07  Dumitru Daniliuc  <dumi@chromium.org>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Fix Chromium's HTML5 DB support in --single-process mode.
> > +
> 
> No newline here.

Removed.

> > +        https://bugs.webkit.org/show_bug.cgi?id=37186.
> > +
> > +        Always register and use Chromium's SQLite VFS for
> > +        WebSQLDatabases. Keep using the default VFS in all other
> > +        cases. This change should allow Chromium to support
> > +        WebSQLDatabases in --single-process mode.
> > +
> > +        Also, cleaning up a big SQLiteFileSystemChromium and getting rid
> 
> This sentence doesn't parse in my head.....a big what?

s/big/bit/.

> r=me, provided Michael signs off on it as well and you double check that
> LocalStorage still works under --single-process.

Will do.
Comment 12 Michael Nordman 2010-04-07 18:03:13 PDT
LGTM
Comment 13 Dumitru Daniliuc 2010-04-07 18:19:08 PDT
Landed as r57245.