Revert "Replace param |disposition| with |should_report| on IsFeatureEnabled"

This reverts commit ccf8b883.

Reason for revert: To revert https://chromium-review.googlesource.com/c/chromium/src/+/2028666

https://chromium-review.googlesource.com/c/chromium/src/+/2028666 seems to cause LinuxCFI tests failures.

LinuxCFI tests start failing at
https://ci.chromium.org/p/chromium/builders/ci/Linux%20CFI/16433
16433 contains https://chromium-review.googlesource.com/c/chromium/src/+/2028666 

Original change's description:
> 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}

TBR=dcheng@chromium.org,iclelland@chromium.org,chenleihu@google.com

Change-Id: Id6531935bbc777662fc3af2af65612495cb5a803
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2059555Reviewed-by: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/heads/master@{#741872}

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

@@ -8258,7 +8258,7 @@ void Document::CountPotentialFeaturePolicyViolation(
 }
 void Document::ReportFeaturePolicyViolation(
     mojom::blink::FeaturePolicyFeature feature,
-    mojom::blink::FeaturePolicyDisposition disposition,
+    mojom::FeaturePolicyDisposition disposition,
     const String& message,
     const String& source_file) const {
   if (!RuntimeEnabledFeatures::FeaturePolicyReportingEnabled(this))
@@ -8270,9 +8270,8 @@ void Document::ReportFeaturePolicyViolation(
   // Construct the feature policy violation report.
   const String& feature_name = GetNameForFeature(feature);
   const String& disp_str =
-      (disposition == mojom::blink::FeaturePolicyDisposition::kReport
-           ? "report"
-           : "enforce");
+      (disposition == mojom::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);
   }
 
-  bool should_report;
+  base::Optional<mojom::FeaturePolicyDisposition> disposition;
   bool enabled = GetSecurityContext().IsFeatureEnabled(feature, threshold_value,
-                                                       &should_report);
+                                                       &disposition);
 
   if (enabled) {
     // Report if the proposed header semantics change would have affected the
@@ -429,13 +429,8 @@ bool ExecutionContext::IsFeatureEnabled(
     }
   }
 
-  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);
-  }
-
+  if (disposition && report_on_failure == ReportOptions::kReportOnFailure)
+    ReportFeaturePolicyViolation(feature, *disposition, message, source_file);
   return enabled;
 }
 

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

@@ -184,21 +184,16 @@ bool SecurityContext::IsFeatureEnabled(
 bool SecurityContext::IsFeatureEnabled(
     mojom::blink::FeaturePolicyFeature feature,
     PolicyValue threshold_value,
-    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;
+    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;
   }
-
-  return feature_policy_result;
+  return (state != FeatureEnabledState::kDisabled);
 }
 
 bool SecurityContext::IsFeatureEnabled(
@@ -216,9 +211,13 @@ bool SecurityContext::IsFeatureEnabled(
   return document_policy_->IsFeatureEnabled(feature, threshold_value);
 }
 
-void SecurityContext::LogImagePolicies(
+FeatureEnabledState SecurityContext::GetFeatureEnabledState(
     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::
@@ -242,6 +241,15 @@ void SecurityContext::LogImagePolicies(
               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,6 +59,7 @@ 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
@@ -158,12 +159,14 @@ class CORE_EXPORT SecurityContext {
 
   // Tests whether the policy-controlled feature is enabled in this frame.
   // Use ExecutionContext::IsFeatureEnabled if a failure should be reported.
-  // |should_report| is an extra return value that represent whether
-  // the 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.
   bool IsFeatureEnabled(mojom::blink::FeaturePolicyFeature) const;
-  bool IsFeatureEnabled(mojom::blink::FeaturePolicyFeature,
-                        PolicyValue threshold_value,
-                        bool* should_report = nullptr) const;
+  bool IsFeatureEnabled(
+      mojom::blink::FeaturePolicyFeature,
+      PolicyValue threshold_value,
+      base::Optional<mojom::FeaturePolicyDisposition>* = nullptr) const;
 
   bool IsFeatureEnabled(mojom::blink::DocumentPolicyFeature) const;
   bool IsFeatureEnabled(mojom::blink::DocumentPolicyFeature,
@@ -177,8 +180,9 @@ class CORE_EXPORT SecurityContext {
   std::unique_ptr<DocumentPolicy> document_policy_;
 
  private:
-  void LogImagePolicies(mojom::blink::FeaturePolicyFeature,
-                        PolicyValue threshold_value) const;
+  FeatureEnabledState GetFeatureEnabledState(mojom::blink::FeaturePolicyFeature,
+                                             PolicyValue threshold_value) const;
+
   Member<ContentSecurityPolicy> content_security_policy_;
 
   network::mojom::IPAddressSpace address_space_;