RESOLVED FIXED 128060
IDB: Support IDBFactory.deleteDatabase()
https://bugs.webkit.org/show_bug.cgi?id=128060
Summary IDB: Support IDBFactory.deleteDatabase()
Jon Lee
Reported 2014-02-01 19:57:43 PST
Required for running layout tests. <rdar://problem/15965021>
Attachments
Patch v1 (29.25 KB, patch)
2014-02-02 01:09 PST, Brady Eidson
fpizlo: review+
Brady Eidson
Comment 1 2014-02-02 01:09:20 PST
Created attachment 222918 [details] Patch v1 Touches the filesystem (delete), so requires extra scrutiny.
WebKit Commit Bot
Comment 2 2014-02-02 01:11:04 PST
Attachment 222918 [details] did not pass style-queue: ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBFactoryBackend.h:44: Missing space after , [whitespace/comma] [3] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Filip Pizlo
Comment 3 2014-02-02 10:21:39 PST
Comment on attachment 222918 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=222918&action=review r=me > Source/WebCore/Modules/indexeddb/IDBFactory.cpp:164 > - m_backend->deleteDatabase(name, request, context->securityOrigin(), context, getIndexedDBDatabasePath(context)); > + m_backend->deleteDatabase(name, *(context->securityOrigin()), *(context->topOrigin()), request, context, getIndexedDBDatabasePath(context)); It feels like instead of *(context->securityOrigin()), you could just say *context->securityOrigin(). The extra parens make me initially expect something sneaky to be going on even though it's pretty straight-forward.
Simon Fraser (smfr)
Comment 4 2014-02-02 10:40:00 PST
Comment on attachment 222918 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=222918&action=review Count me as a pair of eyes. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackend.cpp:558 > -void IDBDatabaseBackend::deleteDatabaseAsync(PassRefPtr<IDBCallbacks> callbacks) > +void IDBDatabaseBackend::deleteDatabaseAsync(PassRefPtr<IDBCallbacks> prpCallbacks) Hungarian notation!? > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:166 > + LOG(IDB, "UniqueIDBDatabase::shutdownBackingStore would deleting file '%s' on disk", dbFilename.utf8().data()); would deleting file?
Timothy Hatcher
Comment 5 2014-02-02 10:49:51 PST
Comment on attachment 222918 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=222918&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackend.cpp:558 >> +void IDBDatabaseBackend::deleteDatabaseAsync(PassRefPtr<IDBCallbacks> prpCallbacks) > > Hungarian notation!? It is the sanctioned prefix for PassRefPtr when you want to put it into a RefPtr in the function. http://www.webkit.org/coding/RefPtr.html
Brady Eidson
Comment 6 2014-02-02 11:21:23 PST
(In reply to comment #4) > (From update of attachment 222918 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222918&action=review > > Count me as a pair of eyes. > > > Source/WebCore/Modules/indexeddb/IDBDatabaseBackend.cpp:558 > > -void IDBDatabaseBackend::deleteDatabaseAsync(PassRefPtr<IDBCallbacks> callbacks) > > +void IDBDatabaseBackend::deleteDatabaseAsync(PassRefPtr<IDBCallbacks> prpCallbacks) > > Hungarian notation!? Tim already pointed out this is the PassRefPtr standard. > > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:166 > > + LOG(IDB, "UniqueIDBDatabase::shutdownBackingStore would deleting file '%s' on disk", dbFilename.utf8().data()); > > would deleting file? Yikes. Thanks! (In reply to comment #3) > > Source/WebCore/Modules/indexeddb/IDBFactory.cpp:164 > > - m_backend->deleteDatabase(name, request, context->securityOrigin(), context, getIndexedDBDatabasePath(context)); > > + m_backend->deleteDatabase(name, *(context->securityOrigin()), *(context->topOrigin()), request, context, getIndexedDBDatabasePath(context)); > > It feels like instead of *(context->securityOrigin()), you could just say *context->securityOrigin(). The extra parens make me initially expect something sneaky to be going on even though it's pretty straight-forward. You're right. Thanks!
Maciej Stachowiak
Comment 7 2014-02-02 11:31:01 PST
(In reply to comment #5) > (From update of attachment 222918 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222918&action=review > > >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackend.cpp:558 > >> +void IDBDatabaseBackend::deleteDatabaseAsync(PassRefPtr<IDBCallbacks> prpCallbacks) > > > > Hungarian notation!? > > It is the sanctioned prefix for PassRefPtr when you want to put it into a RefPtr in the function. http://www.webkit.org/coding/RefPtr.html What a weird choice. I get the desire to distinguish the name from the actually used one, but I'd use something like "passCallbacks" or "callbacksParam" or "callbacksArg" to make this clear. I will have to ask Darin why he used "prp" in the RefPtr guide.
Maciej Stachowiak
Comment 8 2014-02-02 11:42:23 PST
Comment on attachment 222918 [details] Patch v1 I don't see any issues besides those spotted by others. r=me
Brady Eidson
Comment 9 2014-02-02 11:51:32 PST
Note You need to log in before you can comment on or make changes to this bug.