Modern IDB: Support opening and deleting SQLite databases on disk
Created attachment 268923 [details] Patch v1 Touches filesystem - Let's get 3 reviewers up in here.
Attachment 268923 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Storage/WebDatabaseProvider.mm:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 268923 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=268923&action=review > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:74 > + bool ensureValidRecordsTable(); > + std::unique_ptr<IDBDatabaseInfo> extractExistingDatabaseInfo(); > + std::unique_ptr<IDBDatabaseInfo> createAndPopulateInitialDatabaseInfo(); These are unimplemented in this patch, but it doesn't hurt to add them to a header. > Source/WebKit/mac/Storage/WebDatabaseProvider.mm:36 > + databasesDirectory = @"~/Library/WebKit/Databases/___IndexedDB"; This is equivalent to the old way we get a directory for the databases, but doesn't this allow an application to have access to the IDB databases of another application that uses WebKit?
(In reply to comment #3) > Comment on attachment 268923 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268923&action=review > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:74 > > + bool ensureValidRecordsTable(); > > + std::unique_ptr<IDBDatabaseInfo> extractExistingDatabaseInfo(); > > + std::unique_ptr<IDBDatabaseInfo> createAndPopulateInitialDatabaseInfo(); > > These are unimplemented in this patch, but it doesn't hurt to add them to a > header. Yah, whoops - I'll remove them. This patch was originally much bigger but I decided the review on "create" and "delete" by themselves would be much nicer. > > Source/WebKit/mac/Storage/WebDatabaseProvider.mm:36 > > + databasesDirectory = @"~/Library/WebKit/Databases/___IndexedDB"; > > This is equivalent to the old way we get a directory for the databases, but > doesn't this allow an application to have access to the IDB databases of > another application that uses WebKit? Yes. Which - for WebSQL databases - was kind of okay, but is definitely not for IDB. Good catch. I'll throw in the app's bundle id.
Created attachment 268934 [details] Patch v2
Attachment 268934 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Storage/WebDatabaseProvider.mm:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 268934 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=268934&action=review > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:65 > + if (!m_sqliteDB->open(dbFilename)) { This calls SQLiteFileSystem::openDatabase which calls sqlite3_open which creates the file if it doesn't exist. I think there should be a corresponding if (m_sqliteDB) { m_sqliteDB->close(); m_sqliteDB = nullptr; } in ~SQLiteIDBBackingStore that closes it without deleting it. > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:75 > + if (!m_sqliteDB) > + return *m_databaseInfo; > + > return *m_databaseInfo; This is strange. It doesn't matter if m_sqliteDB is null. And right now m_sqliteDB is never null here. Will this be a meaningful check once the fixme is fixed? > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:180 > + if (m_sqliteDB) > + m_sqliteDB->close(); Shouldn't m_sqliteDB be set to nullptr after this? > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:183 > + deleteFile(dbFilename); > + deleteEmptyDirectory(m_absoluteDatabaseDirectory); Since this corresponds the SQLiteFileSystem::openDatabase, I think these should be SQLiteFileSystem::deleteDatabaseFile and SQLiteFileSystem::deleteEmptyDatabaseDirectory to match stylistically, even though those functions do nothing but call these functions.
Comment on attachment 268934 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=268934&action=review > Source/WebKit/mac/Storage/WebDatabaseProvider.mm:38 > + databasesDirectory = WebCore::pathByAppendingComponent(databasesDirectory, ASCIILiteral("___IndexedDB")); No bundle identifier if WebDatabaseDirectoryDefaultsKey is in the standardUserDefaults?
(In reply to comment #8) > Comment on attachment 268934 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268934&action=review > > > Source/WebKit/mac/Storage/WebDatabaseProvider.mm:38 > > + databasesDirectory = WebCore::pathByAppendingComponent(databasesDirectory, ASCIILiteral("___IndexedDB")); > > No bundle identifier if WebDatabaseDirectoryDefaultsKey is in the > standardUserDefaults? Correct. The bundle id is only used to segregate folks using the default. If authors are savvy enough to specify the path, they're also savvy enough to specify their bundle id, leading to the problem we're trying to avoid. If they're that savvy, we can't stop them from getting into this type of trouble.
(In reply to comment #7) > Comment on attachment 268934 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268934&action=review > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:65 > > + if (!m_sqliteDB->open(dbFilename)) { > > This calls SQLiteFileSystem::openDatabase which calls sqlite3_open which > creates the file if it doesn't exist. I think there should be a > corresponding if (m_sqliteDB) { m_sqliteDB->close(); m_sqliteDB = nullptr; } > in ~SQLiteIDBBackingStore that closes it without deleting it. > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:75 > > + if (!m_sqliteDB) > > + return *m_databaseInfo; > > + > > return *m_databaseInfo; > > This is strange. It doesn't matter if m_sqliteDB is null. And right now > m_sqliteDB is never null here. Will this be a meaningful check once the > fixme is fixed? > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:180 > > + if (m_sqliteDB) > > + m_sqliteDB->close(); > > Shouldn't m_sqliteDB be set to nullptr after this? > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:183 > > + deleteFile(dbFilename); > > + deleteEmptyDirectory(m_absoluteDatabaseDirectory); > > Since this corresponds the SQLiteFileSystem::openDatabase, I think these > should be SQLiteFileSystem::deleteDatabaseFile and > SQLiteFileSystem::deleteEmptyDatabaseDirectory to match stylistically, even > though those functions do nothing but call these functions. Will make these changes in v3 of the patch when I get a chance to work on v3.
Created attachment 268975 [details] Patch v3
Attachment 268975 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Storage/WebDatabaseProvider.mm:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 268975 [details] Patch v3 r=me Get two more
Comment on attachment 268975 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=268975&action=review > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:128 > +/* > + if (m_databaseDirectoryPath.isEmpty()) > + return MemoryIDBBackingStore::create(identifier); > + > + return std::make_unique<SQLiteIDBBackingStore>(identifier, m_databaseDirectoryPath); > +*/ Please don't check-in commented out code.
Comment on attachment 268975 [details] Patch v3 r=me
Created attachment 269030 [details] Patch for landing
Attachment 269030 [details] did not pass style-queue: ERROR: Source/WebKit/mac/Storage/WebDatabaseProvider.mm:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 269030 [details] Patch for landing Clearing flags on attachment: 269030 Committed r195090: <http://trac.webkit.org/changeset/195090>