Replace param |disposition| with |should_report| on IsFeatureEnabled

Previously |disposition| on IsFeatureEnabled have an awkward type
base::Optional<mojom::FeaturePolicyDisposition>*, which has 3 states
correspond to FeatureEnabledState:
- base::nullopt <= kEnabled
- FeaturePolicyDisposition::kReport <= kReportOnly
- FeaturePolicyDisposition::kEnforce <= kDisabled

This CL simplifies the representation of this piece of information to
|should_report|, and removes FeatureEnabledState enum.

The mapping of FeatureEnabledState to new logic:
kEnabled: enabled=true; should_report=false
kReportOnly: enabled = true; should_report=true
kDisabled: enabled = false; should_report=true

Change-Id: I1fa82fb5e9924b8eed448853b139772e024bd531
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037326
Commit-Queue: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741789}

third_party/blink/renderer/core/dom/document.cc

@@ -8258,7 +8258,7 @@ void Document::CountPotentialFeaturePolicyViolation(
 }
 void Document::ReportFeaturePolicyViolation(
     mojom::blink::FeaturePolicyFeature feature,
-    mojom::FeaturePolicyDisposition disposition,
+    mojom::blink::FeaturePolicyDisposition disposition,
     const String& message,
     const String& source_file) const {
   if (!RuntimeEnabledFeatures::FeaturePolicyReportingEnabled(this))
@@ -8270,8 +8270,9 @@ void Document::ReportFeaturePolicyViolation(
   // Construct the feature policy violation report.
   const String& feature_name = GetNameForFeature(feature);
   const String& disp_str =
-      (disposition == mojom::FeaturePolicyDisposition::kReport ? "report"
-                                                               : "enforce");
+      (disposition == mojom::blink::FeaturePolicyDisposition::kReport
+           ? "report"
+           : "enforce");
 
   FeaturePolicyViolationReportBody* body =
       source_file.IsEmpty()

third_party/blink/renderer/core/execution_context/execution_context.cc

@@ -410,9 +410,9 @@ bool ExecutionContext::IsFeatureEnabled(
     CountPotentialFeaturePolicyViolation(feature);
   }
 
-  base::Optional<mojom::FeaturePolicyDisposition> disposition;
+  bool should_report;
   bool enabled = GetSecurityContext().IsFeatureEnabled(feature, threshold_value,
-                                                       &disposition);
+                                                       &should_report);
 
   if (enabled) {
     // Report if the proposed header semantics change would have affected the
@@ -429,8 +429,13 @@ bool ExecutionContext::IsFeatureEnabled(
     }
   }
 
-  if (disposition && report_on_failure == ReportOptions::kReportOnFailure)
-    ReportFeaturePolicyViolation(feature, *disposition, message, source_file);
+  if (should_report && report_on_failure == ReportOptions::kReportOnFailure) {
+    mojom::blink::FeaturePolicyDisposition disposition =
+        enabled ? mojom::blink::FeaturePolicyDisposition::kReport
+                : mojom::blink::FeaturePolicyDisposition::kEnforce;
+    ReportFeaturePolicyViolation(feature, disposition, message, source_file);
+  }
+
   return enabled;
 }
 

third_party/blink/renderer/core/execution_context/security_context.cc

@@ -184,16 +184,21 @@ bool SecurityContext::IsFeatureEnabled(
 bool SecurityContext::IsFeatureEnabled(
     mojom::blink::FeaturePolicyFeature feature,
     PolicyValue threshold_value,
-    base::Optional<mojom::FeaturePolicyDisposition>* disposition) const {
-  FeatureEnabledState state = GetFeatureEnabledState(feature, threshold_value);
-  if (state == FeatureEnabledState::kEnabled)
-    return true;
-  if (disposition) {
-    *disposition = (state == FeatureEnabledState::kReportOnly)
-                       ? mojom::FeaturePolicyDisposition::kReport
-                       : mojom::FeaturePolicyDisposition::kEnforce;
+    bool* should_report) const {
+  LogImagePolicies(feature, threshold_value);
+  DCHECK(feature_policy_);
+  bool feature_policy_result =
+      feature_policy_->IsFeatureEnabled(feature, threshold_value);
+  bool report_only_feature_policy_result =
+      !report_only_feature_policy_ ||
+      report_only_feature_policy_->IsFeatureEnabled(feature, threshold_value);
+
+  if (should_report) {
+    *should_report =
+        !feature_policy_result || !report_only_feature_policy_result;
   }
-  return (state != FeatureEnabledState::kDisabled);
+
+  return feature_policy_result;
 }
 
 bool SecurityContext::IsFeatureEnabled(
@@ -211,13 +216,9 @@ bool SecurityContext::IsFeatureEnabled(
   return document_policy_->IsFeatureEnabled(feature, threshold_value);
 }
 
-FeatureEnabledState SecurityContext::GetFeatureEnabledState(
+void SecurityContext::LogImagePolicies(
     mojom::blink::FeaturePolicyFeature feature,
     PolicyValue threshold_value) const {
-  // The policy should always be initialized before checking it to ensure we
-  // properly inherit the parent policy.
-  DCHECK(feature_policy_);
-
   // Log metrics for unoptimized-*-images and oversized-images policies.
   if ((feature >= mojom::blink::FeaturePolicyFeature::kUnoptimizedLossyImages &&
        feature <= mojom::blink::FeaturePolicyFeature::
@@ -241,15 +242,6 @@ FeatureEnabledState SecurityContext::GetFeatureEnabledState(
               GetImagePolicyHistogramName(feature), 0, 100, 101, 0x1));
     }
   }
-  if (feature_policy_->IsFeatureEnabled(feature, threshold_value)) {
-    if (report_only_feature_policy_ &&
-        !report_only_feature_policy_->IsFeatureEnabled(feature,
-                                                       threshold_value)) {
-      return FeatureEnabledState::kReportOnly;
-    }
-    return FeatureEnabledState::kEnabled;
-  }
-  return FeatureEnabledState::kDisabled;
 }
 
 }  // namespace blink

third_party/blink/renderer/core/execution_context/security_context.h

@@ -59,7 +59,6 @@ using ParsedFeaturePolicy = std::vector<ParsedFeaturePolicyDeclaration>;
 // Whether to report policy violations when checking whether a feature is
 // enabled.
 enum class ReportOptions { kReportOnFailure, kDoNotReport };
-enum class FeatureEnabledState { kDisabled, kReportOnly, kEnabled };
 
 // Defines the security properties (such as the security origin, content
 // security policy, and other restrictions) of an environment in which
@@ -159,14 +158,12 @@ class CORE_EXPORT SecurityContext {
 
   // Tests whether the policy-controlled feature is enabled in this frame.
   // Use ExecutionContext::IsFeatureEnabled if a failure should be reported.
-  // If a non-null base::Optional<mojom::FeaturePolicyDisposition>* is provided
-  // and the feature is disabled via feature policy, it will be populated to
-  // indicate whether the feature usage should be blocked or merely reported.
+  // |should_report| is an extra return value that represent whether
+  // the failure should be reported.
   bool IsFeatureEnabled(mojom::blink::FeaturePolicyFeature) const;
-  bool IsFeatureEnabled(
-      mojom::blink::FeaturePolicyFeature,
-      PolicyValue threshold_value,
-      base::Optional<mojom::FeaturePolicyDisposition>* = nullptr) const;
+  bool IsFeatureEnabled(mojom::blink::FeaturePolicyFeature,
+                        PolicyValue threshold_value,
+                        bool* should_report = nullptr) const;
 
   bool IsFeatureEnabled(mojom::blink::DocumentPolicyFeature) const;
   bool IsFeatureEnabled(mojom::blink::DocumentPolicyFeature,
@@ -180,9 +177,8 @@ class CORE_EXPORT SecurityContext {
   std::unique_ptr<DocumentPolicy> document_policy_;
 
  private:
-  FeatureEnabledState GetFeatureEnabledState(mojom::blink::FeaturePolicyFeature,
-                                             PolicyValue threshold_value) const;
-
+  void LogImagePolicies(mojom::blink::FeaturePolicyFeature,
+                        PolicyValue threshold_value) const;
   Member<ContentSecurityPolicy> content_security_policy_;
 
   network::mojom::IPAddressSpace address_space_;