RESOLVED FIXED 96663
IndexedDB: Remove "current transaction" concept from backing store
https://bugs.webkit.org/show_bug.cgi?id=96663
Summary IndexedDB: Remove "current transaction" concept from backing store
Joshua Bell
Reported 2012-09-13 09:58:04 PDT
IndexedDB: Remove "current transaction" concept from backing store
Attachments
Patch (101.67 KB, patch)
2012-09-13 10:01 PDT, Joshua Bell
no flags
Patch (110.73 KB, patch)
2012-09-13 10:20 PDT, Joshua Bell
no flags
Patch (110.65 KB, patch)
2012-09-17 10:16 PDT, Joshua Bell
no flags
Patch (107.98 KB, patch)
2012-09-17 16:12 PDT, Joshua Bell
no flags
Patch (107.83 KB, patch)
2012-09-18 11:42 PDT, Joshua Bell
no flags
Patch for landing (108.09 KB, patch)
2012-09-19 16:03 PDT, Joshua Bell
no flags
Joshua Bell
Comment 1 2012-09-13 10:01:01 PDT
Joshua Bell
Comment 2 2012-09-13 10:03:22 PDT
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)
WebKit Review Bot
Comment 3 2012-09-13 10:07:17 PDT
Comment on attachment 163895 [details] Patch Attachment 163895 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13833837
Joshua Bell
Comment 4 2012-09-13 10:20:16 PDT
David Grogan
Comment 5 2012-09-14 18:06:14 PDT
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
Joshua Bell
Comment 6 2012-09-17 09:21:35 PDT
(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... !
Joshua Bell
Comment 7 2012-09-17 10:16:25 PDT
Joshua Bell
Comment 8 2012-09-17 10:17:19 PDT
References -> Pointers Using an overloaded casting operator is still a bit weird. Better suggestions appreciated.
David Grogan
Comment 9 2012-09-17 15:55:01 PDT
(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.
Alec Flett
Comment 10 2012-09-17 16:06:18 PDT
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 '=' ?
Joshua Bell
Comment 11 2012-09-17 16:12:07 PDT
Joshua Bell
Comment 12 2012-09-17 16:13:24 PDT
(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.
Joshua Bell
Comment 13 2012-09-18 10:19:03 PDT
alec, david - one more look before I send this off to tony?
Alec Flett
Comment 14 2012-09-18 11:39:52 PDT
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
Joshua Bell
Comment 15 2012-09-18 11:42:28 PDT
Joshua Bell
Comment 16 2012-09-18 12:05:20 PDT
tony@ - another one, not urgent - r?
WebKit Review Bot
Comment 17 2012-09-19 13:28:59 PDT
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
Joshua Bell
Comment 18 2012-09-19 16:03:24 PDT
Created attachment 164793 [details] Patch for landing
WebKit Review Bot
Comment 19 2012-09-19 16:27:02 PDT
Comment on attachment 164793 [details] Patch for landing Clearing flags on attachment: 164793 Committed r129066: <http://trac.webkit.org/changeset/129066>
WebKit Review Bot
Comment 20 2012-09-19 16:27:06 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.