Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2735903002:
Fix OnceCallback example in docs/callback.md (Closed)
DescriptionFix OnceCallback example in docs/callback.md
Fixes the original example, which resulted in an invalid conversion from
base::OnceCallback<void(int)> to base::Closure. Also adds a comment that
this usage is not yet supported.
BUG=none
R=tzik@chromium.org
Review-Url: https://codereview.chromium.org/2735903002
Cr-Commit-Position: refs/heads/master@{#456886}
Committed: https://chromium.googlesource.com/chromium/src/+/126f704d1c44627098d024123ade6466bde7825f
Patch Set 1 #Patch Set 2 : fix? #Patch Set 3 : tzik #
Total comments: 2
Patch Set 4 : BindOnce #Messages
Total messages: 19 (9 generated)
Description was changed from ========== Fix OnceCallback example in docs/callback.md BUG=none R=tzik@chromium.org ========== to ========== Fix OnceCallback example in docs/callback.md The original example results in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. BUG=none R=tzik@chromium.org ==========
tzik: This is based on an actual error I'm encountering. Is my explanation in the CL description right, and is the new example what you were going for?
Description was changed from ========== Fix OnceCallback example in docs/callback.md The original example results in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. BUG=none R=tzik@chromium.org ========== to ========== Fix OnceCallback example in docs/callback.md The original example results in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. BUG=none R=tzik@chromium.org COMMIT=false ==========
On 2017/03/07 01:01:36, michaelpg wrote: > tzik: This is based on an actual error I'm encountering. Is my explanation in > the CL description right, and is the new example what you were going for? Hrm, this won't compile either. I found this example: https://cs.chromium.org/chromium/src/media/base/bind_to_current_loop.h?q=post... which uses base::Passed and a helper function to actually run the callback; is that the current "best practice" for delaying a call to a OnceCallback?
On 2017/03/07 01:26:59, michaelpg wrote: > On 2017/03/07 01:01:36, michaelpg wrote: > > tzik: This is based on an actual error I'm encountering. Is my explanation in > > the CL description right, and is the new example what you were going for? > > Hrm, this won't compile either. I found this example: > > https://cs.chromium.org/chromium/src/media/base/bind_to_current_loop.h?q=post... > > which uses base::Passed and a helper function to actually run the callback; is > that the current "best practice" for delaying a call to a OnceCallback? Sorry for delay. TaskRunner::PostTask does not support OnceCallback until http://crrev.com/2637843002 lands. And BindToCurrentLoop case is a hack to run it forcibly. # This was needed since BindToCurrentLoop was a blocker of TaskRunner::PostTask migration. Can we update the document as PS1 with BindOnce and a comment about TaskRunner::PostTask state? E.g. "TaskRunner does not yet support OnceClosure. Once it completes the migration, OnceClosure can be posted as below."
Description was changed from ========== Fix OnceCallback example in docs/callback.md The original example results in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. BUG=none R=tzik@chromium.org COMMIT=false ========== to ========== Fix OnceCallback example in docs/callback.md Fixes the original example, which resulted in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. Also adds a comment that this usage is not yet supported. BUG=none R=tzik@chromium.org COMMIT=false ==========
On 2017/03/13 06:40:38, tzik wrote: > On 2017/03/07 01:26:59, michaelpg wrote: > > On 2017/03/07 01:01:36, michaelpg wrote: > > > tzik: This is based on an actual error I'm encountering. Is my explanation > in > > > the CL description right, and is the new example what you were going for? > > > > Hrm, this won't compile either. I found this example: > > > > > https://cs.chromium.org/chromium/src/media/base/bind_to_current_loop.h?q=post... > > > > which uses base::Passed and a helper function to actually run the callback; is > > that the current "best practice" for delaying a call to a OnceCallback? > > Sorry for delay. > TaskRunner::PostTask does not support OnceCallback until > http://crrev.com/2637843002 lands. Ah, so this CL is a fool's errand :-) > And BindToCurrentLoop case is a hack to run it forcibly. > # This was needed since BindToCurrentLoop was a blocker of TaskRunner::PostTask > migration. > > Can we update the document as PS1 with BindOnce and a comment about > TaskRunner::PostTask state? E.g. "TaskRunner does not yet support OnceClosure. > Once it completes the migration, OnceClosure can be posted as below." Done, I think, PTAL.
Thanks! https://codereview.chromium.org/2735903002/diff/40001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2735903002/diff/40001/docs/callback.md#newcode73 docs/callback.md:73: PostTask(FROM_HERE, base::Bind(std::move(cb), 42)); // not yet implemented! Could you use BindOnce instead of Bind?
https://codereview.chromium.org/2735903002/diff/40001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2735903002/diff/40001/docs/callback.md#newcode73 docs/callback.md:73: PostTask(FROM_HERE, base::Bind(std::move(cb), 42)); // not yet implemented! On 2017/03/14 03:25:06, tzik wrote: > Could you use BindOnce instead of Bind? yeah, that would make sense, wouldn't it... done
lgtm
The CQ bit was checked by michaelpg@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
COMMIT=false detected. CQ is abandoning the patch.
Description was changed from ========== Fix OnceCallback example in docs/callback.md Fixes the original example, which resulted in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. Also adds a comment that this usage is not yet supported. BUG=none R=tzik@chromium.org COMMIT=false ========== to ========== Fix OnceCallback example in docs/callback.md Fixes the original example, which resulted in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. Also adds a comment that this usage is not yet supported. BUG=none R=tzik@chromium.org ==========
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489533032202370,
"parent_rev": "cae62987517d4bb239dc860d906cb43c65a7383a", "commit_rev":
"126f704d1c44627098d024123ade6466bde7825f"}
Message was sent while issue was closed.
Description was changed from ========== Fix OnceCallback example in docs/callback.md Fixes the original example, which resulted in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. Also adds a comment that this usage is not yet supported. BUG=none R=tzik@chromium.org ========== to ========== Fix OnceCallback example in docs/callback.md Fixes the original example, which resulted in an invalid conversion from base::OnceCallback<void(int)> to base::Closure. Also adds a comment that this usage is not yet supported. BUG=none R=tzik@chromium.org Review-Url: https://codereview.chromium.org/2735903002 Cr-Commit-Position: refs/heads/master@{#456886} Committed: https://chromium.googlesource.com/chromium/src/+/126f704d1c44627098d024123ade... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/126f704d1c44627098d024123ade... |
