Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(173)

Issue 2867713002: Use OnceCallback on Mojo interfaces in //chrome/common/media_router (Closed)

Created:
3 years, 7 months ago by tzik
Modified:
3 years, 6 months ago
CC:
Aaron Boodman, abarth-chromium, chfremer+watch_chromium.org, chromium-reviews, darin (slow to review), feature-media-reviews_chromium.org, imcheng+watch_chromium.org, mfoltz+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, zhaobin+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use OnceCallback on Mojo interfaces in //chrome/common/media_router This CL flips `use_once_calback` flag on the Mojo code generator, and fixes all compile errors after it. After this CL, Mojo interfaces in //chrome/common/media_router starts using base::OnceCallback instead of base::Callback on its return value handling. The migration recipe was: - Convert pass-by-ref callback objects to pass-by-value. - Use std::move() to forward it to other consumer, or to invoke it with Callback::Run(). - Handle wherever copies are required manually. - Check if the conversion doesn't change the semantics. As the transfer and invocation clobber the callback object, care about use-after-move. It's considered safe to consume almost scoped-out callback. BUG=714018 Review-Url: https://codereview.chromium.org/2867713002 Cr-Commit-Position: refs/heads/master@{#475449} Committed: https://chromium.googlesource.com/chromium/src/+/318acc6c2bc98fa5f872dec87a47f47bf7e96619

Patch Set 1 #

Patch Set 2 : rebase. +#include #

Total comments: 6

Patch Set 3 : . #

Patch Set 4 : rebase #

Total comments: 2

Patch Set 5 : rebase #

Messages

Total messages: 52 (31 generated)
tzik
PTAL
3 years, 7 months ago (2017-05-09 09:09:40 UTC) #9
mark a. foltz
The bug doesn't really explain why this is an improvement over the existing code. The ...
3 years, 7 months ago (2017-05-09 22:28:44 UTC) #12
tzik
https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc File chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc#newcode70 chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:70: MOCK_METHOD2(StartMock, On 2017/05/09 22:28:44, mark a. foltz wrote: > ...
3 years, 7 months ago (2017-05-11 09:00:31 UTC) #15
zhaobin
https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc#newcode23 chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:23: DeviceDescriptionCallback callback) { On 2017/05/11 09:00:30, tzik wrote: > ...
3 years, 7 months ago (2017-05-11 17:38:57 UTC) #19
tzik
https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc#newcode23 chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:23: DeviceDescriptionCallback callback) { On 2017/05/11 17:38:57, zhaobin wrote: > ...
3 years, 7 months ago (2017-05-15 06:28:15 UTC) #20
mark a. foltz
Can you answer the question as to why it's necessary to std::move() before running a ...
3 years, 6 months ago (2017-05-24 22:49:11 UTC) #21
tzik
On 2017/05/09 22:28:44, mark a. foltz wrote: > The bug doesn't really explain why this ...
3 years, 6 months ago (2017-05-25 06:57:19 UTC) #22
mark a. foltz
On 2017/05/25 at 06:57:19, tzik wrote: > On 2017/05/09 22:28:44, mark a. foltz wrote: > ...
3 years, 6 months ago (2017-05-26 17:05:15 UTC) #23
mark a. foltz
On 2017/05/26 at 17:05:15, mark a. foltz wrote: > On 2017/05/25 at 06:57:19, tzik wrote: ...
3 years, 6 months ago (2017-05-26 20:17:42 UTC) #24
tzik
On 2017/05/26 17:05:15, mark a. foltz wrote: > On 2017/05/25 at 06:57:19, tzik wrote: > ...
3 years, 6 months ago (2017-05-26 21:19:42 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2867713002/40001
3 years, 6 months ago (2017-05-29 06:13:05 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/220715) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 6 months ago (2017-05-29 06:14:54 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2867713002/60001
3 years, 6 months ago (2017-05-29 09:08:37 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/449860)
3 years, 6 months ago (2017-05-29 09:15:48 UTC) #38
tzik
Adding dcheng as a SECURITY_OWNERS. PTAL to media_router_struct_traits_unittest.cc.
3 years, 6 months ago (2017-05-29 09:17:38 UTC) #40
dcheng
struct traits LGTM I think we might need to figure out a better solution in ...
3 years, 6 months ago (2017-05-29 09:30:22 UTC) #41
tzik
https://codereview.chromium.org/2867713002/diff/60001/chrome/browser/media/router/mojo/media_router_mojo_test.h File chrome/browser/media/router/mojo/media_router_mojo_test.h (right): https://codereview.chromium.org/2867713002/diff/60001/chrome/browser/media/router/mojo/media_router_mojo_test.h#newcode52 chrome/browser/media/router/mojo/media_router_mojo_test.h:52: CreateRouteCallback& callback)); On 2017/05/29 09:30:22, dcheng wrote: > Can ...
3 years, 6 months ago (2017-05-30 05:25:51 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2867713002/60001
3 years, 6 months ago (2017-05-30 05:26:40 UTC) #44
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/450419)
3 years, 6 months ago (2017-05-30 05:33:43 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2867713002/80001
3 years, 6 months ago (2017-05-30 05:43:14 UTC) #49
commit-bot: I haz the power
3 years, 6 months ago (2017-05-30 07:11:27 UTC) #52
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/318acc6c2bc98fa5f872dec87a47...

Powered by Google App Engine
This is Rietveld 408576698