Bug 96663

Summary: IndexedDB: Remove "current transaction" concept from backing store
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: New BugsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dglazkov, dgrogan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Joshua Bell 2012-09-13 09:58:04 PDT
IndexedDB: Remove "current transaction" concept from backing store
Comment 1 Joshua Bell 2012-09-13 10:01:01 PDT
Created attachment 163895 [details]
Patch
Comment 2 Joshua Bell 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)
Comment 3 WebKit Review Bot 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
Comment 4 Joshua Bell 2012-09-13 10:20:16 PDT
Created attachment 163900 [details]
Patch
Comment 5 David Grogan 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
Comment 6 Joshua Bell 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... !
Comment 7 Joshua Bell 2012-09-17 10:16:25 PDT
Created attachment 164411 [details]
Patch
Comment 8 Joshua Bell 2012-09-17 10:17:19 PDT
References -> Pointers

Using an overloaded casting operator is still a bit weird. Better suggestions appreciated.
Comment 9 David Grogan 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.
Comment 10 Alec Flett 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 '=' ?
Comment 11 Joshua Bell 2012-09-17 16:12:07 PDT
Created attachment 164471 [details]
Patch
Comment 12 Joshua Bell 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.
Comment 13 Joshua Bell 2012-09-18 10:19:03 PDT
alec, david - one more look before I send this off to tony?
Comment 14 Alec Flett 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
Comment 15 Joshua Bell 2012-09-18 11:42:28 PDT
Created attachment 164594 [details]
Patch
Comment 16 Joshua Bell 2012-09-18 12:05:20 PDT
tony@ - another one, not urgent - r?
Comment 17 WebKit Review Bot 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
Comment 18 Joshua Bell 2012-09-19 16:03:24 PDT
Created attachment 164793 [details]
Patch for landing
Comment 19 WebKit Review Bot 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>
Comment 20 WebKit Review Bot 2012-09-19 16:27:06 PDT
All reviewed patches have been landed.  Closing bug.