Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618683003:
Update debugd client. (Closed)
DescriptionUpdate debugd client.
New dbusd API calls are being added to debugd in https://chromium-review.googlesource.com/#/c/420974. This updates the chromium debugd client to expose them.
Note this will be held for submission until after the M57 branch (Jan 19)
BUG=674731
Review-Url: https://codereview.chromium.org/2618683003
Cr-Commit-Position: refs/heads/master@{#446361}
Committed: https://chromium.googlesource.com/chromium/src/+/ca066a6bb4e4b2ab4d0c2cd316f4044fe2b36d78
Patch Set 1 #Patch Set 2 : Change result code from bool to int32 at skaus request #
Total comments: 2
Messages
Total messages: 26 (15 generated)
Description was changed from ========== Update debugd client. New dbusd API calls are being added to debugd in https://chromium-review.googlesource.com/#/c/420974. This updates the chromium debugd client to expose them. BUG=674731 ========== to ========== Update debugd client. New dbusd API calls are being added to debugd in https://chromium-review.googlesource.com/#/c/420974. This updates the chromium debugd client to expose them. Note this will be held for submission until after the M57 branch (Jan 19) BUG=674731 ==========
justincarlson@chromium.org changed reviewers: + stevenjb@chromium.org
owner lgtm Please also have someone familiar with debug_daemon_client.cc (or at least debugd) review this.
justincarlson@chromium.org changed reviewers: + skau@chromium.org
On 2017/01/19 17:52:50, stevenjb wrote: > owner lgtm > > Please also have someone familiar with debug_daemon_client.cc (or at least > debugd) review this. Sean, can you take a look?
Can you change this to handle error codes? You'll have to change the callback type.
On 2017/01/24 22:49:05, skau wrote: > Can you change this to handle error codes? You'll have to change the callback > type. Done.
lgtm https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... File chromeos/dbus/debug_daemon_client.h (right): https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... chromeos/dbus/debug_daemon_client.h:218: using CupsAddPrinterCallback = base::Callback<void(int32_t status)>; Are you going to use u or i in the dbus specification? If you use u, this should be a uint32_t
https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... File chromeos/dbus/debug_daemon_client.h (right): https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... chromeos/dbus/debug_daemon_client.h:218: using CupsAddPrinterCallback = base::Callback<void(int32_t status)>; On 2017/01/25 00:16:40, skau wrote: > Are you going to use u or i in the dbus specification? If you use u, this should > be a uint32_t Was planning to use int32_t; is there a reason you want to go unsigned?
On 2017/01/25 00:32:35, Carlson wrote: > https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... > File chromeos/dbus/debug_daemon_client.h (right): > > https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... > chromeos/dbus/debug_daemon_client.h:218: using CupsAddPrinterCallback = > base::Callback<void(int32_t status)>; > On 2017/01/25 00:16:40, skau wrote: > > Are you going to use u or i in the dbus specification? If you use u, this > should > > be a uint32_t > > Was planning to use int32_t; is there a reason you want to go unsigned? Positive integers are nicer? I wouldn't expect a negative status code since we have a singular success value.
On 2017/01/25 02:04:04, skau wrote: > On 2017/01/25 00:32:35, Carlson wrote: > > > https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... > > File chromeos/dbus/debug_daemon_client.h (right): > > > > > https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... > > chromeos/dbus/debug_daemon_client.h:218: using CupsAddPrinterCallback = > > base::Callback<void(int32_t status)>; > > On 2017/01/25 00:16:40, skau wrote: > > > Are you going to use u or i in the dbus specification? If you use u, this > > should > > > be a uint32_t > > > > Was planning to use int32_t; is there a reason you want to go unsigned? > > Positive integers are nicer? I wouldn't expect a negative status code since we > have a singular success value. I don't actually have a strong preference personally here, but I think you're on the wrong side of the style guide here: "Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate)." So I'd say make your error codes positive anyways and don't worry about it being a signed type. Unless you think there are going to be more than 2^31-1 errors, in which case we've got bigger problems. :) -J
The CQ bit was checked by justincarlson@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/01/25 17:36:01, Carlson wrote: > On 2017/01/25 02:04:04, skau wrote: > > On 2017/01/25 00:32:35, Carlson wrote: > > > > > > https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... > > > File chromeos/dbus/debug_daemon_client.h (right): > > > > > > > > > https://codereview.chromium.org/2618683003/diff/20001/chromeos/dbus/debug_dae... > > > chromeos/dbus/debug_daemon_client.h:218: using CupsAddPrinterCallback = > > > base::Callback<void(int32_t status)>; > > > On 2017/01/25 00:16:40, skau wrote: > > > > Are you going to use u or i in the dbus specification? If you use u, this > > > should > > > > be a uint32_t > > > > > > Was planning to use int32_t; is there a reason you want to go unsigned? > > > > Positive integers are nicer? I wouldn't expect a negative status code since > we > > have a singular success value. > > I don't actually have a strong preference personally here, but I think you're on > the wrong side of the style guide here: > > "Do not use unsigned types to mean “this value should never be < 0”. For that, > use assertions or run-time checks (as appropriate)." > > So I'd say make your error codes positive anyways and don't worry about it being > a signed type. Unless you think there are going to be more than 2^31-1 errors, > in which case we've got bigger problems. :) > > -J I forgot about that rule. int32_t is clearly more appropriate since it's not a bit field.
The CQ bit was checked by justincarlson@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 justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2618683003/#ps20001 (title: "Change result code from bool to int32 at skaus request")
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": 20001, "attempt_start_ts": 1485451487871440,
"parent_rev": "2b2839b3fba03dab76bee97c6aa4eb1ab1ecb836", "commit_rev":
"ca066a6bb4e4b2ab4d0c2cd316f4044fe2b36d78"}
Message was sent while issue was closed.
Description was changed from ========== Update debugd client. New dbusd API calls are being added to debugd in https://chromium-review.googlesource.com/#/c/420974. This updates the chromium debugd client to expose them. Note this will be held for submission until after the M57 branch (Jan 19) BUG=674731 ========== to ========== Update debugd client. New dbusd API calls are being added to debugd in https://chromium-review.googlesource.com/#/c/420974. This updates the chromium debugd client to expose them. Note this will be held for submission until after the M57 branch (Jan 19) BUG=674731 Review-Url: https://codereview.chromium.org/2618683003 Cr-Commit-Position: refs/heads/master@{#446361} Committed: https://chromium.googlesource.com/chromium/src/+/ca066a6bb4e4b2ab4d0c2cd316f4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ca066a6bb4e4b2ab4d0c2cd316f4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
