Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2530833002:
Start authpolicyd (Closed)
|
Created:
4 years ago by Roman Sorokin (ftl) Modified:
4 years ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStart authpolicyd
Start in two places:
-On press custom shortcut (Ctrl-Alt-Shift-A) for the enrollment
-On reading install attributes in case it's Active Directory managed
device.
BUG=638663
TEST=manual
Committed: https://crrev.com/3bfc9e3eb223dca44ef3cf0b82832da7c27358eb
Cr-Commit-Position: refs/heads/master@{#435603}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Nits #
Total comments: 2
Patch Set 3 : More nits #
Messages
Total messages: 23 (12 generated)
rsorokin@chromium.org changed reviewers: + tnagel@chromium.org
Thiemo, PTAL
rsorokin@chromium.org changed reviewers: + alemate@chromium.org
Alex, PTAL
The CQ bit was checked by rsorokin@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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with nits. https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:116: DCHECK(thread_manager); nit: remove DCHECK According to https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... : - A consequence of this is that you should not handle DCHECK() failures, even if failure would result in a crash. https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:119: DCHECK(upstart_client); Ditto. https://codereview.chromium.org/2530833002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2530833002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1212: DCHECK(thread_manager); ditto https://codereview.chromium.org/2530833002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1214: DCHECK(upstart_client); ditto
The CQ bit was checked by rsorokin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:116: DCHECK(thread_manager); On 2016/12/01 09:02:20, Alexander Alekseev wrote: > nit: remove DCHECK > > According to > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > : > > - A consequence of this is that you should not handle DCHECK() failures, even if > failure would result in a crash. Done. https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:119: DCHECK(upstart_client); On 2016/12/01 09:02:20, Alexander Alekseev wrote: > Ditto. Done. https://codereview.chromium.org/2530833002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2530833002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1212: DCHECK(thread_manager); On 2016/12/01 09:02:20, Alexander Alekseev wrote: > ditto Done. https://codereview.chromium.org/2530833002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1214: DCHECK(upstart_client); On 2016/12/01 09:02:20, Alexander Alekseev wrote: > ditto Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % nits https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:116: DCHECK(thread_manager); > - A consequence of this is that you should not handle DCHECK() failures, even if > failure would result in a crash. I'm fine with removing the DCHECK(), but just for the record, what you write is not the intention of "you should not handle DCHECK() failure". // Good. DCHECK(ptr); ptr->DoSomething(); // Bad. You should not handle DCHECK() failure. DCHECK(ptr); if (ptr) ptr->DoSomething(); https://codereview.chromium.org/2530833002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/2530833002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:118: upstart_client->StartAuthPolicyService(); Nit: I think it's clearer to write without temporary variables: chromeos::DBusThreadManager::Get() ->GetUpstartClient() ->StartAuthPolicyService(); https://codereview.chromium.org/2530833002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2530833002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1210: chromeos::DBusThreadManager* thread_manager = Nit: same as the other call
The CQ bit was checked by rsorokin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org, tnagel@chromium.org Link to the patchset: https://codereview.chromium.org/2530833002/#ps40001 (title: "More nits")
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": 40001, "attempt_start_ts": 1480599473404800,
"parent_rev": "4c2240bfb766adfc6438c4e8812fac3b862fa474", "commit_rev":
"5939f8dd62324fbfa375dfb905354e9c4b953e27"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
On 2016/12/01 14:03:46, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) still lgtm
Message was sent while issue was closed.
Description was changed from ========== Start authpolicyd Start in two places: -On press custom shortcut (Ctrl-Alt-Shift-A) for the enrollment -On reading install attributes in case it's Active Directory managed device. BUG=638663 TEST=manual ========== to ========== Start authpolicyd Start in two places: -On press custom shortcut (Ctrl-Alt-Shift-A) for the enrollment -On reading install attributes in case it's Active Directory managed device. BUG=638663 TEST=manual Committed: https://crrev.com/3bfc9e3eb223dca44ef3cf0b82832da7c27358eb Cr-Commit-Position: refs/heads/master@{#435603} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3bfc9e3eb223dca44ef3cf0b82832da7c27358eb Cr-Commit-Position: refs/heads/master@{#435603}
Message was sent while issue was closed.
https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/2530833002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:116: DCHECK(thread_manager); On 2016/12/01 13:33:51, Thiemo Nagel (slow) wrote: > > - A consequence of this is that you should not handle DCHECK() failures, even > if > > failure would result in a crash. > > I'm fine with removing the DCHECK(), but just for the record, what you write is > not the intention of "you should not handle DCHECK() failure". > > // Good. > DCHECK(ptr); > ptr->DoSomething(); > > // Bad. You should not handle DCHECK() failure. > DCHECK(ptr); > if (ptr) > ptr->DoSomething(); Yes, I actually agree with you. I just got used to "never DCHECK pointers if it is going to crash on the next line". But we can put it back if you think (as owner) that they are helpful here.
Message was sent while issue was closed.
> Yes, I actually agree with you. I just got used to "never DCHECK pointers if it > is going to crash on the next line". For debug builds I don't care much. Just make sure to not dereference a nullptr in production builds (i.e. don't rely on nullptr dereference crashing) because that can cause subtle problems. > But we can put it back if you think (as > owner) that they are helpful here. I have no opinion on that. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
