Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211353004:
(WONT COMMIT) Pass a boPass a bool object allocated on heap to CallbackPromiseAdapter::onSuccess (2) (Closed)
DescriptionPass a bool object allocated on heap to CallbackPromiseAdapter::onSuccess (2)
CallbackPromiseAdapter is designed to take ownership of a passed object.
See comments in CallbackPromiseAdapter.h for details.
(1) Chromium: https://codereview.chromium.org/1217633003/
(2) Blink: THIS PATCH
(3) Chromium: https://codereview.chromium.org/1225543002/
(4) Blink: https://codereview.chromium.org/1221693004/
BUG=493531
Patch Set 1 : #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 16 (4 generated)
Patchset #1 (id:1) has been deleted
nhiroki@chromium.org changed reviewers: + yhirano@chromium.org
PTAL
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/CallbackPromiseAdapter.h:257: m_resolver->resolve(*ownPtr); This will end up with calling resolveOrReject(OwnPtr<bool>, ...), which will call toV8(OwnPtr<bool>, ...). This seems not quite right to me. hirano-san?
https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/CallbackPromiseAdapter.h:257: m_resolver->resolve(*ownPtr); On 2015/07/03 05:57:49, haraken wrote: > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), which will > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > hirano-san? > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I missing anything?
https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/CallbackPromiseAdapter.h:257: m_resolver->resolve(*ownPtr); On 2015/07/03 06:04:12, yhirano wrote: > On 2015/07/03 05:57:49, haraken wrote: > > > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), which will > > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > > > hirano-san? > > > > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I missing > anything? ah, OK. Then what's the merit of using an OwnPtr? It looks a bit strange to use OwnPtr<bool>.
https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/CallbackPromiseAdapter.h:257: m_resolver->resolve(*ownPtr); On 2015/07/03 06:09:06, haraken wrote: > On 2015/07/03 06:04:12, yhirano wrote: > > On 2015/07/03 05:57:49, haraken wrote: > > > > > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), which will > > > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > > > > > hirano-san? > > > > > > > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I missing > > anything? > > ah, OK. Then what's the merit of using an OwnPtr? It looks a bit strange to use > OwnPtr<bool>. Sorry, I'm not really sure why you feel this a bit strange... which solution are you thinking? (1) manually deleting an object, (2) passing an object w/o ownership as the original implementation, or (3) passing an object by value Re (1): I'd prefer a scoped pointer to avoid accidental leak. Re (2): This looks a bit awkward to me because other types are passed with ownership. Re (3): This should be feasible, but we need to add more WebCallbacks interfaces for bool type.
On 2015/07/03 06:59:40, nhiroki wrote: > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > Source/bindings/core/v8/CallbackPromiseAdapter.h:257: > m_resolver->resolve(*ownPtr); > On 2015/07/03 06:09:06, haraken wrote: > > On 2015/07/03 06:04:12, yhirano wrote: > > > On 2015/07/03 05:57:49, haraken wrote: > > > > > > > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), which > will > > > > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > > > > > > > hirano-san? > > > > > > > > > > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I > missing > > > anything? > > > > ah, OK. Then what's the merit of using an OwnPtr? It looks a bit strange to > use > > OwnPtr<bool>. > > Sorry, I'm not really sure why you feel this a bit strange... which solution are > you thinking? > > (1) manually deleting an object, > (2) passing an object w/o ownership as the original implementation, or > (3) passing an object by value > > Re (1): I'd prefer a scoped pointer to avoid accidental leak. > Re (2): This looks a bit awkward to me because other types are passed with > ownership. > Re (3): This should be feasible, but we need to add more WebCallbacks interfaces > for bool type. Sorry, I was misunderstanding; you're totally right! LGTM.
lgtm
On 2015/07/03 06:59:40, nhiroki wrote: > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > Source/bindings/core/v8/CallbackPromiseAdapter.h:257: > m_resolver->resolve(*ownPtr); > On 2015/07/03 06:09:06, haraken wrote: > > On 2015/07/03 06:04:12, yhirano wrote: > > > On 2015/07/03 05:57:49, haraken wrote: > > > > > > > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), which > will > > > > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > > > > > > > hirano-san? > > > > > > > > > > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I > missing > > > anything? > > > > ah, OK. Then what's the merit of using an OwnPtr? It looks a bit strange to > use > > OwnPtr<bool>. > > Sorry, I'm not really sure why you feel this a bit strange... which solution are > you thinking? > > (1) manually deleting an object, > (2) passing an object w/o ownership as the original implementation, or > (3) passing an object by value > > Re (1): I'd prefer a scoped pointer to avoid accidental leak. > Re (2): This looks a bit awkward to me because other types are passed with > ownership. > Re (3): This should be feasible, but we need to add more WebCallbacks interfaces > for bool type. For reference would you be able to tell a bit more details about how much more code we need for doing 3? I'm feeling allocating a heap for passing a bool is unfortunate. I understand the current approach gives the most consistency, but would like to weigh the other option a bit if you could help.
nhiroki@chromium.org changed reviewers: + kinuko@chromium.org
On 2015/07/03 09:25:34, kinuko wrote: > On 2015/07/03 06:59:40, nhiroki wrote: > > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > > File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): > > > > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > > Source/bindings/core/v8/CallbackPromiseAdapter.h:257: > > m_resolver->resolve(*ownPtr); > > On 2015/07/03 06:09:06, haraken wrote: > > > On 2015/07/03 06:04:12, yhirano wrote: > > > > On 2015/07/03 05:57:49, haraken wrote: > > > > > > > > > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), which > > will > > > > > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > > > > > > > > > hirano-san? > > > > > > > > > > > > > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I > > missing > > > > anything? > > > > > > ah, OK. Then what's the merit of using an OwnPtr? It looks a bit strange to > > use > > > OwnPtr<bool>. > > > > Sorry, I'm not really sure why you feel this a bit strange... which solution > are > > you thinking? > > > > (1) manually deleting an object, > > (2) passing an object w/o ownership as the original implementation, or > > (3) passing an object by value > > > > Re (1): I'd prefer a scoped pointer to avoid accidental leak. > > Re (2): This looks a bit awkward to me because other types are passed with > > ownership. > > Re (3): This should be feasible, but we need to add more WebCallbacks > interfaces > > for bool type. > > For reference would you be able to tell a bit more details about how much more > code we need for doing 3? I'm feeling allocating a heap for passing a bool is > unfortunate. > I understand the current approach gives the most consistency, but would like to > weigh the other option a bit if you could help. I created CLs for pass-by-value version (3). That would be simpler than I expected. WDYT? (blink) https://codereview.chromium.org/1222103006/ (chromium) https://codereview.chromium.org/1218073019/
On 2015/07/08 07:17:11, nhiroki (ooo) wrote: > On 2015/07/03 09:25:34, kinuko wrote: > > On 2015/07/03 06:59:40, nhiroki wrote: > > > > > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > > > File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): > > > > > > > > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > > > Source/bindings/core/v8/CallbackPromiseAdapter.h:257: > > > m_resolver->resolve(*ownPtr); > > > On 2015/07/03 06:09:06, haraken wrote: > > > > On 2015/07/03 06:04:12, yhirano wrote: > > > > > On 2015/07/03 05:57:49, haraken wrote: > > > > > > > > > > > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), > which > > > will > > > > > > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > > > > > > > > > > > hirano-san? > > > > > > > > > > > > > > > > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I > > > missing > > > > > anything? > > > > > > > > ah, OK. Then what's the merit of using an OwnPtr? It looks a bit strange > to > > > use > > > > OwnPtr<bool>. > > > > > > Sorry, I'm not really sure why you feel this a bit strange... which solution > > are > > > you thinking? > > > > > > (1) manually deleting an object, > > > (2) passing an object w/o ownership as the original implementation, or > > > (3) passing an object by value > > > > > > Re (1): I'd prefer a scoped pointer to avoid accidental leak. > > > Re (2): This looks a bit awkward to me because other types are passed with > > > ownership. > > > Re (3): This should be feasible, but we need to add more WebCallbacks > > interfaces > > > for bool type. > > > > For reference would you be able to tell a bit more details about how much more > > code we need for doing 3? I'm feeling allocating a heap for passing a bool is > > unfortunate. > > I understand the current approach gives the most consistency, but would like > to > > weigh the other option a bit if you could help. > > I created CLs for pass-by-value version (3). That would be simpler than I > expected. WDYT? > > (blink) https://codereview.chromium.org/1222103006/ > (chromium) https://codereview.chromium.org/1218073019/ As discussed offline w/ yhirano@ and kinuko@, yhirano@ suggested another approach make the ownership model in WebCallbacks clearer and he will work on it, so I close this series of CLs. Thank you all.
On 2015/07/08 07:17:11, nhiroki (ooo) wrote: > On 2015/07/03 09:25:34, kinuko wrote: > > On 2015/07/03 06:59:40, nhiroki wrote: > > > > > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > > > File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): > > > > > > > > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > > > Source/bindings/core/v8/CallbackPromiseAdapter.h:257: > > > m_resolver->resolve(*ownPtr); > > > On 2015/07/03 06:09:06, haraken wrote: > > > > On 2015/07/03 06:04:12, yhirano wrote: > > > > > On 2015/07/03 05:57:49, haraken wrote: > > > > > > > > > > > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), > which > > > will > > > > > > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > > > > > > > > > > > hirano-san? > > > > > > > > > > > > > > > > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I > > > missing > > > > > anything? > > > > > > > > ah, OK. Then what's the merit of using an OwnPtr? It looks a bit strange > to > > > use > > > > OwnPtr<bool>. > > > > > > Sorry, I'm not really sure why you feel this a bit strange... which solution > > are > > > you thinking? > > > > > > (1) manually deleting an object, > > > (2) passing an object w/o ownership as the original implementation, or > > > (3) passing an object by value > > > > > > Re (1): I'd prefer a scoped pointer to avoid accidental leak. > > > Re (2): This looks a bit awkward to me because other types are passed with > > > ownership. > > > Re (3): This should be feasible, but we need to add more WebCallbacks > > interfaces > > > for bool type. > > > > For reference would you be able to tell a bit more details about how much more > > code we need for doing 3? I'm feeling allocating a heap for passing a bool is > > unfortunate. > > I understand the current approach gives the most consistency, but would like > to > > weigh the other option a bit if you could help. > > I created CLs for pass-by-value version (3). That would be simpler than I > expected. WDYT? > > (blink) https://codereview.chromium.org/1222103006/ > (chromium) https://codereview.chromium.org/1218073019/ (Follow-up) thanks for exploring the idea. As we discussed offline I feel a better way to address this would be to stop forcing the consumer of WebCallback to use pointers and fixed ownership semantics at the interface level. Ideally it'd be nicer if each consumer can determine what types of value it wants to pass and how the ownership should be managed. If changing the design like that is too cumbersome before repo merge (i.e. until we start to be able to specify ownership semantics using common types) it might be still better to keep it as is for this specific bool cases (but with TODO).
On 2015/07/10 04:18:48, kinuko wrote: > On 2015/07/08 07:17:11, nhiroki (ooo) wrote: > > On 2015/07/03 09:25:34, kinuko wrote: > > > On 2015/07/03 06:59:40, nhiroki wrote: > > > > > > > > > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > > > > File Source/bindings/core/v8/CallbackPromiseAdapter.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1211353004/diff/20001/Source/bindings/core/v8... > > > > Source/bindings/core/v8/CallbackPromiseAdapter.h:257: > > > > m_resolver->resolve(*ownPtr); > > > > On 2015/07/03 06:09:06, haraken wrote: > > > > > On 2015/07/03 06:04:12, yhirano wrote: > > > > > > On 2015/07/03 05:57:49, haraken wrote: > > > > > > > > > > > > > > This will end up with calling resolveOrReject(OwnPtr<bool>, ...), > > which > > > > will > > > > > > > call toV8(OwnPtr<bool>, ...). This seems not quite right to me. > > > > > > > > > > > > > > hirano-san? > > > > > > > > > > > > > > > > > > > This statement passes |*ownPtr| of type |bool|, so it looks good. Am I > > > > missing > > > > > > anything? > > > > > > > > > > ah, OK. Then what's the merit of using an OwnPtr? It looks a bit strange > > to > > > > use > > > > > OwnPtr<bool>. > > > > > > > > Sorry, I'm not really sure why you feel this a bit strange... which > solution > > > are > > > > you thinking? > > > > > > > > (1) manually deleting an object, > > > > (2) passing an object w/o ownership as the original implementation, or > > > > (3) passing an object by value > > > > > > > > Re (1): I'd prefer a scoped pointer to avoid accidental leak. > > > > Re (2): This looks a bit awkward to me because other types are passed with > > > > ownership. > > > > Re (3): This should be feasible, but we need to add more WebCallbacks > > > interfaces > > > > for bool type. > > > > > > For reference would you be able to tell a bit more details about how much > more > > > code we need for doing 3? I'm feeling allocating a heap for passing a bool > is > > > unfortunate. > > > I understand the current approach gives the most consistency, but would like > > to > > > weigh the other option a bit if you could help. > > > > I created CLs for pass-by-value version (3). That would be simpler than I > > expected. WDYT? > > > > (blink) https://codereview.chromium.org/1222103006/ > > (chromium) https://codereview.chromium.org/1218073019/ > > (Follow-up) thanks for exploring the idea. As we discussed offline I feel a > better way to address this would be to stop forcing the consumer of WebCallback > to use pointers and fixed ownership semantics at the interface level. Ideally > it'd be nicer if each consumer can determine what types of value it wants to > pass and how the ownership should be managed. If changing the design like that > is too cumbersome before repo merge (i.e. until we start to be able to specify > ownership semantics using common types) it might be still better to keep it as > is for this specific bool cases (but with TODO). (Oops, missed nhiroki's follow-up response before sending this) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
