Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2005753006:
ImageCapture: ScopedResultCallback (Closed)
|
Created:
4 years, 7 months ago by mcasas Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImageCapture: ScopedResultCallback
ImageCapture mojo command servicing in the Browser
needs to jump several threads (UI --> IO --> Device(s)).
On ToT, each possible error condition in this chain is
treated more or less individually, which is ugly, (especially
as I'm adding capability retrieval/settings in // CLs).
This CL proposes ScopedResultCallback, which is a
move-only type wrapping a Mojo |callback_| _and_ an
associated |error_callback_|, which is a callback provided
to signal to Blink-side that something went wrong. Using
and passing this callback along the callstack allows for:
- removing the boolean return value
- not having to run the callback by hand: If and when
the methods/functions receiving this ScopedCallback
don't Run() it nor pass it explicitly, it would be
tantamount to "rejecting" it.
For reference, the callstack follows a sequence:
ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation).
BUG=518807
Committed: https://crrev.com/033172cfd48f0c1d634324a38bf75711572cfb13
Cr-Commit-Position: refs/heads/master@{#397182}
Patch Set 1 #
Total comments: 10
Patch Set 2 : reillyg@ comments #
Total comments: 4
Patch Set 3 : Android and threading #
Total comments: 12
Patch Set 4 : reillyg@ second round of comments #
Total comments: 4
Patch Set 5 : miu@s comments #
Total comments: 16
Patch Set 6 : miu@s comments (minus renaming) #Patch Set 7 : s/ScopedCallback/ScopedResultCallback/ #
Total comments: 2
Patch Set 8 : rockot@s nit #Messages
Total messages: 53 (27 generated)
Description was changed from ========== ScopedMojoCallback BUG=518807 ========== to ========== ScopedMojoCallback ImageCapture mojo commands need to jump several threads before they are serviced. On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, specially as I'm adding capability retrieval/settings in // CLs. Instead, this CL proposes ScopedMojoCallback, which is a moveable only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a Binding on the former that is used to signify to Blink-side that something went wrong. Using and passing this callback along the way allows for removing the boolean return + having to run the callback by hand. Now if the methods/functions receiving this ScopedMojoCallback don't run it or pass it explicitly, it would be tantamount to "rejecting" it. BUG=518807 ==========
Description was changed from ========== ScopedMojoCallback ImageCapture mojo commands need to jump several threads before they are serviced. On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, specially as I'm adding capability retrieval/settings in // CLs. Instead, this CL proposes ScopedMojoCallback, which is a moveable only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a Binding on the former that is used to signify to Blink-side that something went wrong. Using and passing this callback along the way allows for removing the boolean return + having to run the callback by hand. Now if the methods/functions receiving this ScopedMojoCallback don't run it or pass it explicitly, it would be tantamount to "rejecting" it. BUG=518807 ========== to ========== ScopedMojoCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedMojoCallback, which is a move-only type wrapping a Mojo|callback_| _and_ an associated |error_callback_|, which is a Binding of the former used to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand. If and when the methods/functions receiving this ScopedMojoCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
mcasas@chromium.org changed reviewers: + reillyg@chromium.org
reillyg@ can you please take a first look? FTR, the call sequence is: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation).
Description was changed from ========== ScopedMojoCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedMojoCallback, which is a move-only type wrapping a Mojo|callback_| _and_ an associated |error_callback_|, which is a Binding of the former used to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand. If and when the methods/functions receiving this ScopedMojoCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ========== to ========== ImageCapture: ScopedMojoCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedMojoCallback, which is a move-only type wrapping a Mojo|callback_| _and_ an associated |error_callback_|, which is a Binding of the former used to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand. If and when the methods/functions receiving this ScopedMojoCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ==========
https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:150: new ScopedMojoCallback<TakePhotoCallback>(callback)); Something is wrong here. Shouldn't it be ScopedMojoCallback<TakePhotoCallback>*? |cb| doesn't have a get() method so the line below shouldn't compile. https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... File media/capture/video/scoped_mojo_callback.h (right): https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... media/capture/video/scoped_mojo_callback.h:18: class ScopedMojoCallback { Technically, this could be called |ScopedCallback| since it doesn't actually assume that |callback| is a mojo::Callback. It would work just fine for a base::Callback as well. https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... media/capture/video/scoped_mojo_callback.h:22: ScopedMojoCallback(std::unique_ptr<CallbackType> callback, I would make this parameter just const CallbackType& callback as it will make calling the constructor simpler. Since callers are just copying the callback into a heap allocated one anyways it doesn't really add any safety. https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... media/capture/video/scoped_mojo_callback.h:23: const base::Closure& on_error_callback) I would mirror what ScopedWebCallbacks does and make this a base::Callback<void(const CallbackType&)> so that this object isn't carrying around two copies of |callback_|. https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... media/capture/video/scoped_mojo_callback.h:42: std::unique_ptr<CallbackType> callback_; Just make this CallbackType and call reset() in PassCallback() to null the callback.
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
reillyg@ PTAL (VCDAndroid still untested) https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:150: new ScopedMojoCallback<TakePhotoCallback>(callback)); On 2016/05/25 18:32:57, Reilly Grant wrote: > Something is wrong here. Shouldn't it be ScopedMojoCallback<TakePhotoCallback>*? > |cb| doesn't have a get() method so the line below shouldn't compile. Yeah, I'm away from my Android-compiling rig, so wasn't really trying to get this running, yet :) https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... File media/capture/video/scoped_mojo_callback.h (right): https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... media/capture/video/scoped_mojo_callback.h:18: class ScopedMojoCallback { On 2016/05/25 18:32:57, Reilly Grant wrote: > Technically, this could be called |ScopedCallback| since it doesn't actually > assume that |callback| is a mojo::Callback. It would work just fine for a > base::Callback as well. Renamed. https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... media/capture/video/scoped_mojo_callback.h:22: ScopedMojoCallback(std::unique_ptr<CallbackType> callback, On 2016/05/25 18:32:57, Reilly Grant wrote: > I would make this parameter just const CallbackType& callback as it will make > calling the constructor simpler. Since callers are just copying the callback > into a heap allocated one anyways it doesn't really add any safety. Done. https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... media/capture/video/scoped_mojo_callback.h:23: const base::Closure& on_error_callback) On 2016/05/25 18:32:57, Reilly Grant wrote: > I would mirror what ScopedWebCallbacks does and make this a > base::Callback<void(const CallbackType&)> so that this object isn't carrying > around two copies of |callback_|. Good idea, done. https://codereview.chromium.org/2005753006/diff/60001/media/capture/video/sco... media/capture/video/scoped_mojo_callback.h:42: std::unique_ptr<CallbackType> callback_; On 2016/05/25 18:32:57, Reilly Grant wrote: > Just make this CallbackType and call reset() in PassCallback() to null the > callback. Done.
ScopedCallback looks good (with nits) but this patch needs more testing before I'll sign off on it. https://codereview.chromium.org/2005753006/diff/100001/media/capture/video/an... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2005753006/diff/100001/media/capture/video/an... media/capture/video/android/video_capture_device_android.cc:151: const intptr_t callback_id = reinterpret_cast<intptr_t>(&cb); &cb is a stack pointer. You can't pass it to Java. https://codereview.chromium.org/2005753006/diff/100001/media/capture/video/sc... File media/capture/video/scoped_callback.h (right): https://codereview.chromium.org/2005753006/diff/100001/media/capture/video/sc... media/capture/video/scoped_callback.h:24: : callback_(std::move(callback)), on_error_callback_(on_error_callback) {} nit: The move here and below are unnecessary.
Description was changed from ========== ImageCapture: ScopedMojoCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedMojoCallback, which is a move-only type wrapping a Mojo|callback_| _and_ an associated |error_callback_|, which is a Binding of the former used to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand. If and when the methods/functions receiving this ScopedMojoCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ========== to ========== ImageCapture: ScopedCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ==========
reillyg@ now the Android part is working, and had to fine tune the threading in image_capture_impl.cc, PTAL again https://codereview.chromium.org/2005753006/diff/100001/media/capture/video/an... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2005753006/diff/100001/media/capture/video/an... media/capture/video/android/video_capture_device_android.cc:151: const intptr_t callback_id = reinterpret_cast<intptr_t>(&cb); On 2016/05/26 00:27:45, Reilly Grant wrote: > &cb is a stack pointer. You can't pass it to Java. Yeah, all this code was essentially untouched, I should have been more clear. https://codereview.chromium.org/2005753006/diff/100001/media/capture/video/sc... File media/capture/video/scoped_callback.h (right): https://codereview.chromium.org/2005753006/diff/100001/media/capture/video/sc... media/capture/video/scoped_callback.h:24: : callback_(std::move(callback)), on_error_callback_(on_error_callback) {} On 2016/05/26 00:27:45, Reilly Grant wrote: > nit: The move here and below are unnecessary. Done.
Patchset #3 (id:120001) has been deleted
lgtm with nits https://codereview.chromium.org/2005753006/diff/140001/content/browser/media/... File content/browser/media/capture/image_capture_impl.cc (right): https://codereview.chromium.org/2005753006/diff/140001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:82: std::move(callback), nit: Unnecessary move. https://codereview.chromium.org/2005753006/diff/140001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:99: base::Passed(std::move(scoped_callback)))); base::Passed(&scoped_callback), it's shorter. https://codereview.chromium.org/2005753006/diff/140001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2005753006/diff/140001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:845: media::ScopedCallback<media::VideoCaptureDevice::TakePhotoCallback> cb) { nit: s/cb/callback/ https://codereview.chromium.org/2005753006/diff/140001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:859: base::Passed(std::move(cb)))); base::Passed(&cb) https://codereview.chromium.org/2005753006/diff/140001/media/capture/video/an... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2005753006/diff/140001/media/capture/video/an... media/capture/video/android/video_capture_device_android.cc:149: std::unique_ptr<ScopedCallback<TakePhotoCallback>> cb( nit: s/cb/heap_callback/ https://codereview.chromium.org/2005753006/diff/140001/media/capture/video/fa... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2005753006/diff/140001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:169: base::Bind(&DoTakeFakePhoto, base::Passed(std::move(callback)), nit: base::Passed(&callback)
mcasas@chromium.org changed reviewers: + miu@chromium.org
miu@ PTAL (especially at image_capture_impl.cc and scoped_callback.h) https://codereview.chromium.org/2005753006/diff/140001/content/browser/media/... File content/browser/media/capture/image_capture_impl.cc (right): https://codereview.chromium.org/2005753006/diff/140001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:82: std::move(callback), On 2016/05/26 17:50:03, Reilly Grant wrote: > nit: Unnecessary move. Done. https://codereview.chromium.org/2005753006/diff/140001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:99: base::Passed(std::move(scoped_callback)))); On 2016/05/26 17:50:03, Reilly Grant wrote: > base::Passed(&scoped_callback), it's shorter. Done. https://codereview.chromium.org/2005753006/diff/140001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2005753006/diff/140001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:845: media::ScopedCallback<media::VideoCaptureDevice::TakePhotoCallback> cb) { On 2016/05/26 17:50:03, Reilly Grant wrote: > nit: s/cb/callback/ Done. https://codereview.chromium.org/2005753006/diff/140001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:859: base::Passed(std::move(cb)))); On 2016/05/26 17:50:03, Reilly Grant wrote: > base::Passed(&cb) Done. https://codereview.chromium.org/2005753006/diff/140001/media/capture/video/an... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2005753006/diff/140001/media/capture/video/an... media/capture/video/android/video_capture_device_android.cc:149: std::unique_ptr<ScopedCallback<TakePhotoCallback>> cb( On 2016/05/26 17:50:04, Reilly Grant wrote: > nit: s/cb/heap_callback/ Done. https://codereview.chromium.org/2005753006/diff/140001/media/capture/video/fa... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2005753006/diff/140001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:169: base::Bind(&DoTakeFakePhoto, base::Passed(std::move(callback)), On 2016/05/26 17:50:04, Reilly Grant wrote: > nit: base::Passed(&callback) Done.
Initial comments on Patch Set 4: https://codereview.chromium.org/2005753006/diff/160001/content/browser/media/... File content/browser/media/capture/image_capture_impl.cc (right): https://codereview.chromium.org/2005753006/diff/160001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:33: if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { Instead of trampolining, can you just use media::BindToCurrentLoop() on L91 below? https://codereview.chromium.org/2005753006/diff/160001/media/capture/video/sc... File media/capture/video/scoped_callback.h (right): https://codereview.chromium.org/2005753006/diff/160001/media/capture/video/sc... media/capture/video/scoped_callback.h:17: class ScopedCallback { I think this exists already (in base/callback_helpers.h).
miu@ PTAL https://codereview.chromium.org/2005753006/diff/160001/content/browser/media/... File content/browser/media/capture/image_capture_impl.cc (right): https://codereview.chromium.org/2005753006/diff/160001/content/browser/media/... content/browser/media/capture/image_capture_impl.cc:33: if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { On 2016/05/27 22:36:38, miu wrote: > Instead of trampolining, can you just use media::BindToCurrentLoop() on L91 > below? media::BindToCurrentLoop() does not know how to forward mojo::Array<> etc, so until that time comes, can't do that here -- unlike the other RunFailed...Callback() functions, which do use it. Actually, on second thoughts this function would never be called on UI thread since we already have at least one jump to IO. So I simplified it a bit. https://codereview.chromium.org/2005753006/diff/160001/media/capture/video/sc... File media/capture/video/scoped_callback.h (right): https://codereview.chromium.org/2005753006/diff/160001/media/capture/video/sc... media/capture/video/scoped_callback.h:17: class ScopedCallback { On 2016/05/27 22:36:38, miu wrote: > I think this exists already (in base/callback_helpers.h). Partially. There is ScopedClosureRunner(), which will call the closure it wraps when it gets destroyed, whereas ScopedCallback will call the ctor OnErrorCallback when it's destructed. The rationale here is that I'd like to create a ScopedCallback as soon as the mojo 'command' is 'serviced' and use is as a move-only type so when it's destroyed, the said OnErrorCallback will be called, unless the mojo callback has been taken out (with PassCallback()). It basically implies that there are two ways to resolve a mojo callback (success/failure), their semantics differing by their arguments.
I like this idea. This utility would be useful in a number of situations I've come across recently. Maybe in the future, if this gains traction in the media code, it would be proven worthy of moving into src/base? Comments on Patch Set 5: https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/DEPS File media/capture/video/DEPS (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/DE... media/capture/video/DEPS:2: "+third_party/WebKit/public/platform/modules/imagecapture", Is this legal? I thought only src/media/blink could depend on WebKit code, and only then, the publicly exported interfaces. https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... File media/capture/video/scoped_callback.h (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:12: // This class wraps a |callback_| and guarantees that it will be called on This is a little inaccurate. It will guarantee that either |callback_| has been called, or will pass the callback to |on_error_callback_| at destruction time for "default handling." https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:17: class ScopedCallback { For me, the name of this utility class was a bit confusing (e.g., I was wondering if you had re-invented base::ScopedClosureRunner or something like that). In fact, it's very close to "promises" in JavaScript. How about naming this something like ResultPromise or ResultCallbackPair? https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:28: on_error_callback_.Run(callback_); Just in case that |callback_| has a ref-counted pointer in it: on_error_callback_.Run(base::ResetAndReturn(&callback_)); https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:39: CallbackType PassCallback() { A few things: 1. Instead of this method, would client code become a bit simpler if this were like base::Callback<>::Run()? 2. Perhaps |on_error_callback_| should be reset before the success callback is run. 3. nit: Use base::ResetAndReturn(). Putting it all together: template<typename... Args> void Success(Args... args) { on_error_callback_.Reset(); base::ResetAndReturn(&callback_).Run(args); } https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/vi... media/capture/video/video_capture_device.h:317: using TakePhotoCallback = blink::mojom::ImageCapture::TakePhotoCallback; ditto: Is it legal to introduce blink dependencies to src/media interfaces?
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
miu@ PTAL https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/DEPS File media/capture/video/DEPS (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/DE... media/capture/video/DEPS:2: "+third_party/WebKit/public/platform/modules/imagecapture", On 2016/05/28 01:29:56, miu wrote: > Is this legal? I thought only src/media/blink could depend on WebKit code, and > only then, the publicly exported interfaces. Hmm please see my reply to your other similar comment in video_capture_device.h. https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... File media/capture/video/scoped_callback.h (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:12: // This class wraps a |callback_| and guarantees that it will be called on On 2016/05/28 01:29:56, miu wrote: > This is a little inaccurate. It will guarantee that either |callback_| has been > called, or will pass the callback to |on_error_callback_| at destruction time > for "default handling." Rewritten. https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:17: class ScopedCallback { On 2016/05/28 01:29:56, miu wrote: > For me, the name of this utility class was a bit confusing (e.g., I was > wondering if you had re-invented base::ScopedClosureRunner or something like > that). If this class was to get traction, I think its base/ location could indeed be next to ScopedClosureRunner in base/callback_helpers.h. > In fact, it's very close to "promises" in JavaScript. It is, except Promises have explicit resolve() and reject() methods, and there are already nice wrappers for those, e.g.ScopedWebCallbacks [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > How about naming this something like ResultPromise or > ResultCallbackPair? I added the Scoped part to make evident that this class is move-only, as opposed to mojo::Callback which is ref counted under the hood, making it hard to reason about its cancellation (who should cancel it, on which thread etc). I think the key elements are: - this class is move-only - forces either the callback or the error callback to be Run() - either of those can only be called _once_ (whereas {base,mojo}::Callback can be Run() several times). - the OnErrorCallback operates on the same very callback that this class wraps! ScopedErrorOrCallback ? CallbackErrorPair? https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:28: on_error_callback_.Run(callback_); On 2016/05/28 01:29:56, miu wrote: > Just in case that |callback_| has a ref-counted pointer in it: > > on_error_callback_.Run(base::ResetAndReturn(&callback_)); Done. (Note that base::ResetAndReturn() doesn't work on mojo::Callback<>). https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:39: CallbackType PassCallback() { On 2016/05/28 01:29:56, miu wrote: > A few things: > > 1. Instead of this method, would client code become a bit simpler if this were > like base::Callback<>::Run()? > > 2. Perhaps |on_error_callback_| should be reset before the success callback is > run. > > 3. nit: Use base::ResetAndReturn(). > > Putting it all together: > > template<typename... Args> > void Success(Args... args) { > on_error_callback_.Reset(); > base::ResetAndReturn(&callback_).Run(args); > } Taken all in except the base::ResetAndReturn() for the reason mentioned above. The glue logic had to be a bit more complex to deal with move-only types. https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/vi... media/capture/video/video_capture_device.h:317: using TakePhotoCallback = blink::mojom::ImageCapture::TakePhotoCallback; On 2016/05/28 01:29:56, miu wrote: > ditto: Is it legal to introduce blink dependencies to src/media interfaces? For the purpose of this CL, I can (and will) reintroduce the previous |using TakePhotoCallback| syntax, so the dependency to blink is no more, while I talk to the appropriate people. The problem/question of these Classes needing Blink type will surface again when implementing getPhotoCapabilities sincethis one uses a mojo struct pointer as return type, and it'll be silly to provide duplicate structures here just to avoid cleaning up the dependency graph. Note that I'm still needing to use mojo types here, but that shouldn't be a problem, i.e. I understood that, eventually, mojo would be a pervasive set of services, ubiquitous like base::. SG?
Patchset #6 (id:240001) has been deleted
Patchset #6 (id:260001) has been deleted
lgtm % nit: https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... File media/capture/video/scoped_callback.h (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:17: class ScopedCallback { On 2016/05/31 20:19:04, mcasas wrote: > On 2016/05/28 01:29:56, miu wrote: > > For me, the name of this utility class was a bit confusing (e.g., I was > > wondering if you had re-invented base::ScopedClosureRunner or something like > > that). > > ScopedErrorOrCallback ? > CallbackErrorPair? nit: ScopedResultCallback? (since this mechanism is for guaranteeing either a result callback is run, or it is passed to an error handler when the operation aborts/fails) https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:39: CallbackType PassCallback() { On 2016/05/31 20:19:04, mcasas wrote: > Taken all in except the base::ResetAndReturn() for the > reason mentioned above. The glue logic had to be a bit > more complex to deal with move-only types. Understood. https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/vi... media/capture/video/video_capture_device.h:317: using TakePhotoCallback = blink::mojom::ImageCapture::TakePhotoCallback; On 2016/05/31 20:19:05, mcasas wrote: > On 2016/05/28 01:29:56, miu wrote: > > ditto: Is it legal to introduce blink dependencies to src/media interfaces? > > For the purpose of this CL, I can (and will) reintroduce > the previous |using TakePhotoCallback| syntax, so > the dependency to blink is no more, while I talk to > the appropriate people. The problem/question of these > Classes needing Blink type will surface again when > implementing getPhotoCapabilities sincethis one uses a > mojo struct pointer as return type, and it'll be silly to > provide duplicate structures here just to avoid cleaning > up the dependency graph. > > Note that I'm still needing to use mojo types here, > but that shouldn't be a problem, i.e. I understood > that, eventually, mojo would be a pervasive set of > services, ubiquitous like base::. SG? Acknowledged.
Description was changed from ========== ImageCapture: ScopedCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ========== to ========== ImageCapture: ScopedReturnCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedReturnCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ==========
Description was changed from ========== ImageCapture: ScopedReturnCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedReturnCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ========== to ========== ImageCapture: ScopedReturnCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedReturnCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ==========
mcasas@chromium.org changed reviewers: + xhwang@chromium.org
xhwang@ plz RS for trivial media/capture.gypi addition. https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... File media/capture/video/scoped_callback.h (right): https://codereview.chromium.org/2005753006/diff/180001/media/capture/video/sc... media/capture/video/scoped_callback.h:17: class ScopedCallback { On 2016/06/01 00:53:36, miu wrote: > On 2016/05/31 20:19:04, mcasas wrote: > > On 2016/05/28 01:29:56, miu wrote: > > > For me, the name of this utility class was a bit confusing (e.g., I was > > > wondering if you had re-invented base::ScopedClosureRunner or something like > > > that). > > > > ScopedErrorOrCallback ? > > CallbackErrorPair? > > nit: ScopedResultCallback? (since this mechanism is for guaranteeing either a > result callback is run, or it is passed to an error handler when the operation > aborts/fails) ScopedResultCallback sounds great.
rs lgtm
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2005753006/#ps300001 (title: "s/ScopedCallback/ScopedResultCallback/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005753006/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2005753006/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mcasas@chromium.org changed reviewers: + rockot@chromium.org
rockot@ OWNERS RS for addition to a DEPS:
** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
'+mojo/public/cpp/bindings',
lgtm but I have an inline nit as a non-OWNER https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... File media/capture/video/scoped_result_callback.h (right): https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... media/capture/video/scoped_result_callback.h:39: on_error_callback_.Run(ResetAndReturn(&callback_)); IMHO it's kind of unfortunate to introduce ResetAndReturn in this header, even if it's in an anonymous namespace. But you don't need to. It's redundant and unnecessary to use here (callback_ cannot possibly re-enter this object unless something else is broken, so you don't need to reset it). The only necessary callsite is inside Run(), and you could just inline the logic there instead.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, xhwang@chromium.org, reillyg@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2005753006/#ps320001 (title: "rockot@s nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005753006/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2005753006/320001
Forgot to mention, might want to update the CL description (ScopedReturnCallback?)
rockot@: the CL description was changed to ScopedReturnCallback a while back but the emails being sent did not get an updated Subject line... :? https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... File media/capture/video/scoped_result_callback.h (right): https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... media/capture/video/scoped_result_callback.h:39: on_error_callback_.Run(ResetAndReturn(&callback_)); On 2016/06/01 06:23:52, Ken Rockot wrote: > IMHO it's kind of unfortunate to introduce ResetAndReturn in this header, even > if it's in an anonymous namespace. But you don't need to. It's redundant and > unnecessary to use here (callback_ cannot possibly re-enter this object unless > something else is broken, so you don't need to reset it). The only necessary > callsite is inside Run(), and you could just inline the logic there instead. Done.
On 2016/06/01 17:12:08, mcasas wrote: > rockot@: the CL description was changed > to ScopedReturnCallback a while back but > the emails being sent did not get an > updated Subject line... :? > > https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... > File media/capture/video/scoped_result_callback.h (right): > > https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... > media/capture/video/scoped_result_callback.h:39: > on_error_callback_.Run(ResetAndReturn(&callback_)); > On 2016/06/01 06:23:52, Ken Rockot wrote: > > IMHO it's kind of unfortunate to introduce ResetAndReturn in this header, even > > if it's in an anonymous namespace. But you don't need to. It's redundant and > > unnecessary to use here (callback_ cannot possibly re-enter this object unless > > something else is broken, so you don't need to reset it). The only necessary > > callsite is inside Run(), and you could just inline the logic there instead. > > Done. Yeah, it appears the email title never change, may be such that we can keep them in the same thread.
On 2016/06/01 at 17:19:39, xhwang wrote: > On 2016/06/01 17:12:08, mcasas wrote: > > rockot@: the CL description was changed > > to ScopedReturnCallback a while back but > > the emails being sent did not get an > > updated Subject line... :? > > > > https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... > > File media/capture/video/scoped_result_callback.h (right): > > > > https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... > > media/capture/video/scoped_result_callback.h:39: > > on_error_callback_.Run(ResetAndReturn(&callback_)); > > On 2016/06/01 06:23:52, Ken Rockot wrote: > > > IMHO it's kind of unfortunate to introduce ResetAndReturn in this header, even > > > if it's in an anonymous namespace. But you don't need to. It's redundant and > > > unnecessary to use here (callback_ cannot possibly re-enter this object unless > > > something else is broken, so you don't need to reset it). The only necessary > > > callsite is inside Run(), and you could just inline the logic there instead. > > > > Done. > > Yeah, it appears the email title never change, may be such that we can keep them in the same thread. No, I know that behavior is normal. What I mean is the description says ScopedReturnCallback, but the type is named ScopedResultCallback.
Description was changed from ========== ImageCapture: ScopedReturnCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedReturnCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ========== to ========== ImageCapture: ScopedResultCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedResultCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ==========
On 2016/06/01 17:32:01, Ken Rockot wrote: > On 2016/06/01 at 17:19:39, xhwang wrote: > > On 2016/06/01 17:12:08, mcasas wrote: > > > rockot@: the CL description was changed > > > to ScopedReturnCallback a while back but > > > the emails being sent did not get an > > > updated Subject line... :? > > > > > > > https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... > > > File media/capture/video/scoped_result_callback.h (right): > > > > > > > https://codereview.chromium.org/2005753006/diff/300001/media/capture/video/sc... > > > media/capture/video/scoped_result_callback.h:39: > > > on_error_callback_.Run(ResetAndReturn(&callback_)); > > > On 2016/06/01 06:23:52, Ken Rockot wrote: > > > > IMHO it's kind of unfortunate to introduce ResetAndReturn in this header, > even > > > > if it's in an anonymous namespace. But you don't need to. It's redundant > and > > > > unnecessary to use here (callback_ cannot possibly re-enter this object > unless > > > > something else is broken, so you don't need to reset it). The only > necessary > > > > callsite is inside Run(), and you could just inline the logic there > instead. > > > > > > Done. > > > > Yeah, it appears the email title never change, may be such that we can keep > them in the same thread. > > No, I know that behavior is normal. What I mean is the description says > ScopedReturnCallback, but the type is named ScopedResultCallback. Oeps, renamed.
Message was sent while issue was closed.
Description was changed from ========== ImageCapture: ScopedResultCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedResultCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ========== to ========== ImageCapture: ScopedResultCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedResultCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== ImageCapture: ScopedResultCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedResultCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 ========== to ========== ImageCapture: ScopedResultCallback ImageCapture mojo command servicing in the Browser needs to jump several threads (UI --> IO --> Device(s)). On ToT, each possible error condition in this chain is treated more or less individually, which is ugly, (especially as I'm adding capability retrieval/settings in // CLs). This CL proposes ScopedResultCallback, which is a move-only type wrapping a Mojo |callback_| _and_ an associated |error_callback_|, which is a callback provided to signal to Blink-side that something went wrong. Using and passing this callback along the callstack allows for: - removing the boolean return value - not having to run the callback by hand: If and when the methods/functions receiving this ScopedCallback don't Run() it nor pass it explicitly, it would be tantamount to "rejecting" it. For reference, the callstack follows a sequence: ImageCaptureImpl --> VideoCaptureManager --> VideoCaptureDevice (implementation). BUG=518807 Committed: https://crrev.com/033172cfd48f0c1d634324a38bf75711572cfb13 Cr-Commit-Position: refs/heads/master@{#397182} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/033172cfd48f0c1d634324a38bf75711572cfb13 Cr-Commit-Position: refs/heads/master@{#397182} |
