Bug 103931 – IndexedDB: Replace use of ScriptExecutionContext::Task (Part 2)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

IndexedDB: Replace use of ScriptExecutionContext::Task (Part 2)

[Joshua Bell]

Created attachment 177343 [details]
Patch

[Joshua Bell]

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?

[Joshua Bell]

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.)

[David Grogan]

Comment on attachment 177343 [details]
Patch

You can remove include "ScriptExecutionContext.h from IDBTransactionBackendImpl.* without compilation problems?

[Joshua Bell]

(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.

[Joshua Bell]

Created attachment 177353 [details]
Patch

[Alec Flett]

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()

[Joshua Bell]

(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)

[Joshua Bell]

(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?

[Tony Chang]

Comment on attachment 177353 [details]
Patch

rs=me

[WebKit Review Bot]

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

[Joshua Bell]

Created attachment 177762 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177762 [details]
Patch for landing

Clearing flags on attachment: 177762

Committed r136696: <http://trac.webkit.org/changeset/136696>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.