| Summary: | IndexedDB: Core upgradeneeded logic | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Grogan <dgrogan> | ||||||||||||||||||||
| Component: | New Bugs | Assignee: | David Grogan <dgrogan> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | alecflett, jsbell, levin+threading, ojan, tony, webkit.review.bot | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Bug Depends on: | 93055 | ||||||||||||||||||||||
| Bug Blocks: | 92883 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
David Grogan
2012-07-27 15:46:09 PDT
Created attachment 155071 [details]
Patch
Created attachment 155080 [details]
Patch
Josh and Alec, could you take a look at this? This patch has code that only runs in the backend, and is broken off from bug 89505. I could break this up even further, first doing IDBFactoryBackendImpl and then IDBDatabaseBackendImpl if desired. Comment on attachment 155080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155080&action=review > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:59 > +class IDBDatabaseBackendImpl::PendingOpenWithVersionCall : public RefCounted<PendingOpenWithVersionCall> { Any reason not to just put the class definition inside the class? I didn't even know this syntax was legal ! (I guess it's kind of like a namespace...) > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:425 > + // The frontend takes care of the following clause, the backend just This sentence is slightly confusing - is there an "and then.." or something? I'm confused... or something like ", and then the frontend will only deliver to connections with closePending set"? (addition of 'and' and 'only') > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:446 > + RefPtr<IDBTransactionBackendInterface> transactionInterface = this->transaction(objectStoreNames.get(), IDBTransaction::VERSION_CHANGE, ec); don't need "this->" > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:466 > + ASSERT(!m_intVersion); Not sure what this is asserting for - is this different from NoIntVersion? my comments are pretty minor not understanding most of it - perhaps monday you can give us an overview, and help educate at least me about some of the states and issues this is tackling? Comment on attachment 155080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155080&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:59 >> +class IDBDatabaseBackendImpl::PendingOpenWithVersionCall : public RefCounted<PendingOpenWithVersionCall> { > > Any reason not to just put the class definition inside the class? I didn't even know this syntax was legal ! (I guess it's kind of like a namespace...) They would bloat the header file, cause long local recompiles, etc. Though I don't know how big of a deal that actually is. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:425 >> + // The frontend takes care of the following clause, the backend just > > This sentence is slightly confusing - is there an "and then.." or something? I'm confused... or something like ", and then the frontend will only deliver to connections with closePending set"? (addition of 'and' and 'only') Tried to reword it, let me know if it's clearer. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:446 >> + RefPtr<IDBTransactionBackendInterface> transactionInterface = this->transaction(objectStoreNames.get(), IDBTransaction::VERSION_CHANGE, ec); > > don't need "this->" Removed. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:466 >> + ASSERT(!m_intVersion); > > Not sure what this is asserting for - is this different from NoIntVersion? Ah, thanks, it should be ASSERT(m_intVersion == NoIntVersion). This means that I have no layout test testing this scenario, I will add one. Created attachment 155096 [details]
Patch
Comment on attachment 155096 [details] Patch Looks like a great chunk to break off and get landed. Nits, but also two things which look incorrect to me. View in context: https://bugs.webkit.org/attachment.cgi?id=155096&action=review > Source/WebCore/Modules/indexeddb/IDBBackingStore.h:54 > + ASSERT_NOT_REACHED(); Why this instead of pure virtual, like the others? > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:239 > + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, String::format("You can't use the setVersion function if you've already set the version through an open call. The current integer version is %"PRId64, m_intVersion))); Shouldn't this method return here if the error event is fired? > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:289 > + database->m_intVersion = version; For kicks, toss an ASSERT in here that version > oldVersion? > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:473 > + m_pendingOpenWithVersionCalls.append(PendingOpenWithVersionCall::create(callbacks, version)); This line here looks a little odd - I'm not sure why we're add a pending call in this case. If it's intentional, can you explain? > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:530 > + // FIXME: Instead of relying on transactions.size(), make connectionCount Ugh, yeah, this is tricky. Thanks for the comment. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:283 > + return true; If you touch this patch again, toss in a FIXME/bug reference here. Comment on attachment 155096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155096&action=review >> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:54 >> + ASSERT_NOT_REACHED(); > > Why this instead of pure virtual, like the others? Don't recall, changed. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:239 >> + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, String::format("You can't use the setVersion function if you've already set the version through an open call. The current integer version is %"PRId64, m_intVersion))); > > Shouldn't this method return here if the error event is fired? Egads, yes. Added a layout test that exposes this bug. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:289 >> + database->m_intVersion = version; > > For kicks, toss an ASSERT in here that version > oldVersion? SG >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:473 >> + m_pendingOpenWithVersionCalls.append(PendingOpenWithVersionCall::create(callbacks, version)); > > This line here looks a little odd - I'm not sure why we're add a pending call in this case. If it's intentional, can you explain? It was intentional, but it's not working out well enough. The idea was that by the time that pending call is serviced, the existing version would be the one it requested and a success event would be delivered. Instead, I'm going to replace it with a PendingOpenWithVersionCallSecondHalf queue that gets serviced first in processPendingCalls. >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:283 >> + return true; > > If you touch this patch again, toss in a FIXME/bug reference here. Done. Created attachment 155404 [details]
Patch
Comment on attachment 155096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155096&action=review >>> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:54 >>> + ASSERT_NOT_REACHED(); >> >> Why this instead of pure virtual, like the others? > > Don't recall, changed. To avoid having to update the FakeBackingStore in this patch. Changed back. (In reply to comment #11) > (From update of attachment 155096 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155096&action=review > > >>> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:54 > >>> + ASSERT_NOT_REACHED(); > >> > >> Why this instead of pure virtual, like the others? > > > > Don't recall, changed. > > To avoid having to update the FakeBackingStore in this patch. Changed back. Scratch that, FakeBackingStore is already in this patch. Created attachment 155659 [details]
Patch
Created attachment 155701 [details]
Patch
Tony, could you review this? It's the first of the patches mentioned in bug 92560. Ojan, could you review this? Comment on attachment 155701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155701&action=review Still LGTM > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:-290 > - ASSERT(connectionCount() <= 1); It's a shame to lose this strong assertion about state, but I can't think of an equivalent. Looking forward to simplifying when setVersion is gone. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:339 > + ASSERT(m_pendingSecondHalfOpenWithVersionCalls.size() <= 1); Given this, does it need to be a vector? (Maybe FIXME for later to clarify the state machine the spec implies.) > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:423 > + // Now that we give max priority to open calls that follow upgradeneeded Reword to remove the "Now that...", e.g. "We give max priority to open calls that follow upgradeneeded events; trigger the rest of the queues being service when those open calls are finished." Created attachment 155914 [details]
Patch
(In reply to comment #17) > (From update of attachment 155701 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155701&action=review > > Still LGTM > > > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:-290 > > - ASSERT(connectionCount() <= 1); > > It's a shame to lose this strong assertion about state, but I can't think of an equivalent. Looking forward to simplifying when setVersion is gone. > > > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:339 > > + ASSERT(m_pendingSecondHalfOpenWithVersionCalls.size() <= 1); > > Given this, does it need to be a vector? (Maybe FIXME for later to clarify the state machine the spec implies.) Probably not, there's some comments about this in the ChangeLog, but I added a FIXME. > > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:423 > > + // Now that we give max priority to open calls that follow upgradeneeded > > Reword to remove the "Now that...", e.g. "We give max priority to open calls that follow upgradeneeded events; trigger the rest of the queues being service when those open calls are finished." Done. Comment on attachment 155914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155914&action=review Just a bunch of style nits > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:325 > + // If this was an open-with-version call, there should be a "second Make this a FIXME maybe? > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:447 > + ASSERT_NOT_REACHED(); It's a little confusing to have this without an associated FIXME. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:458 > + // The spec dictates we wait until all the version change events are Should this be a FIXME? > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:470 > + RefPtr<DOMStringList> objectStoreNames = DOMStringList::create(); Nit: I'd put a line-break before this line just for readability after the early return. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:478 > + if (!transaction->scheduleTask(createCallbackTask(&IDBDatabaseBackendImpl::setIntVersionInternal, database, requestedVersion, callbacks, transaction), > + createCallbackTask(&IDBDatabaseBackendImpl::resetVersion, database, m_version))) { This might be a little easier to read if you created the callback tasks before the if-statement. > Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:168 > + databaseBackend = IDBDatabaseBackendImpl::create(name, backingStore.get(), m_transactionCoordinator.get(), this, uniqueIdentifier); Nit: I'd prefer a line-break before this line for readability. > Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:177 > + if (version == IDBDatabaseMetadata::NoIntVersion) Nit: would prefer a line-break before this line as well. Comment on attachment 155914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155914&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:325 >> + // If this was an open-with-version call, there should be a "second > > Make this a FIXME maybe? SG >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:447 >> + ASSERT_NOT_REACHED(); > > It's a little confusing to have this without an associated FIXME. Agreed, added. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:458 >> + // The spec dictates we wait until all the version change events are > > Should this be a FIXME? Done. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:470 >> + RefPtr<DOMStringList> objectStoreNames = DOMStringList::create(); > > Nit: I'd put a line-break before this line just for readability after the early return. Done. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:478 >> + createCallbackTask(&IDBDatabaseBackendImpl::resetVersion, database, m_version))) { > > This might be a little easier to read if you created the callback tasks before the if-statement. Done. >> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:168 >> + databaseBackend = IDBDatabaseBackendImpl::create(name, backingStore.get(), m_transactionCoordinator.get(), this, uniqueIdentifier); > > Nit: I'd prefer a line-break before this line for readability. Done. >> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:177 >> + if (version == IDBDatabaseMetadata::NoIntVersion) > > Nit: would prefer a line-break before this line as well. Done. Created attachment 156200 [details]
Patch for landing
Comment on attachment 156200 [details] Patch for landing Clearing flags on attachment: 156200 Committed r124540: <http://trac.webkit.org/changeset/124540> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 93055 Created attachment 156451 [details]
Patch
Does anyone care to comment on (1) below? If I don't hear anything by monday sometime I'll resubmit. There were 3 compile problems on windows 1) There's no PRId64 printf conversion specifier for int64_t, so IDBDatabaseBackendImpl.cpp now casts to long long and uses %lld. I still want to store integer versions as int64_t to match the WebIDL definition of long long and so that we know exactly how many bytes we have to write to leveldb. 2) IDBLevelDBBackingStore::createIDBDatabaseMetadata had a non-const parameter and a derived class had const 3) IDBCallbacks.h needed some #includes (In reply to comment #27) > Does anyone care to comment on (1) below? If I don't hear anything by monday sometime I'll resubmit. > > There were 3 compile problems on windows > > 1) There's no PRId64 printf conversion specifier for int64_t, so IDBDatabaseBackendImpl.cpp now casts to long long and uses %lld. I still want to store integer versions as int64_t to match the WebIDL definition of long long and so that we know exactly how many bytes we have to write to leveldb. SGTM - verify it compiles everywhere with a git-try? > > 2) IDBLevelDBBackingStore::createIDBDatabaseMetadata had a non-const parameter and a derived class had const > > 3) IDBCallbacks.h needed some #includes (In reply to comment #28) > (In reply to comment #27) > > Does anyone care to comment on (1) below? If I don't hear anything by monday sometime I'll resubmit. > > > > There were 3 compile problems on windows > > > > 1) There's no PRId64 printf conversion specifier for int64_t, so IDBDatabaseBackendImpl.cpp now casts to long long and uses %lld. I still want to store integer versions as int64_t to match the WebIDL definition of long long and so that we know exactly how many bytes we have to write to leveldb. > > SGTM - verify it compiles everywhere with a git-try? I did win_rel and mac_rel, but now that you mention it, I should really try everything. > > > > > 2) IDBLevelDBBackingStore::createIDBDatabaseMetadata had a non-const parameter and a derived class had const > > > > 3) IDBCallbacks.h needed some #includes Comment on attachment 156451 [details] Patch Clearing flags on attachment: 156451 Committed r124675: <http://trac.webkit.org/changeset/124675> All reviewed patches have been landed. Closing bug. |