Bug 101483 – IndexedDB: Replace use of ScriptExecutionContext::Task (Part 1)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

We use ScriptExecutionContext::Task in IDBTransactionBackendImpl to hold "tasks", but this pulls a lot of machinery in that we don't need e.g. ThreadSafeRefCounted, and so forth.

Consider replacing this usage with something else - either a generic closure class, or operation-specific holders (e.g. ObjectStoreStorageOperation, ObjectStoreRetrievalOperation, etc).

The latter is particularly appealing when we consider moving those to the front end as they would encapsulate the asynchronous logic (e.g. for an "add" you need to do at least one "get" to verify that the value is not present, more "gets" if there are "unique" indexes, then several "puts" - all of which may be async)

[Alec Flett]

I believe this is also the last use of ScriptExecutionContext in the backend - so when this is fixed, all references to that should be removed (if anything, that's kind of a test if this bug is fixed or not)

[Joshua Bell]

Created attachment 176608 [details]
Patch

[Joshua Bell]

Comment on attachment 176608 [details]
Patch

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

This is what I'm thinking about as a first step. This is hard to read as a unified diff, so try side-by-side...

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:66
> +class IDBCursorBackendImpl::CursorIterationOperation

A type is defined for each operation. Cursors end up being trickier than most since they have state, unlike store/index operations.

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:71
> +        return createCallbackTask(&CursorIterationOperation::perform, cursor, key, callbacks);

This would be temporary for the first part of the refactoring - instead of returning an instance of this class, keep returning a ScriptExecutionContext::Task...

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:75
> +    static void perform(ScriptExecutionContext*, PassRefPtr<IDBCursorBackendImpl> cursor, PassRefPtr<IDBKey> key, PassRefPtr<IDBCallbacks> callbacks)

But move the implementation here - this used to be IDBCursorBackendImpl::continueInternal

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:166
> +    if (!m_transaction->scheduleTask(m_taskType, CursorIterationOperation::create(this, key, callbacks)))

Sample call site. This would then remain unchanged even when the operation stopped being a ScriptExecutionContext::Task.

[David Grogan]

Comment on attachment 176608 [details]
Patch

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

Let me know if I've understood the direction or totally missed the boat.

>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:66
>> +class IDBCursorBackendImpl::CursorIterationOperation
> 
> A type is defined for each operation. Cursors end up being trickier than most since they have state, unlike store/index operations.

Is the idea that there will be an Operation base class that this will derive from?
Operation would have one substantive method, e.g. void perform(void)?

>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:71
>> +        return createCallbackTask(&CursorIterationOperation::perform, cursor, key, callbacks);
> 
> This would be temporary for the first part of the refactoring - instead of returning an instance of this class, keep returning a ScriptExecutionContext::Task...

When this class derives from Operation, the parameters will be copied to class data members?

>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:166
>> +    if (!m_transaction->scheduleTask(m_taskType, CursorIterationOperation::create(this, key, callbacks)))
> 
> Sample call site. This would then remain unchanged even when the operation stopped being a ScriptExecutionContext::Task.

scheduleTask will take a PassRefPtr<Operation> ?

[Joshua Bell]

Comment on attachment 176608 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:66
>>> +class IDBCursorBackendImpl::CursorIterationOperation
>> 
>> A type is defined for each operation. Cursors end up being trickier than most since they have state, unlike store/index operations.
> 
> Is the idea that there will be an Operation base class that this will derive from?
> Operation would have one substantive method, e.g. void perform(void)?

Yes, exactly. If/when the backing store becomes asynchronous (to implement caching), the operations would end up with more internal state, callbacks, etc. with perform(void) as the entry point. But while the backing store is synchronous, it can all live in perform(void).

>>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:71
>>> +        return createCallbackTask(&CursorIterationOperation::perform, cursor, key, callbacks);
>> 
>> This would be temporary for the first part of the refactoring - instead of returning an instance of this class, keep returning a ScriptExecutionContext::Task...
> 
> When this class derives from Operation, the parameters will be copied to class data members?

Exactly.

>>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:166
>>> +    if (!m_transaction->scheduleTask(m_taskType, CursorIterationOperation::create(this, key, callbacks)))
>> 
>> Sample call site. This would then remain unchanged even when the operation stopped being a ScriptExecutionContext::Task.
> 
> scheduleTask will take a PassRefPtr<Operation> ?

Correct.

[David Grogan]

Sounds great. LGTM

[Alec Flett]

Comment on attachment 176608 [details]
Patch

I'm ambivalent: the functional programmer in me cringes at all the extra classes, but the C++ developer in me appreciates the encapsulation.

so LGTM!

[Joshua Bell]

Created attachment 176854 [details]
Patch

[Joshua Bell]

dgrogan@, alecflett@ - please take a look.

One nice thing I realized is that just as today the Task is implicitly called with ScriptExecutionContext* as the first parameter, eventually Operation::perform() can be called with IDBTransactionBackendImpl as the parameter so operations don't need to carry it with them.

[David Grogan]

Comment on attachment 176854 [details]
Patch

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

LGTM

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:-256
> -    RefPtr<IDBObjectStoreBackendImpl> objectStore = this;

I'm 95% sure these are unnecessary and not self references to guard against deletion in some weird situation. I'm assuming you convinced yourself of something similar?

[Joshua Bell]

(In reply to comment #10)
> > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:-256
> > -    RefPtr<IDBObjectStoreBackendImpl> objectStore = this;
> 
> I'm 95% sure these are unnecessary and not self references to guard against deletion in some weird situation. I'm assuming you convinced yourself of something similar?

Yes. I think it was either stylistic or perhaps createCallbackTask() didn't convert raw pointers to PassRefPtrs at some point, but they were not used consistently.

[Joshua Bell]

tony@ - can you r?

[Tony Chang]

Comment on attachment 176854 [details]
Patch

This seems like an improvement to static callbacks on the implementations, although it is a bit verbose.

[WebKit Review Bot]

Comment on attachment 176854 [details]
Patch

Clearing flags on attachment: 176854

Committed r136272: <http://trac.webkit.org/changeset/136272>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.