|
|
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. |
DescriptionUse 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)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use OnceCallback on Mojo interfaces in //chrome/common/media_router ========== to ========== 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 ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tzik@chromium.org changed reviewers: + mfoltz@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The bug doesn't really explain why this is an improvement over the existing code. The small amount of typing saved by pass-by-value is a small benefit compared to the ubiquitous usage of std::move. Can base::OnceCallback be redesigned to not require moving to a temporary before invocation? https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:70: MOCK_METHOD2(StartMock, s/StartMock/StartInternal/ for consistency with other code https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:23: DeviceDescriptionCallback callback) { Can this just be a base::RepeatingCallback instead of copying?
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... 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: > s/StartMock/StartInternal/ for consistency with other code Done. https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:23: DeviceDescriptionCallback callback) { On 2017/05/09 22:28:44, mark a. foltz wrote: > Can this just be a base::RepeatingCallback instead of copying? So, the callback is not from Mojo-generated code? OK, reverted the code and updated the callback type to RepeatingCallback.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
zhaobin@chromium.org changed reviewers: + zhaobin@chromium.org
https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:23: DeviceDescriptionCallback callback) { On 2017/05/11 09:00:30, tzik wrote: > On 2017/05/09 22:28:44, mark a. foltz wrote: > > Can this just be a base::RepeatingCallback instead of copying? > > So, the callback is not from Mojo-generated code? OK, reverted the code and > updated the callback type to RepeatingCallback. The callback is from mojo-generated code. using DeviceDescriptionCallback = chrome::mojom::DialDeviceDescriptionParser::ParseDialDeviceDescriptionCallback;
https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc (right): https://codereview.chromium.org/2867713002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc:23: DeviceDescriptionCallback callback) { On 2017/05/11 17:38:57, zhaobin wrote: > On 2017/05/11 09:00:30, tzik wrote: > > On 2017/05/09 22:28:44, mark a. foltz wrote: > > > Can this just be a base::RepeatingCallback instead of copying? > > > > So, the callback is not from Mojo-generated code? OK, reverted the code and > > updated the callback type to RepeatingCallback. > > The callback is from mojo-generated code. > > using DeviceDescriptionCallback = > chrome::mojom::DialDeviceDescriptionParser::ParseDialDeviceDescriptionCallback; Right, the callback type was from Mojo. But, as the callback object itself is not created by Mojo-generated code, it seems able to have separate typedef, and leave it legacy callback now, as this CL do now.
Can you answer the question as to why it's necessary to std::move() before running a OnceCallback?
On 2017/05/09 22:28:44, mark a. foltz wrote: > The bug doesn't really explain why this is an improvement over the existing > code. The small amount of typing saved by pass-by-value is a small benefit > compared to the ubiquitous usage of std::move. Can base::OnceCallback be > redesigned to not require moving to a temporary before invocation? > Added that to a discussion section to the explainer document. https://docs.google.com/document/d/1jsaJAT-WbFS2ZwLEPkAgGcMTQ6MQLIQlyQfd1JeEu... The main motivation to OnceCallback itself is to avoid the reference counting of the internal storage of Callback. That may contain any thread unsafe object, and tends to introduce potential data race as the destruction thread is unpredictable. For the rvalue qualified Callback::Run(), I believe it's the most straightforward way to mark an object as consumed. Rather than adding `Callback::RunAndReset()` or similar and requesting devs to be aware that behavior, I'd put it on the general move semantics.
On 2017/05/25 at 06:57:19, tzik wrote: > On 2017/05/09 22:28:44, mark a. foltz wrote: > > The bug doesn't really explain why this is an improvement over the existing > > code. The small amount of typing saved by pass-by-value is a small benefit > > compared to the ubiquitous usage of std::move. Can base::OnceCallback be > > redesigned to not require moving to a temporary before invocation? > > > > Added that to a discussion section to the explainer document. > https://docs.google.com/document/d/1jsaJAT-WbFS2ZwLEPkAgGcMTQ6MQLIQlyQfd1JeEu... > > The main motivation to OnceCallback itself is to avoid the reference counting of the internal storage of Callback. That may contain any thread unsafe object, and tends to introduce potential data race as the destruction thread is unpredictable. > For the rvalue qualified Callback::Run(), I believe it's the most straightforward way to mark an object as consumed. Rather than adding `Callback::RunAndReset()` or similar and requesting devs to be aware that behavior, I'd put it on the general move semantics. The problem I see is that OnceCallback is-a Callback despite having completely different semantics around Run(), and you've worked around this by overloading the type of the Run() method receiver (by requiring an rvalue or not). Is the design rationale for this documented anywhere?
On 2017/05/26 at 17:05:15, mark a. foltz wrote: > On 2017/05/25 at 06:57:19, tzik wrote: > > On 2017/05/09 22:28:44, mark a. foltz wrote: > > > The bug doesn't really explain why this is an improvement over the existing > > > code. The small amount of typing saved by pass-by-value is a small benefit > > > compared to the ubiquitous usage of std::move. Can base::OnceCallback be > > > redesigned to not require moving to a temporary before invocation? > > > > > > > Added that to a discussion section to the explainer document. > > https://docs.google.com/document/d/1jsaJAT-WbFS2ZwLEPkAgGcMTQ6MQLIQlyQfd1JeEu... > > > > The main motivation to OnceCallback itself is to avoid the reference counting of the internal storage of Callback. That may contain any thread unsafe object, and tends to introduce potential data race as the destruction thread is unpredictable. > > For the rvalue qualified Callback::Run(), I believe it's the most straightforward way to mark an object as consumed. Rather than adding `Callback::RunAndReset()` or similar and requesting devs to be aware that behavior, I'd put it on the general move semantics. > > The problem I see is that OnceCallback is-a Callback despite having completely different semantics around Run(), and you've worked around this by overloading the type of the Run() method receiver (by requiring an rvalue or not). > > Is the design rationale for this documented anywhere? LGTM Okay, I read the document. This still isn't great from a style point of view, but I agree it's better than the alternatives considered. Having compiler support for use-after-move detection is the concrete benefit I was looking for in my original question.
On 2017/05/26 17:05:15, mark a. foltz wrote: > On 2017/05/25 at 06:57:19, tzik wrote: > > On 2017/05/09 22:28:44, mark a. foltz wrote: > > > The bug doesn't really explain why this is an improvement over the existing > > > code. The small amount of typing saved by pass-by-value is a small benefit > > > compared to the ubiquitous usage of std::move. Can base::OnceCallback be > > > redesigned to not require moving to a temporary before invocation? > > > > > > > Added that to a discussion section to the explainer document. > > > https://docs.google.com/document/d/1jsaJAT-WbFS2ZwLEPkAgGcMTQ6MQLIQlyQfd1JeEu... > > > > The main motivation to OnceCallback itself is to avoid the reference counting > of the internal storage of Callback. That may contain any thread unsafe object, > and tends to introduce potential data race as the destruction thread is > unpredictable. > > For the rvalue qualified Callback::Run(), I believe it's the most > straightforward way to mark an object as consumed. Rather than adding > `Callback::RunAndReset()` or similar and requesting devs to be aware that > behavior, I'd put it on the general move semantics. > > The problem I see is that OnceCallback is-a Callback despite having completely > different semantics around Run(), and you've worked around this by overloading > the type of the Run() method receiver (by requiring an rvalue or not). The hierarchy is intended to be the opposite direction: Callback is-a OnceCallback, but OnceCallback is not a Callback. Callback is convertible to OnceCallback, both of them are movable, and can Run() on a rvalue reference. Callback has a Run() overload for lvalue reference and copyable as the extension. > > Is the design rationale for this documented anywhere? There are some old docs and in-repository doc about Callback and its conversion: https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md https://docs.google.com/document/d/114s1KTrf6xXdwInRDMDJAdxX_zIKt6xJSbZ-9qBgU... https://docs.google.com/presentation/d/1nDe5iDUQ6U-4WmJMvfBll72xNCUG8wXRrHmof... We should probably revise the in-repo doc for that?
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2867713002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
tzik@chromium.org changed reviewers: + dcheng@chromium.org
Adding dcheng as a SECURITY_OWNERS. PTAL to media_router_struct_traits_unittest.cc.
struct traits LGTM I think we might need to figure out a better solution in the long run for the mutable reference thing, since it is banned by the style guide. https://codereview.chromium.org/2867713002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_test.h (right): https://codereview.chromium.org/2867713002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.h:52: CreateRouteCallback& callback)); Can this be done without using a mutable reference argument, which is banned by Google style? (I'm guessing not, since we won't be able to run the callback then) Maybe that's an argument not to use gmock here in the long run?
https://codereview.chromium.org/2867713002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_test.h (right): https://codereview.chromium.org/2867713002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_test.h:52: CreateRouteCallback& callback)); On 2017/05/29 09:30:22, dcheng wrote: > Can this be done without using a mutable reference argument, which is banned by > Google style? > > (I'm guessing not, since we won't be able to run the callback then) > > Maybe that's an argument not to use gmock here in the long run? The move-only type support of gmock seems getting ready, according to b/10817494. Let's revisit this later to clean up the mutable references.
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2867713002/#ps80001 (title: "rebase")
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": 80001, "attempt_start_ts": 1496122968845810, "parent_rev": "e6fa9ad53a68ce6204ade3cb16b0d8cf7090858a", "commit_rev": "318acc6c2bc98fa5f872dec87a47f47bf7e96619"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/318acc6c2bc98fa5f872dec87a47... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/318acc6c2bc98fa5f872dec87a47... |