Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694933003:
Return management state without retrieving the management domain if the (Closed)
|
Created:
3 years, 10 months ago by Marton Hunyady Modified:
3 years, 10 months ago Reviewers:
Thiemo Nagel CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReturn management state without querying the domain.
This fixes the problem that for Active Directory managed devices
ProfilePolicyConnector::IsManaged returned false. (The original issue
that ProfilePolicyConnector::GetManagementDomain is an empty string for
AD-managed devices is crbug.com/692104.)
BUG=692107
Review-Url: https://codereview.chromium.org/2694933003
Cr-Commit-Position: refs/heads/master@{#451353}
Committed: https://chromium.googlesource.com/chromium/src/+/1892aaff7ccf6ad63b3e36d402a039d95db06c5d
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add special_user_policy_provider case #
Total comments: 2
Patch Set 3 : Extract redundant logic into GetActualPolicyStore #
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run
hunyadym@chromium.org changed reviewers: + tnagel@chromium.org
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/2694933003/diff/1/chrome/browser/policy/profi... File chrome/browser/policy/profile_policy_connector.cc (right): https://codereview.chromium.org/2694933003/diff/1/chrome/browser/policy/profi... chrome/browser/policy/profile_policy_connector.cc:159: return !GetManagementDomain().empty(); I'd suggest to use the store also in the case of special policy provider, that way you can get rid of GetManagementDomain() completely.
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 hunyadym@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/2694933003/diff/1/chrome/browser/policy/profi... File chrome/browser/policy/profile_policy_connector.cc (right): https://codereview.chromium.org/2694933003/diff/1/chrome/browser/policy/profi... chrome/browser/policy/profile_policy_connector.cc:159: return !GetManagementDomain().empty(); On 2017/02/14 17:06:03, Thiemo Nagel (slow) wrote: > I'd suggest to use the store also in the case of special policy provider, that > way you can get rid of GetManagementDomain() completely. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2694933003/diff/20001/chrome/browser/policy/p... File chrome/browser/policy/profile_policy_connector.cc (right): https://codereview.chromium.org/2694933003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/profile_policy_connector.cc:159: #if defined(OS_CHROMEOS) Nit: Instead of duplicating that code, I'd suggest to create a private method which returns a const ptr to the store that's being used. (Eg. GetEffectiveStore() or GetActualStore() or maybe also just GetStore().)
The CQ bit was checked by hunyadym@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/2694933003/diff/20001/chrome/browser/policy/p... File chrome/browser/policy/profile_policy_connector.cc (right): https://codereview.chromium.org/2694933003/diff/20001/chrome/browser/policy/p... chrome/browser/policy/profile_policy_connector.cc:159: #if defined(OS_CHROMEOS) On 2017/02/15 16:23:03, Thiemo Nagel wrote: > Nit: Instead of duplicating that code, I'd suggest to create a private method > which returns a const ptr to the store that's being used. (Eg. > GetEffectiveStore() or GetActualStore() or maybe also just GetStore().) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Lgtm! Nit: Please don't split the commit's subject line over two lines, this is highly unreadable as a lot of git-based tools assume that the subject is a (short) single line. https://chris.beams.io/posts/git-commit/
Description was changed from ========== Return management state without retrieving the management domain if the policy store exists. This fixes the problem that for Active Directory managed devices ProfilePolicyConnector::IsManaged returned false. (The original issue that ProfilePolicyConnector::GetManagementDomain is an empty string for AD-managed devices is crbug.com/692104.) BUG=692107 ========== to ========== Return management state without querying the domain. This fixes the problem that for Active Directory managed devices ProfilePolicyConnector::IsManaged returned false. (The original issue that ProfilePolicyConnector::GetManagementDomain is an empty string for AD-managed devices is crbug.com/692104.) BUG=692107 ==========
The CQ bit was checked by hunyadym@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": 40001, "attempt_start_ts": 1487354695766870,
"parent_rev": "8281f251376f2a21ad28737f5c7416f10c276d6f", "commit_rev":
"1892aaff7ccf6ad63b3e36d402a039d95db06c5d"}
Message was sent while issue was closed.
Description was changed from ========== Return management state without querying the domain. This fixes the problem that for Active Directory managed devices ProfilePolicyConnector::IsManaged returned false. (The original issue that ProfilePolicyConnector::GetManagementDomain is an empty string for AD-managed devices is crbug.com/692104.) BUG=692107 ========== to ========== Return management state without querying the domain. This fixes the problem that for Active Directory managed devices ProfilePolicyConnector::IsManaged returned false. (The original issue that ProfilePolicyConnector::GetManagementDomain is an empty string for AD-managed devices is crbug.com/692104.) BUG=692107 Review-Url: https://codereview.chromium.org/2694933003 Cr-Commit-Position: refs/heads/master@{#451353} Committed: https://chromium.googlesource.com/chromium/src/+/1892aaff7ccf6ad63b3e36d402a0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1892aaff7ccf6ad63b3e36d402a0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
