Bug 103931 - IndexedDB: Replace use of ScriptExecutionContext::Task (Part 2)
Summary: IndexedDB: Replace use of ScriptExecutionContext::Task (Part 2)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-03 14:42 PST by Joshua Bell
Modified: 2012-12-05 09:07 PST (History)
5 users (show)

See Also:


Attachments
Patch (113.55 KB, patch)
2012-12-03 15:05 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (115.71 KB, patch)
2012-12-03 15:39 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (116.92 KB, patch)
2012-12-05 08:40 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-12-03 14:42:51 PST
IndexedDB: Replace use of ScriptExecutionContext::Task (Part 2)
Comment 1 Joshua Bell 2012-12-03 15:05:47 PST
Created attachment 177343 [details]
Patch
Comment 2 Joshua Bell 2012-12-03 15:08:55 PST
Good riddance to ScriptExecutionContext::Task and incorrect usage of ThreadSafeRefCounted<> in IDB. Hooray!

But this does mean that the guts of each XXXXOperation class are spilled out explicitly rather than being hidden by the use of fancy-schmancy templates. I'm a fan of making things explicit here, since we can see where objects are being held where IDs might be better, reference holding is more explicit (e.g. for the AbortOperations), etc.

dgrogan@, alecflett@ - take a look?
Comment 3 Joshua Bell 2012-12-03 15:10:58 PST
Comment on attachment 177343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177343&action=review

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:507
> +        m_key = autoIncKey;

This is the only place where an XXXXOperation's member is modified. In the future (when operations are broken into multiple async steps) this might be common, but it's the odd case now. I am unsure whether to leave it like this or make a local copy.

(The operation is disposed of immediately following the perform() call so it doesn't affect the behavior at all.)

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:550
> +    m_callbacks->onSuccess(m_key.release());

See also here. (Again, doesn't actually matter since the Operation is disposed immediately after this method returns.)
Comment 4 David Grogan 2012-12-03 15:33:19 PST
Comment on attachment 177343 [details]
Patch

You can remove include "ScriptExecutionContext.h from IDBTransactionBackendImpl.* without compilation problems?
Comment 5 Joshua Bell 2012-12-03 15:39:15 PST
(In reply to comment #4)
> (From update of attachment 177343 [details])
> You can remove include "ScriptExecutionContext.h from IDBTransactionBackendImpl.* without compilation problems?

Yes, thanks. Nuked it from there and a few more places. Only IDBFactoryBackend* still needs it, since the chromium proxy uses it for permission checks.
Comment 6 Joshua Bell 2012-12-03 15:39:24 PST
Created attachment 177353 [details]
Patch
Comment 7 Alec Flett 2012-12-03 16:22:26 PST
Comment on attachment 177353 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177353&action=review

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.h:61
> +        virtual void perform(IDBTransactionBackendImpl*) = 0;

I know everything is using it here, but I'd sooner keep this as void perform() (without any params) so that future async operations that dont' use IDBTransactionBackendImpl can use this without passing null or refactoring again later. This also allows you to completely encapsulate the target state and ownership (i.e. of a particular IDBTransactionBackendImpl) in the constructor, rather than spreading it between parameters to the constructor and a later call to perform()
Comment 8 Joshua Bell 2012-12-03 16:28:59 PST
(In reply to comment #7)
> (From update of attachment 177353 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177353&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.h:61
> > +        virtual void perform(IDBTransactionBackendImpl*) = 0;
> 
> I know everything is using it here, but I'd sooner keep this as void perform() (without any params) so that future async operations that dont' use IDBTransactionBackendImpl can use this without passing null or refactoring again later. This also allows you to completely encapsulate the target state and ownership (i.e. of a particular IDBTransactionBackendImpl) in the constructor, rather than spreading it between parameters to the constructor and a later call to perform()

Not all use it - cursors are a special case (since they have an internal transaction reference due to lifetime issues) and AbortOperations MUST NOT use the transaction. The reason I did it this way is because all IDBTransactionBackendImpl::Operations are owned by an IDBTransactionBackendImpl so maintaining an "up pointer" is redundant. 

I would imagine that other queues of things might exist (c.f. the PendingXXXCall in IDBDatabaseBackendImpl) that wouldn't need a transaction, but there's nothing special about IDBTransactionBackendImpl::Operations that require its use... but maybe I'm missing something. I'll ponder (or discuss offline)
Comment 9 Joshua Bell 2012-12-04 14:23:16 PST
(In reply to comment #8)
> The reason I did it this way is because all IDBTransactionBackendImpl::Operations are owned by an IDBTransactionBackendImpl so maintaining an "up pointer" is redundant. 

FYI, discussed this offline w/ alec and he claims to be satisfied.

tony@ - r?
Comment 10 Tony Chang 2012-12-04 14:46:42 PST
Comment on attachment 177353 [details]
Patch

rs=me
Comment 11 WebKit Review Bot 2012-12-04 19:27:30 PST
Comment on attachment 177353 [details]
Patch

Rejecting attachment 177353 [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:
eddb/IDBTransactionBackendImpl.cpp
patching file Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.h
patching file Source/WebCore/Modules/indexeddb/IDBTransactionBackendInterface.h
patching file Source/WebCore/Modules/indexeddb/IDBTransactionCoordinator.cpp
patching file Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.cpp

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/15158099
Comment 12 Joshua Bell 2012-12-05 08:40:11 PST
Created attachment 177762 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-12-05 09:07:26 PST
Comment on attachment 177762 [details]
Patch for landing

Clearing flags on attachment: 177762

Committed r136696: <http://trac.webkit.org/changeset/136696>
Comment 14 WebKit Review Bot 2012-12-05 09:07:30 PST
All reviewed patches have been landed.  Closing bug.