Replace IDBPendingTransactionMonitor with end-of-recursion-scope tasks

Replace IDBPendingTransactionMonitor with end-of-recursion-scope tasks

V8PerIsolateData cannot own IDBPendingTransactionMonitor, because
IDBPendingTransactionMonitor is defined in modules/indexeddb and
V8PerIsolateData is defined in bindings/core. Replace the monitor
entirely by having IDBTransactions register tasks to be run once
the scopes are unwound.

BUG=358074
R=haraken,tasak,adamk

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185137

[jsbell, 2014-11-10 20:04:20]

Since ModuleProxy::didLeaveScriptContextForRecursionScope() is a no-op with this
change, we could remove that and the registration code as well.

[adamk, 2014-11-10 20:29:22]

lgtm

I love this approach. Still a little squeamish about adding a general notion of
"end of scope tasks", since it's easy to confuse that with microtasks. I like
the ASSERT that end of scope tasks don't queue any more tasks. I'll think about
whether there's anything more we can do to reduce their power or make it clear
what kind of work they're for. Do you think it's too heavy-handed to add a
comment with EndOfScopeTask warning against its usage without consulting
<experts>?

https://codereview.chromium.org/686153003/diff/1/Source/bindings/core/v8/V8Pe...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/686153003/diff/1/Source/bindings/core/v8/V8Pe...
Source/bindings/core/v8/V8PerIsolateData.cpp:261: for (OwnPtr<EndOfScopeTask>&
task : tasks)
Why not

auto& task

?

[jsbell, 2014-11-10 20:51:54]

On 2014/11/10 20:29:22, adamk wrote:
> Do you think it's too heavy-handed to add a
> comment with EndOfScopeTask warning against its usage without consulting
> <experts>?

I think that's a good idea. As we've discussed in person, this "end of task"
concept is fairly unique to Indexed DB and inspired microtasks but is very
different from them, and the composition with microtasks is weird enough that we
should really avoid introducing more.

Maybe something like:

// EndOfScopeTasks are run by V8RecursionScope when
// control is returning to C++ from script, after
// executing a script task (e.g. callback, event)
// or microtasks (e.g. promise). This is explicitly
// needed for Indexed DB transactions per spec, but
// should in general be avoided.

https://codereview.chromium.org/686153003/diff/1/Source/bindings/core/v8/V8Pe...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/686153003/diff/1/Source/bindings/core/v8/V8Pe...
Source/bindings/core/v8/V8PerIsolateData.cpp:261: for (OwnPtr<EndOfScopeTask>&
task : tasks)
On 2014/11/10 20:29:21, adamk wrote:
> Why not
> 
> auto& task

Happy to change. I used an explicit type since it's "complicated" (an OwnPtr)
but not too unreadably long. I'm also over-compensating for being a huge fan of
auto. :)

[adamk, 2014-11-10 20:57:34]

I like your proposed comment text a lot, please add!

https://codereview.chromium.org/686153003/diff/1/Source/bindings/core/v8/V8Pe...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/686153003/diff/1/Source/bindings/core/v8/V8Pe...
Source/bindings/core/v8/V8PerIsolateData.cpp:261: for (OwnPtr<EndOfScopeTask>&
task : tasks)
On 2014/11/10 20:51:54, jsbell wrote:
> On 2014/11/10 20:29:21, adamk wrote:
> > Why not
> > 
> > auto& task
> 
> Happy to change. I used an explicit type since it's "complicated" (an OwnPtr)
> but not too unreadably long. I'm also over-compensating for being a huge fan
of
> auto. :)

Given that the type already appears 2 lines above, I think auto is fine here.

[jsbell, 2014-11-10 21:22:42]

On 2014/11/10 20:57:34, adamk wrote:
> I like your proposed comment text a lot, please add!

Done.

https://codereview.chromium.org/686153003/diff/1/Source/bindings/core/v8/V8Pe...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/686153003/diff/1/Source/bindings/core/v8/V8Pe...
Source/bindings/core/v8/V8PerIsolateData.cpp:261: for (OwnPtr<EndOfScopeTask>&
task : tasks)
On 2014/11/10 20:57:34, adamk wrote:
> On 2014/11/10 20:51:54, jsbell wrote:
> > On 2014/11/10 20:29:21, adamk wrote:
> > > Why not
> > > 
> > > auto& task
> > 
> > Happy to change. I used an explicit type since it's "complicated" (an
OwnPtr)
> > but not too unreadably long. I'm also over-compensating for being a huge fan
> of
> > auto. :)
> 
> Given that the type already appears 2 lines above, I think auto is fine here.

Done.

[adamk, 2014-11-10 21:54:12]

still lgtm

tasak should take a look and see what he thinks of this approach, modules-wise.

[haraken, 2014-11-11 01:09:15]

LGTM, this looks nicer!

https://codereview.chromium.org/686153003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/ModuleBindingsInitializer.cpp (right):

https://codereview.chromium.org/686153003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/ModuleBindingsInitializer.cpp:22: void
ModuleBindingsInitializer::init()

Can we probably completely remove ModuleBindingsInitializer?

(tasak@ will take a look at it.)

https://codereview.chromium.org/686153003/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/686153003/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBTransaction.cpp:78:
DeactivateTransactionTask(IDBTransaction* transaction)

Add explicit.

[tasak, 2014-11-11 02:11:53]

lgtm

[jsbell, 2014-11-11 17:37:15]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-11-11 17:38:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/686153003/40001

[jsbell, 2014-11-11 17:38:26]

https://codereview.chromium.org/686153003/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/ModuleBindingsInitializer.cpp (right):

https://codereview.chromium.org/686153003/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/ModuleBindingsInitializer.cpp:22: void
ModuleBindingsInitializer::init()
On 2014/11/11 01:09:15, haraken wrote:
> 
> Can we probably completely remove ModuleBindingsInitializer?
> 
> (tasak@ will take a look at it.)

We still need the initPartialInterfacesInModules() hookup. But yeah, I'm sure
tasak@ can find more to rip out and simplify.

https://codereview.chromium.org/686153003/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/686153003/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBTransaction.cpp:78:
DeactivateTransactionTask(IDBTransaction* transaction)
On 2014/11/11 01:09:15, haraken wrote:
> 
> Add explicit.

Done.

[commit-bot: I haz the power, 2014-11-11 18:43:51]

Committed patchset #3 (id:40001) as 185137