Simplify PlatformVerificationFlow::CheckConsent() logic.

Simplify PlatformVerificationFlow::CheckConsent() logic.

BUG=455262
TEST=Fixed unittest and manually tested various scenarios.

Committed: https://crrev.com/a90a4a2320aaf9ece58856a62731b943650e8a6e
Cr-Commit-Position: refs/heads/master@{#319511}

[xhwang, 2015-03-06 17:11:00]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, dkrahn@chromium.org

[xhwang, 2015-03-06 17:11:00]

Please see the bug for the context. PTAL!

[xhwang, 2015-03-06 17:15:10]

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:219: bool /*
attestation_enrolled */) {
We can do a lot of cleanup around this. But since this is still hacky and we'll
do more clean up after the content setting is not synced, I'd like to keep this
CL small and do that later.

https://codereview.chromium.org/979273003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/979273003/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:754: const char kRAConsentGranted[] =
"settings.privacy.ra_consent_granted";
Previously we set this pref even even when user clicked "NO". So we have to
create a new pref and ignore the old pref value.

[Darren Krahn, 2015-03-06 17:28:35]

Please add a 'TEST=' clause to the CL. Otherwise LGTM.

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:229:
ReportError(context.callback, USER_REJECTED);
It seems this is the most useful when the content settings *are* synced. Useful
in the sense that the user will see potentially fewer dialogs.

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:453: if
(consent_response == CONSENT_RESPONSE_ALLOW)
optional nit: Since the 'goto fail' bug I've started to add {} to even
single-line conditions to help catch bad merges.

[ddorwin, 2015-03-06 17:46:11]

lgtm % nits

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:222: bool
enabled_for_domain = false;
s/domain/origin/
The function would need to be renamed too.

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:241: //
protection on this device, or if user has never given consent for this
you probably want to reverse this if you do the comment below

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:244:
!pref_service->GetBoolean(prefs::kRAConsentGranted) || !found;
nit: Swap to short circuit with the bool check

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:265:
prefs->RegisterBooleanPref(prefs::kRAConsentGranted, false,
nit: false,  // Default.

[xhwang, 2015-03-06 18:38:28]

comments addressed

[xhwang, 2015-03-06 18:40:11]

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:222: bool
enabled_for_domain = false;
On 2015/03/06 17:46:11, ddorwin wrote:
> s/domain/origin/
> The function would need to be renamed too.

Done.

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:229:
ReportError(context.callback, USER_REJECTED);
On 2015/03/06 17:28:35, Darren Krahn wrote:
> It seems this is the most useful when the content settings *are* synced.
Useful
> in the sense that the user will see potentially fewer dialogs.

The settings are still synced. Actually this is already checked due to l.182,
but the setting could be changed since then (due to user action or syncing), so
we are checking again for the integrity of this function. All of these could be
simplified when we stop syncing the content setting.

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:241: //
protection on this device, or if user has never given consent for this
On 2015/03/06 17:46:11, ddorwin wrote:
> you probably want to reverse this if you do the comment below

Done.

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:244:
!pref_service->GetBoolean(prefs::kRAConsentGranted) || !found;
On 2015/03/06 17:46:11, ddorwin wrote:
> nit: Swap to short circuit with the bool check

Done.

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:265:
prefs->RegisterBooleanPref(prefs::kRAConsentGranted, false,
On 2015/03/06 17:46:11, ddorwin wrote:
> nit: false,  // Default.

Done.

https://codereview.chromium.org/979273003/diff/1/chrome/browser/chromeos/atte...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:453: if
(consent_response == CONSENT_RESPONSE_ALLOW)
On 2015/03/06 17:28:35, Darren Krahn wrote:
> optional nit: Since the 'goto fail' bug I've started to add {} to even
> single-line conditions to help catch bad merges.

Done.

[xhwang, 2015-03-06 19:07:54]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-03-06 19:07:55]

The patchset sent to the CQ was uploaded after l-g-t-m from dkrahn@chromium.org,
ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/979273003/#ps20001 (title:
"comments addressed")

[commit-bot: I haz the power, 2015-03-06 19:08:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/979273003/20001

[commit-bot: I haz the power, 2015-03-06 21:38:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-06 21:38:27]

Patchset 2 (id:??) landed as
https://crrev.com/a90a4a2320aaf9ece58856a62731b943650e8a6e
Cr-Commit-Position: refs/heads/master@{#319511}