Simplify PlatformVerificationFlow::CheckConsent() logic.

chrome/browser/chromeos/attestation/platform_verification_flow.cc

Index: chrome/browser/chromeos/attestation/platform_verification_flow.cc
diff --git a/chrome/browser/chromeos/attestation/platform_verification_flow.cc b/chrome/browser/chromeos/attestation/platform_verification_flow.cc
index 63304303dbb20a7a07540d5d962fe6f7ce93ba61..916913bd26a5d8b1352032a89faff39b6986db6d 100644
--- a/chrome/browser/chromeos/attestation/platform_verification_flow.cc
+++ b/chrome/browser/chromeos/attestation/platform_verification_flow.cc
@@ -216,23 +216,33 @@ void PlatformVerificationFlow::CheckEnrollment(const ChallengeContext& context,
 }
 
 void PlatformVerificationFlow::CheckConsent(const ChallengeContext& context,
-                                            bool attestation_enrolled) {
-  PrefService* pref_service = delegate_->GetPrefs(context.web_contents);
+                                            bool /* attestation_enrolled */) {

[xhwang, 2015/03/06 17:15:09]

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.

+  content::WebContents* web_contents = context.web_contents;
+
+  bool enabled_for_domain = false;

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

s/domain/origin/
The function would need to be renamed too.

[xhwang, 2015/03/06 18:40:10]

Done.

+  bool found =
+      GetDomainPref(delegate_->GetContentSettings(web_contents),
+                    delegate_->GetURL(web_contents), &enabled_for_domain);
+  if (found && !enabled_for_domain) {
+    VLOG(1) << "Platform verification denied because the domain has been "
+            << "blocked by the user.";
+    ReportError(context.callback, USER_REJECTED);

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

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.

[xhwang, 2015/03/06 18:40:10]

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.

+    return;
+  }
+
+  PrefService* pref_service = delegate_->GetPrefs(web_contents);
   if (!pref_service) {
     LOG(ERROR) << "Failed to get user prefs.";
     ReportError(context.callback, INTERNAL_ERROR);
     return;
   }
-  bool consent_required = (
-      // Consent required if attestation has never been enrolled on this device.
-      !attestation_enrolled ||
-      // Consent required if this is the first use of attestation for content
-      // protection on this device.
-      !pref_service->GetBoolean(prefs::kRAConsentFirstTime) ||
-      // Consent required if consent has never been given for this domain.
-      !GetDomainPref(delegate_->GetContentSettings(context.web_contents),
-                     delegate_->GetURL(context.web_contents),
-                     NULL));
+
+  // Consent required if user has never given consent to attestation for content
+  // protection on this device, or if user has never given consent for this

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

you probably want to reverse this if you do the comment below

[xhwang, 2015/03/06 18:40:10]

Done.

+  // domain.
+  bool consent_required =
+      !pref_service->GetBoolean(prefs::kRAConsentGranted) || !found;

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

nit: Swap to short circuit with the bool check

[xhwang, 2015/03/06 18:40:10]

Done.

+
   Delegate::ConsentCallback consent_callback = base::Bind(
       &PlatformVerificationFlow::OnConsentResponse,
       this,
@@ -252,8 +262,7 @@ void PlatformVerificationFlow::CheckConsent(const ChallengeContext& context,
 
 void PlatformVerificationFlow::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* prefs) {
-  prefs->RegisterBooleanPref(prefs::kRAConsentFirstTime,
-                             false,
+  prefs->RegisterBooleanPref(prefs::kRAConsentGranted, false,

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

nit: false,  // Default.

[xhwang, 2015/03/06 18:40:10]

Done.

                              user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
@@ -440,7 +449,10 @@ bool PlatformVerificationFlow::UpdateSettings(
     LOG(ERROR) << "Failed to get user prefs.";
     return false;
   }
-  pref_service->SetBoolean(prefs::kRAConsentFirstTime, true);
+
+  if (consent_response == CONSENT_RESPONSE_ALLOW)

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

optional nit: Since the 'goto fail' bug I've started to add {} to even
single-line conditions to help catch bad merges.

[xhwang, 2015/03/06 18:40:10]

Done.

+    pref_service->SetBoolean(prefs::kRAConsentGranted, true);
+
   RecordDomainConsent(delegate_->GetContentSettings(web_contents),
                       delegate_->GetURL(web_contents),
                       (consent_response == CONSENT_RESPONSE_ALLOW));