IndexedDB: Remove "current transaction" concept from backing store
Created attachment 163895 [details] Patch
This seems like a good initial step. As a follow-up, should we flip the API around so that most of the methods are on an IDBBackingStore::Transaction() (that has an internal IDBBackingStore pointer) rather than on IDBBackingStore itself? Or do that in this patch as well? (I vote for follow-up)
Comment on attachment 163895 [details] Patch Attachment 163895 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13833837
Created attachment 163900 [details] Patch
Why is Transaction& passed around instead of Transaction*? The WK style guide calls for out parameters to be passed by reference but that doesn't seem to be what's happening. Other than that, LGTM
(In reply to comment #5) > Why is Transaction& passed around instead of Transaction*? The WK style guide calls for out parameters to be passed by reference but that doesn't seem to be what's happening. Bleah, that's just my personal style showing through. Must... conform... !
Created attachment 164411 [details] Patch
References -> Pointers Using an overloaded casting operator is still a bit weird. Better suggestions appreciated.
(In reply to comment #8) > Using an overloaded casting operator is still a bit weird. Better suggestions appreciated. It does seem a little weird to pass an IDB object to leveldb. I guess the obvious alternative is to add getLevelDBTransaction() to IDBTransactionBackendImpl. I'd say the wordiness of that outweighs any downsides of overloading the casting operator.
I made a similar comment this morning :) I wonder if it would alleviate the need for prpTransaction-fu that you're doing in some places too, since you're not dereferencing through an '=' ?
Created attachment 164471 [details] Patch
(In reply to comment #9) > (In reply to comment #8) > > Using an overloaded casting operator is still a bit weird. Better suggestions appreciated. > > It does seem a little weird to pass an IDB object to leveldb. I guess the obvious alternative is to add getLevelDBTransaction() to IDBTransactionBackendImpl. I'd say the wordiness of that outweighs any downsides of overloading the casting operator. Agreed, updated. (In reply to comment #10) > I made a similar comment this morning :) I wonder if it would alleviate the need for prpTransaction-fu that you're doing in some places too, since you're not dereferencing through an '=' ? That too. Also addressed the FIXME. We want to keep IDBBackingStore::begin() and create the LevelDBTransaction there, as that's the point we'd theoretically create the snapshot.
alec, david - one more look before I send this off to tony?
Comment on attachment 164471 [details] Patch LGTM, with one nit - ironic because this is reason #92 I hate the ChangeLogs :) View in context: https://bugs.webkit.org/attachment.cgi?id=164471&action=review > Source/WebCore/ChangeLog:89 > + (WebCore::IDBTransactionBackendImpl::operator IDBBackingStore::Transaction&): You don't have 'operator' in the patch anymore
Created attachment 164594 [details] Patch
tony@ - another one, not urgent - r?
Comment on attachment 164594 [details] Patch Rejecting attachment 164594 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: /Modules/indexeddb/IDBLevelDBBackingStore.h patching file Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp patching file Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.h patching file Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.h patching file Source/WebKit/chromium/tests/IDBFakeBackingStore.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Chang']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13902651
Created attachment 164793 [details] Patch for landing
Comment on attachment 164793 [details] Patch for landing Clearing flags on attachment: 164793 Committed r129066: <http://trac.webkit.org/changeset/129066>
All reviewed patches have been landed. Closing bug.