Bug 37186

Summary: [Chromium] Register Chromium's SQLite VFS even when running in single-process mode
Product: WebKit Reporter: Dumitru Daniliuc <dumi>
Component: New BugsAssignee: Dumitru Daniliuc <dumi>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, ericu, fishd, jorlow, michaeln
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
dglazkov: review+, dumi: commit-queue-
patch
dumi: commit-queue-
patch
jorlow: review-, dumi: commit-queue-
patch jorlow: review+, dumi: commit-queue-

Dumitru Daniliuc
Reported 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.
Attachments
patch (3.18 KB, patch)
2010-04-06 20:44 PDT, Dumitru Daniliuc
dglazkov: review+
dumi: commit-queue-
patch (8.40 KB, patch)
2010-04-07 15:21 PDT, Dumitru Daniliuc
dumi: commit-queue-
patch (7.69 KB, patch)
2010-04-07 15:47 PDT, Dumitru Daniliuc
jorlow: review-
dumi: commit-queue-
patch (9.92 KB, patch)
2010-04-07 17:14 PDT, Dumitru Daniliuc
jorlow: review+
dumi: commit-queue-
Dumitru Daniliuc
Comment 1 2010-04-06 20:44:45 PDT
Dimitri Glazkov (Google)
Comment 2 2010-04-06 20:51:31 PDT
Comment on attachment 52698 [details] patch ok.
Michael Nordman
Comment 3 2010-04-07 11:58:55 PDT
like we talked about, this looks like it breaks DOMStorage in chrome?
Dumitru Daniliuc
Comment 4 2010-04-07 15:21:26 PDT
Created attachment 52788 [details] patch Make WebSQLDatabases happy while not breaking other features.
Dumitru Daniliuc
Comment 5 2010-04-07 15:47:51 PDT
Created attachment 52795 [details] patch Same patch, minus DatabaseTracker changes.
Jeremy Orlow
Comment 6 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.
Dumitru Daniliuc
Comment 7 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.
Jeremy Orlow
Comment 8 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.)
Dumitru Daniliuc
Comment 9 2010-04-07 17:14:56 PDT
Created attachment 52809 [details] patch Done. Please take another look.
Jeremy Orlow
Comment 10 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.
Dumitru Daniliuc
Comment 11 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.
Michael Nordman
Comment 12 2010-04-07 18:03:13 PDT
LGTM
Dumitru Daniliuc
Comment 13 2010-04-07 18:19:08 PDT
Landed as r57245.
Note You need to log in before you can comment on or make changes to this bug.