New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 740015 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Use OnceCallback in src/dbus

Project Member Reported by hashimoto@chromium.org, Jul 7 2017

Issue description

Callbacks like ObjectProxy::ResponseCallback and ObjectProxy::ErrorCallback can be converted to base:OnceCallback.
https://docs.google.com/document/d/1jsaJAT-WbFS2ZwLEPkAgGcMTQ6MQLIQlyQfd1JeEupg/edit#heading=h.5ztx0u98smg8
 
FWIW issue 739622 is about OnceCallback in chromeos/dbus.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a363a8cb1791cb18a4a64d52afde428e92ba73e

commit 2a363a8cb1791cb18a4a64d52afde428e92ba73e
Author: Ryo Hashimoto <hashimoto@chromium.org>
Date: Fri Jul 21 08:11:14 2017

dbus: Let ObjectProxy::CallMethod() take OnceCallback

Currently, ObjectProxy::CallMethod is implemented by creating ErrorCallback from ResponseCallback.
This does not work with OnceCallback because it's not allowed to create two callbacks from one OnceCallback.
To solve this, this change introduces CallMethodCallbackInternal to ObjectProxy which takes either Response* or ErrorResponse*, and dispatches the response to the appropriate callback.

Also, Gmock does not allow directly mocking functions which take move-only
arguments.
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#mocking-methods-that-use-move-only-types
To workaround this, add DoCallMethod to MockObjetProxy.

BUG=740015
TEST=build

Change-Id: I1a98b5fd1d6d70f69674102a3d66bcec0c13214c
Reviewed-on: https://chromium-review.googlesource.com/578541
Commit-Queue: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Satoru Takabayashi <satorux@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488615}
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/chromeos/dbus/biod/biod_client_unittest.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/chromeos/dbus/cras_audio_client_unittest.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/chromeos/dbus/gsm_sms_client_unittest.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/chromeos/dbus/modem_messaging_client_unittest.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/chromeos/dbus/power_manager_client.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/chromeos/dbus/power_manager_client_unittest.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/chromeos/dbus/shill_client_unittest_base.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/chromeos/dbus/shill_client_unittest_base.h
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/dbus/mock_object_proxy.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/dbus/mock_object_proxy.h
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/dbus/mock_unittest.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/dbus/object_proxy.cc
[modify] https://crrev.com/2a363a8cb1791cb18a4a64d52afde428e92ba73e/dbus/object_proxy.h

Components: Internals
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0fc9c713bab87120fa0c78eafa876506333ee235

commit 0fc9c713bab87120fa0c78eafa876506333ee235
Author: Ryo Hashimoto <hashimoto@chromium.org>
Date: Tue Sep 12 04:53:50 2017

dbus: Use more OnceCallbacks in ObjectProxy

Convert WaitForServiceToBeAvailableCallback and OnConnectedCallback to
OnceCallback.

Remove unused MockObjectProxy::WaitForServiceToBeAvailable().
Add MockObjectProxy::DoConnectToSignal() to workaround Gmock's
limitation around move-only arguments.
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#mocking-methods-that-use-move-only-types

BUG=740015
TEST=try

Change-Id: Ide16b01e57eac20f231d729ad570547376a7874d
Reviewed-on: https://chromium-review.googlesource.com/656480
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501180}
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/auth_policy_client.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/biod/biod_client_unittest.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/cras_audio_client_unittest.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/fake_auth_policy_client.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/gsm_sms_client_unittest.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/modem_messaging_client_unittest.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/power_manager_client_unittest.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/services/service_provider_test_helper.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/services/service_provider_test_helper.h
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/shill_client_unittest_base.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/chromeos/dbus/shill_client_unittest_base.h
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/dbus/mock_object_proxy.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/dbus/mock_object_proxy.h
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/dbus/object_proxy.cc
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/dbus/object_proxy.h
[modify] https://crrev.com/0fc9c713bab87120fa0c78eafa876506333ee235/services/device/battery/battery_status_manager_linux_unittest.cc

Comment 5 by derat@chromium.org, Apr 5 2018

Cc: derat@chromium.org ejcaruso@chromium.org
I just got a bit confused due to ExportedObject::OnExportedCallback being a base::Callback rather than a base::OnceCallback. Based on hidehiko@'s recent activity on issue 739622, this work is still ongoing, right?

(And to double-check, OnExportedCallback is only run once, right? It looked like it from the code that I glanced at.)

Sign in to add a comment