CallbackPromiseAdapter types should be more compatible with WebCallbacks (1/3).

CallbackPromiseAdapter types should be more compatible with WebCallbacks (1/3).

This series of CLs makes CallbackPromiseAdapter (CPA) more compatible with
WebCallbacks (WC) in terms of type parameters.

Before this CL, CPA<S, T> corresponded to WC<S::WebType*, T::WebType*>. CPAs
took ownership of passed pointers but there was no place to represent that in
WebCallbacks.

This series of CLs changes that: CPA<S, T> correspond to WC<S::WebType,
T::WebType>. CPA users can specify if he/she wants the parameter ownership by
specifying the type parameter. For example, setting S::WebType to OwnPtr<X>
means it takes ownership. Setting S::WebType to |const X&| means it doesn't
take ownership.
WebCallbacks is exposed to chromium side, so we use WebPassOwnPtr as the
counterpart of PassOwnPtr.

This CL moves WebTypes from S to OwnPtr<S>. In some cases we will
not want to use OwnPtr<S> but I will fix it in the third CL.
WebCallbacks.h and CallbackPromiseAdapter.h are not clean in this
CL but I will clean it up in the third CL.

[1] This CL.
[2] https://codereview.chromium.org/1235083006/
[3] https://codereview.chromium.org/1240763002/

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

[yhirano, 2015-07-15 05:49:26]

yhirano@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2015-07-15 06:19:24]

Hmm. Looking reasonable, but how to let modules express the intent to pass the
ownership would be a bit controversial.

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/CallbackPromiseAdapter.h:42: class
CallbackPromiseAdapterBase {
nit: This could probably be done without inheritance? Is this one introduced
mainly for namespace isolation?

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/CallbackPromiseAdapter.h:59: using Type =
WebPrivateOwnPtr<T>&&;
Rvalue reference is currently not allowed. We could try asking for exceptions
but exposing these types at public/platform feels a bit too visible. Should we
try introducing some PassOwnPtr-like machinery?

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/CallbackPromiseAdapter.h:67: // TODO(yhirano): Use
universal reference and std::forward.
nit: do you mean this TODO to be resolved after we support c++11 library
features in chromium? Might be nice to link to
https://chromium-cpp.appspot.com/.

[yhirano, 2015-07-15 08:18:21]

Patchset #6 (id:100001) has been deleted

[yhirano, 2015-07-15 08:49:45]

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/CallbackPromiseAdapter.h:42: class
CallbackPromiseAdapterBase {
On 2015/07/15 06:19:23, kinuko wrote:
> nit: This could probably be done without inheritance? Is this one introduced
> mainly for namespace isolation?

Yes. Do you like PS2?
By the way, I'm not very interested in cleaning up this file in this CL because
I will change it drastically in https://codereview.chromium.org/1240763002/

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/CallbackPromiseAdapter.h:59: using Type =
WebPrivateOwnPtr<T>&&;
On 2015/07/15 06:19:24, kinuko wrote:
> Rvalue reference is currently not allowed. We could try asking for exceptions
> but exposing these types at public/platform feels a bit too visible. Should we
> try introducing some PassOwnPtr-like machinery?

I see. I uploaded a separate CL https://codereview.chromium.org/1232033004/ and
made this CL use it.

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/CallbackPromiseAdapter.h:67: // TODO(yhirano): Use
universal reference and std::forward.
On 2015/07/15 06:19:23, kinuko wrote:
> nit: do you mean this TODO to be resolved after we support c++11 library
> features in chromium? Might be nice to link to
> https://chromium-cpp.appspot.com/.

Done.

[yhirano, 2015-07-21 07:15:33]

ping

[kinuko, 2015-07-21 11:27:06]

lgtm

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/1234603003/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/CallbackPromiseAdapter.h:42: class
CallbackPromiseAdapterBase {
On 2015/07/15 08:49:45, yhirano wrote:
> On 2015/07/15 06:19:23, kinuko wrote:
> > nit: This could probably be done without inheritance? Is this one introduced
> > mainly for namespace isolation?
> 
> Yes. Do you like PS2?
> By the way, I'm not very interested in cleaning up this file in this CL
because
> I will change it drastically in https://codereview.chromium.org/1240763002/

Ok, understood.

https://codereview.chromium.org/1234603003/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/1234603003/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/CallbackPromiseAdapter.h:67: // TODO(yhirano): Use
universal reference and std::forward.
nit: "... once they're allowed." would make the intention clearer

https://codereview.chromium.org/1234603003/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/CallbackPromiseAdapter.h:83: // TODO(yhirano): Add
comments.
When do you plan to add the comment? In the next follow-up patch?

We'll need to note that an argument that's given as WebPassOwnPtr is assumed to
be passed as OwnPtr (i.e. its corresponding WebType must be OwnPtr) while value
type is assumed to be passed by value.

[yhirano, 2015-07-22 11:15:54]

Patchset #7 (id:140001) has been deleted

[yhirano, 2015-07-22 11:17:38]

https://codereview.chromium.org/1234603003/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/1234603003/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/CallbackPromiseAdapter.h:67: // TODO(yhirano): Use
universal reference and std::forward.
On 2015/07/21 11:27:06, kinuko wrote:
> nit: "... once they're allowed." would make the intention clearer

Done.

https://codereview.chromium.org/1234603003/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/CallbackPromiseAdapter.h:83: // TODO(yhirano): Add
comments.
On 2015/07/21 11:27:06, kinuko wrote:
> When do you plan to add the comment? In the next follow-up patch?
> 
> We'll need to note that an argument that's given as WebPassOwnPtr is assumed
to
> be passed as OwnPtr (i.e. its corresponding WebType must be OwnPtr) while
value
> type is assumed to be passed by value.

Yes,
https://codereview.chromium.org/1240763002/diff/60001/Source/bindings/core/v8...
for example.

[yhirano, 2015-07-24 02:59:34]

Patchset #8 (id:180001) has been deleted

[yhirano, 2015-07-27 12:25:54]

yhirano@chromium.org changed reviewers:
+ mlamouri@chromium.org, mvanouwerkerk@chromium.org, peter@chromium.org,
scheib@chromium.org

[yhirano, 2015-07-27 12:27:01]

yhirano@chromium.org changed reviewers:
+ haraken@chromium.org, mek@chromium.org

[yhirano, 2015-07-27 12:31:05]

PTAL.

mlamouri@ for modules/presentation/, modules/app_banner,
publc/platform/modules/app_banner, public/platform/modules/presentation.
mvanouwerkerk@ for modules/push_messaging,
public/platform/modules/push_messaging.
scheib@ for public/platform/modules/bluetooth.
mek@ for public/platform/modules/geofencing.
peter@ for modules/notifications, public/platform/modules/notifications.
haraken@ for bindings/

Please note that this CL is not so clean (sorry). This is the first CL of the
series and I will clean up code in the last one.

[Michael van Ouwerkerk, 2015-07-27 12:40:46]

push_messaging lgtm

[haraken, 2015-07-27 12:48:32]

bindings/ LGTM

https://codereview.chromium.org/1234603003/diff/240001/Source/bindings/core/v...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/1234603003/diff/240001/Source/bindings/core/v...
Source/bindings/core/v8/CallbackPromiseAdapter.h:78: // TODO(yhirano): Add
comments.

Can we add a comment in this CL?

[Peter Beverloo, 2015-07-27 12:51:12]

notifications lgtm

Since this is a three-part CL, could you please link to the other ones in the
descriptions? E.g.

  [1] This CL.
  [2] https://codereview.chromium.org/1235083006/
  [3] https://codereview.chromium.org/1240763002/

[Marijn Kruisselbrink, 2015-07-27 16:58:12]

geofencing LGTM

[scheib, 2015-07-27 17:35:28]

With comments as per haraken and as noted below, LGTM.

https://codereview.chromium.org/1234603003/diff/240001/public/platform/module...
File public/platform/modules/bluetooth/WebBluetooth.h (right):

https://codereview.chromium.org/1234603003/diff/240001/public/platform/module...
public/platform/modules/bluetooth/WebBluetooth.h:24: // WebBluetoothDevice and
WebBluetoothError object ownership is transferred.
Comments of form "// ... and ... object ownership is transferred" are redundant
with code now -- the code explains this clearly enough so please remove
comments.

[yhirano, 2015-07-29 03:32:31]

Patchset #12 (id:280001) has been deleted

[yhirano, 2015-07-29 03:32:37]

Patchset #12 (id:300001) has been deleted

[yhirano, 2015-07-29 03:32:49]

Patchset #11 (id:260001) has been deleted

[yhirano, 2015-07-29 04:01:52]

yhirano@chromium.org changed reviewers:
+ avavyvod@chromium.org
- mlamouri@chromium.org

[yhirano, 2015-07-29 04:01:54]

Thanks for the review!

> Since this is a three-part CL, could you please link to the > other ones in
the
> descriptions?
Done.

I reverted app_banner related changes because it no longer uses
CallbackPromiseAdapter.
avavyvod@, can you take a look at the presentation change?

https://codereview.chromium.org/1234603003/diff/240001/Source/bindings/core/v...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/1234603003/diff/240001/Source/bindings/core/v...
Source/bindings/core/v8/CallbackPromiseAdapter.h:78: // TODO(yhirano): Add
comments.
On 2015/07/27 12:48:31, haraken wrote:
> 
> Can we add a comment in this CL?

I will add comments at the third CL - Is that enough? Copying the comments from
the CL to this one is possible, but the comment is not fully correct at this
moment.

https://codereview.chromium.org/1234603003/diff/240001/public/platform/module...
File public/platform/modules/bluetooth/WebBluetooth.h (right):

https://codereview.chromium.org/1234603003/diff/240001/public/platform/module...
public/platform/modules/bluetooth/WebBluetooth.h:24: // WebBluetoothDevice and
WebBluetoothError object ownership is transferred.
On 2015/07/27 17:35:28, scheib wrote:
> Comments of form "// ... and ... object ownership is transferred" are
redundant
> with code now -- the code explains this clearly enough so please remove
> comments.

Done.

[yhirano, 2015-07-29 04:02:23]

yhirano@chromium.org changed reviewers:
+ avayvod@chromium.org
- avavyvod@chromium.org

[yhirano, 2015-07-29 04:02:45]

I reverted app_banner related changes because it no longer uses
CallbackPromiseAdapter.
avavyvod@, can you take a look at the presentation change?

[yhirano, 2015-08-04 03:36:32]

On 2015/07/29 04:02:45, yhirano wrote:
> I reverted app_banner related changes because it no longer uses
> CallbackPromiseAdapter.
> avavyvod@, can you take a look at the presentation change?

ping

[whywhat, 2015-08-05 12:44:54]

On 2015/08/04 at 03:36:32, yhirano wrote:
> On 2015/07/29 04:02:45, yhirano wrote:
> > I reverted app_banner related changes because it no longer uses
> > CallbackPromiseAdapter.
> > avavyvod@, can you take a look at the presentation change?
> 
> ping

presentation/ lgtm
sorry for the delay

[yhirano, 2015-08-06 01:11:49]

yhirano@chromium.org changed reviewers:
+ tkent@chromium.org

[yhirano, 2015-08-06 01:11:50]

tkent@, can you take a look at public/ changes?

[tkent, 2015-08-06 02:09:11]

public lgtm

[yhirano, 2015-08-06 02:11:41]

Thank you, all reviewers!

[yhirano, 2015-08-06 02:11:49]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-08-06 02:11:49]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
peter@chromium.org, scheib@chromium.org, mvanouwerkerk@chromium.org,
haraken@chromium.org, mek@chromium.org
Link to the patchset: https://codereview.chromium.org/1234603003/#ps380001
(title: " ")

[commit-bot: I haz the power, 2015-08-06 02:11:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234603003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1234603003/380001

[commit-bot: I haz the power, 2015-08-06 03:02:07]

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

[commit-bot: I haz the power, 2015-08-06 03:02:09]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72747)

[yhirano, 2015-08-06 04:05:08]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-08-06 04:05:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234603003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1234603003/380001

[commit-bot: I haz the power, 2015-08-06 05:04:03]

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

[commit-bot: I haz the power, 2015-08-06 05:04:04]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72758)

[yhirano, 2015-08-06 11:33:08]

Patchset #22 (id:540001) has been deleted

[yhirano, 2015-08-06 11:33:20]

Patchset #22 (id:560001) has been deleted

[yhirano, 2015-08-06 11:33:26]

Patchset #21 (id:520001) has been deleted

[yhirano, 2015-08-06 11:33:32]

Patchset #20 (id:500001) has been deleted

[yhirano, 2015-08-06 12:26:31]

Patchset #24 (id:660001) has been deleted

[yhirano, 2015-08-06 12:26:57]

Patchset #22 (id:620001) has been deleted

[yhirano, 2015-08-06 12:27:11]

Patchset #21 (id:600001) has been deleted

[yhirano, 2015-08-06 12:27:34]

Patchset #22 (id:570041) has been deleted

[yhirano, 2015-08-06 12:32:20]

Patchset #21 (id:640001) has been deleted

[yhirano, 2015-08-06 12:33:34]

Patchset #20 (id:580001) has been deleted

[yhirano, 2015-08-06 13:30:49]

Patchset #22 (id:710001) has been deleted

[yhirano, 2015-08-07 02:35:38]

Patchset #29 (id:850001) has been deleted

[yhirano, 2015-08-07 08:33:32]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-08-07 08:33:32]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
peter@chromium.org, kinuko@chromium.org, tkent@chromium.org,
mvanouwerkerk@chromium.org, haraken@chromium.org, avayvod@chromium.org,
mek@chromium.org
Link to the patchset: https://codereview.chromium.org/1234603003/#ps970001
(title: "retry!")

[yhirano, 2015-08-07 08:33:33]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2015-08-07 08:38:50]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-08-07 08:38:51]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
peter@chromium.org, kinuko@chromium.org, tkent@chromium.org,
mvanouwerkerk@chromium.org, haraken@chromium.org, avayvod@chromium.org,
mek@chromium.org
Link to the patchset: https://codereview.chromium.org/1234603003/#ps970001
(title: "retry!")

[commit-bot: I haz the power, 2015-08-07 08:39:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234603003/970001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1234603003/970001

[commit-bot: I haz the power, 2015-08-07 09:34:39]

Committed patchset #34 (id:970001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200156