Replace hand-written IDBObserverCallback with auto-generated code

Replace hand-written IDBObserverCallback with auto-generated code

Like PerformanceObserverCallback we can auto-generate bindings
for IDBObserverCallback.

BUG=648486
Committed: https://crrev.com/29552806a635424f888d0f1a11fe853c50c71733
Cr-Commit-Position: refs/heads/master@{#424313}

[bashi, 2016-10-07 07:07:25]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, peria@chromium.org, yukishiino@chromium.org

[bashi, 2016-10-07 07:07:26]

PTAL (I have one question before asking indexeddb OWNERS review)

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
(right):

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:57:
V8PrivateProperty::getIDBObserverCallback(info.GetIsolate())
Auto-generated IDBObserverCallback calls setPhantom() for |v8Callback|. I'm not
sure we need this.

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/utilities.py (left):

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/utilities.py:363: match =
re.search(r'\[(.*)\]\s*'
This regexp is totally broken and doesn't work following case :(

[ExperimentalCallbackFunction] callback foo = void ();
[...] interface { ... }

[haraken, 2016-10-07 07:23:35]

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
(right):

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:57:
V8PrivateProperty::getIDBObserverCallback(info.GetIsolate())
On 2016/10/07 07:07:26, bashi1 wrote:
> Auto-generated IDBObserverCallback calls setPhantom() for |v8Callback|. I'm
not
> sure we need this.

The point is that we need to add a strong reference from IDBObserver's wrapper
to the callback. Before this CL, we were doing that by using V8PrivateProperty.
After this CL, we're expected to do that using
V8IDBObserverCallback::setWrapperReference.

So I think what we should do is:

- Add the strong reference with V8IDBObserverCallback::setWrapperReference.
- Remove this custom binding.

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:106: //
crbug.com/653769

BTW, is there any scenario where we want to handle the exception thrown by
m_callback->call()?

[bashi, 2016-10-07 07:28:38]

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
(right):

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:57:
V8PrivateProperty::getIDBObserverCallback(info.GetIsolate())
On 2016/10/07 07:23:35, haraken wrote:
> On 2016/10/07 07:07:26, bashi1 wrote:
> > Auto-generated IDBObserverCallback calls setPhantom() for |v8Callback|. I'm
> not
> > sure we need this.
> 
> The point is that we need to add a strong reference from IDBObserver's wrapper
> to the callback. Before this CL, we were doing that by using
V8PrivateProperty.
> After this CL, we're expected to do that using
> V8IDBObserverCallback::setWrapperReference.
> 
> So I think what we should do is:
> 
> - Add the strong reference with V8IDBObserverCallback::setWrapperReference.
> - Remove this custom binding.
> 

OK, let me ask another question. Who/when to call setWrapperReference() ?

We want to remove this custom binding but I think we should do it in follow-up
CLs (Maybe after all hand-written code for callbacks are gone).

[haraken, 2016-10-07 07:37:27]

On 2016/10/07 07:28:38, bashi1 wrote:
>
https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
> File
> third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
> (right):
> 
>
https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
>
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:57:
> V8PrivateProperty::getIDBObserverCallback(info.GetIsolate())
> On 2016/10/07 07:23:35, haraken wrote:
> > On 2016/10/07 07:07:26, bashi1 wrote:
> > > Auto-generated IDBObserverCallback calls setPhantom() for |v8Callback|.
I'm
> > not
> > > sure we need this.
> > 
> > The point is that we need to add a strong reference from IDBObserver's
wrapper
> > to the callback. Before this CL, we were doing that by using
> V8PrivateProperty.
> > After this CL, we're expected to do that using
> > V8IDBObserverCallback::setWrapperReference.
> > 
> > So I think what we should do is:
> > 
> > - Add the strong reference with V8IDBObserverCallback::setWrapperReference.
> > - Remove this custom binding.
> > 
> 
> OK, let me ask another question. Who/when to call setWrapperReference() ?
> 
> We want to remove this custom binding but I think we should do it in follow-up
> CLs (Maybe after all hand-written code for callbacks are gone).

Oh, sorry, I noticed that the generated callback function is calling
V8PrivateProperty::getXXX().set() in XXXCallback's constructor. So I think you
can simply remove the custom bindings.

Then I begin to wonder what XXXCallback::setWrapperReference is for (who wants
to call it). Does someone want to add the callback from things other than the
associated wrapper?

[bashi, 2016-10-07 07:37:31]

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:106: //
crbug.com/653769
On 2016/10/07 07:23:35, haraken wrote:
> 
> BTW, is there any scenario where we want to handle the exception thrown by
> m_callback->call()?
> 

(Forgot to reply this...)

Yeah, if we don't have to allow user scripts to catch exceptions, I think we can
omit ExceptionState. I just don't know, so I filed a bug for that.

[bashi, 2016-10-07 07:44:29]

On 2016/10/07 07:37:27, haraken wrote:
> On 2016/10/07 07:28:38, bashi1 wrote:
> >
>
https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
> > File
> > third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
> >
>
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:57:
> > V8PrivateProperty::getIDBObserverCallback(info.GetIsolate())
> > On 2016/10/07 07:23:35, haraken wrote:
> > > On 2016/10/07 07:07:26, bashi1 wrote:
> > > > Auto-generated IDBObserverCallback calls setPhantom() for |v8Callback|.
> I'm
> > > not
> > > > sure we need this.
> > > 
> > > The point is that we need to add a strong reference from IDBObserver's
> wrapper
> > > to the callback. Before this CL, we were doing that by using
> > V8PrivateProperty.
> > > After this CL, we're expected to do that using
> > > V8IDBObserverCallback::setWrapperReference.
> > > 
> > > So I think what we should do is:
> > > 
> > > - Add the strong reference with
V8IDBObserverCallback::setWrapperReference.
> > > - Remove this custom binding.
> > > 
> > 
> > OK, let me ask another question. Who/when to call setWrapperReference() ?
> > 
> > We want to remove this custom binding but I think we should do it in
follow-up
> > CLs (Maybe after all hand-written code for callbacks are gone).
> 
> Oh, sorry, I noticed that the generated callback function is calling
> V8PrivateProperty::getXXX().set() in XXXCallback's constructor. So I think you
> can simply remove the custom bindings.

Hmm, maybe I'm looking at wrong code, but it seems generated code doesn't call
V8PrivateProperty::getXXX().set(). Could you point where you are looking at?

> 
> Then I begin to wonder what XXXCallback::setWrapperReference is for (who wants
> to call it). Does someone want to add the callback from things other than the
> associated wrapper?

I think it was added for future use -- when the trace wrapper is fully working.
yukishiino@ should confirm it.

[haraken, 2016-10-07 07:56:16]

On 2016/10/07 07:44:29, bashi1 wrote:
> On 2016/10/07 07:37:27, haraken wrote:
> > On 2016/10/07 07:28:38, bashi1 wrote:
> > >
> >
>
https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
> > > File
> > >
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2397363002/diff/1/third_party/WebKit/Source/b...
> > >
> >
>
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:57:
> > > V8PrivateProperty::getIDBObserverCallback(info.GetIsolate())
> > > On 2016/10/07 07:23:35, haraken wrote:
> > > > On 2016/10/07 07:07:26, bashi1 wrote:
> > > > > Auto-generated IDBObserverCallback calls setPhantom() for
|v8Callback|.
> > I'm
> > > > not
> > > > > sure we need this.
> > > > 
> > > > The point is that we need to add a strong reference from IDBObserver's
> > wrapper
> > > > to the callback. Before this CL, we were doing that by using
> > > V8PrivateProperty.
> > > > After this CL, we're expected to do that using
> > > > V8IDBObserverCallback::setWrapperReference.
> > > > 
> > > > So I think what we should do is:
> > > > 
> > > > - Add the strong reference with
> V8IDBObserverCallback::setWrapperReference.
> > > > - Remove this custom binding.
> > > > 
> > > 
> > > OK, let me ask another question. Who/when to call setWrapperReference() ?
> > > 
> > > We want to remove this custom binding but I think we should do it in
> follow-up
> > > CLs (Maybe after all hand-written code for callbacks are gone).
> > 
> > Oh, sorry, I noticed that the generated callback function is calling
> > V8PrivateProperty::getXXX().set() in XXXCallback's constructor. So I think
you
> > can simply remove the custom bindings.
> 
> Hmm, maybe I'm looking at wrong code, but it seems generated code doesn't call
> V8PrivateProperty::getXXX().set(). Could you point where you are looking at?
> 
> > 
> > Then I begin to wonder what XXXCallback::setWrapperReference is for (who
wants
> > to call it). Does someone want to add the callback from things other than
the
> > associated wrapper?
> 
> I think it was added for future use -- when the trace wrapper is fully
working.
> yukishiino@ should confirm it.

Discussed offline and we have concluded that the
V8PrivateProperty::getXXX().set() would be needed in the custom constructor.
LGTM.

Let's think about a way to remove the custom constructor in a follow-up CL.

[bashi, 2016-10-07 08:02:55]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 08:03:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bashi, 2016-10-07 08:04:18]

bashi@chromium.org changed reviewers:
+ jsbell@chromium.org

[bashi, 2016-10-07 08:04:19]

Updated comment.

jsbell@: Could you review indexeddb/ ?

[commit-bot: I haz the power, 2016-10-07 09:27:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 09:27:23]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-10-07 16:39:08]

lgtm

[bashi, 2016-10-10 23:16:55]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-10-10 23:16:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2397363002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-10 23:17:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 00:51:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-11 00:54:27]

Description was changed from

==========
Replace hand-written IDBObserverCallback with auto-generated code

Like PerformanceObserverCallback we can auto-generate bindings
for IDBObserverCallback.

BUG=648486
==========

to

==========
Replace hand-written IDBObserverCallback with auto-generated code

Like PerformanceObserverCallback we can auto-generate bindings
for IDBObserverCallback.

BUG=648486

Committed: https://crrev.com/29552806a635424f888d0f1a11fe853c50c71733
Cr-Commit-Position: refs/heads/master@{#424313}
==========

[commit-bot: I haz the power, 2016-10-11 00:54:28]

Patchset 3 (id:??) landed as
https://crrev.com/29552806a635424f888d0f1a11fe853c50c71733
Cr-Commit-Position: refs/heads/master@{#424313}