Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887323002:
Do not read redundant output param for StorePolicy DBus call family. (Closed)
|
Created:
3 years, 7 months ago by hidehiko Modified:
3 years, 7 months ago Reviewers:
Daniel Erat CC:
chromium-reviews, oshima+watch_chromium.org, hashimoto+watch_chromium.org, Mattias Nissler (ping if slow) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not read redundant output param for StorePolicy DBus call family.
For consistency, these will be removed from SessionManager.
In case of success, the value is always true, so it is just redundant.
BUG=720240
TEST=Ran bots.
Review-Url: https://codereview.chromium.org/2887323002
Cr-Commit-Position: refs/heads/master@{#472877}
Committed: https://chromium.googlesource.com/chromium/src/+/6eff4f61a15bfee8451bf106d7b74b99f6b4264c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use var. #Messages
Total messages: 19 (12 generated)
The CQ bit was checked by hidehiko@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.
hidehiko@chromium.org changed reviewers: + derat@chromium.org
PTAL.
https://codereview.chromium.org/2887323002/diff/1/chromeos/dbus/session_manag... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2887323002/diff/1/chromeos/dbus/session_manag... chromeos/dbus/session_manager_client.cc:740: callback.Run(bool(response)); i think we usually avoid using c-style casts. would you be okay with changing this to !!response or response != nullptr or something similar?
The CQ bit was checked by hidehiko@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...
Thank you for quick review! PTAL. https://codereview.chromium.org/2887323002/diff/1/chromeos/dbus/session_manag... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2887323002/diff/1/chromeos/dbus/session_manag... chromeos/dbus/session_manager_client.cc:740: callback.Run(bool(response)); On 2017/05/18 16:32:27, Daniel Erat wrote: > i think we usually avoid using c-style casts. would you be okay with changing > this to !!response or response != nullptr or something similar? Good catch! It is bool ctor, but can be interpreted as cast. So, introduced a var and compared with nullptr explicitly, so there's no ambiguity here, IMO. WDYT? cf) https://google.github.io/styleguide/cppguide.html#Casting So, alternatively bool{response} can be used.
whoops, sorry for misreading it! thanks, lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you! Submitting.
The CQ bit was checked by hidehiko@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": 20001, "attempt_start_ts": 1495131072195260,
"parent_rev": "42fad93f54e4ee681975517e2831614b5dbade05", "commit_rev":
"6eff4f61a15bfee8451bf106d7b74b99f6b4264c"}
Message was sent while issue was closed.
Description was changed from ========== Do not read redundant output param for StorePolicy DBus call family. For consistency, these will be removed from SessionManager. In case of success, the value is always true, so it is just redundant. BUG=720240 TEST=Ran bots. ========== to ========== Do not read redundant output param for StorePolicy DBus call family. For consistency, these will be removed from SessionManager. In case of success, the value is always true, so it is just redundant. BUG=720240 TEST=Ran bots. Review-Url: https://codereview.chromium.org/2887323002 Cr-Commit-Position: refs/heads/master@{#472877} Committed: https://chromium.googlesource.com/chromium/src/+/6eff4f61a15bfee8451bf106d7b7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/6eff4f61a15bfee8451bf106d7b7... |
