Add non-comparable PolicyValue support

Previously we allow PolicyValue to always represent certain strictness,
i.e. comparable.

However, with the incoming introduction of enum type PolicyValue, this
will no longer hold true. There is no logical ordering of strictness
for some enums, e.g. Enum.RED > Enum.BLUE does not make sense.

This CL adds the support for non-comparable PolicyValue.

Change-Id: I1f260d6e357e5e09c4577d5cd08af87e2df5f21e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343606Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Charlie Hu <chenleihu@google.com>
Cr-Commit-Position: refs/heads/master@{#797567}

third_party/blink/common/feature_policy/document_policy.cc

@@ -75,26 +75,36 @@ base::Optional<std::string> DocumentPolicy::SerializeInternal(
 
 // static
 DocumentPolicyFeatureState DocumentPolicy::MergeFeatureState(
-    const DocumentPolicyFeatureState& policy1,
-    const DocumentPolicyFeatureState& policy2) {
+    const DocumentPolicyFeatureState& base_policy,
+    const DocumentPolicyFeatureState& override_policy) {
   DocumentPolicyFeatureState result;
-  auto i1 = policy1.begin();
-  auto i2 = policy2.begin();
+  auto i1 = base_policy.begin();
+  auto i2 = override_policy.begin();
 
   // Because std::map is by default ordered in ascending order based on key
   // value, we can run 2 iterators simultaneously through both maps to merge
   // them.
-  while (i1 != policy1.end() || i2 != policy2.end()) {
-    if (i1 == policy1.end()) {
+  while (i1 != base_policy.end() || i2 != override_policy.end()) {
+    if (i1 == base_policy.end()) {
       result.insert(*i2);
       i2++;
-    } else if (i2 == policy2.end()) {
+    } else if (i2 == override_policy.end()) {
       result.insert(*i1);
       i1++;
     } else {
       if (i1->first == i2->first) {
-        // Take the stricter policy when there is a key conflict.
-        result.emplace(i1->first, std::min(i1->second, i2->second));
+        const PolicyValue& base_value = i1->second;
+        const PolicyValue& override_value = i2->second;
+        // When policy value has strictness ordering e.g. boolean, take the
+        // stricter one. In this case a.IsCompatibleWith(b) means a is eq or
+        // stricter than b.
+        // When policy value does not have strictness ordering, e.g. enum,
+        // take override_value. In this case a.IsCompatibleWith(b) means
+        // a != b.
+        const PolicyValue& new_value =
+            base_value.IsCompatibleWith(override_value) ? base_value
+                                                        : override_value;
+        result.emplace(i1->first, new_value);
         i1++;
         i2++;
       } else if (i1->first < i2->first) {
@@ -121,7 +131,7 @@ bool DocumentPolicy::IsFeatureEnabled(
 bool DocumentPolicy::IsFeatureEnabled(
     mojom::DocumentPolicyFeature feature,
     const PolicyValue& threshold_value) const {
-  return GetFeatureValue(feature) >= threshold_value;
+  return threshold_value.IsCompatibleWith(GetFeatureValue(feature));
 }
 
 PolicyValue DocumentPolicy::GetFeatureValue(
@@ -172,10 +182,6 @@ bool DocumentPolicy::IsPolicyCompatible(
     const DocumentPolicyFeatureState& required_policy,
     const DocumentPolicyFeatureState& incoming_policy) {
   for (const auto& required_entry : required_policy) {
-    // feature value > threshold => enabled, where feature value is the value in
-    // document policy and threshold is the value to test against.
-    // The smaller the feature value, the stricter the policy.
-    // Incoming policy should be at least as strict as the required one.
     const auto& feature = required_entry.first;
     const auto& required_value = required_entry.second;
     // Use default value when incoming policy does not specify a value.
@@ -185,7 +191,7 @@ bool DocumentPolicy::IsPolicyCompatible(
             ? incoming_entry->second
             : GetDocumentPolicyFeatureInfoMap().at(feature).default_value;
 
-    if (required_value < incoming_value)
+    if (!incoming_value.IsCompatibleWith(required_value))
       return false;
   }
   return true;

third_party/blink/common/feature_policy/policy_value.cc

@@ -91,13 +91,13 @@ bool operator!=(const PolicyValue& lhs, const PolicyValue& rhs) {
   return !(lhs == rhs);
 }
 
-bool operator<(const PolicyValue& lhs, const PolicyValue& rhs) {
-  DCHECK_EQ(lhs.Type(), rhs.Type());
-  switch (lhs.Type()) {
+bool PolicyValue::IsCompatibleWith(const PolicyValue& required) const {
+  DCHECK_EQ(type_, required.Type());
+  switch (type_) {
     case mojom::PolicyValueType::kBool:
-      return !lhs.BoolValue() && rhs.BoolValue();
+      return !bool_value_ || required.bool_value_;
     case mojom::PolicyValueType::kDecDouble:
-      return lhs.DoubleValue() < rhs.DoubleValue();
+      return double_value_ <= required.double_value_;
     case mojom::PolicyValueType::kNull:
       NOTREACHED();
       break;
@@ -105,18 +105,6 @@ bool operator<(const PolicyValue& lhs, const PolicyValue& rhs) {
   return false;
 }
 
-bool operator<=(const PolicyValue& lhs, const PolicyValue& rhs) {
-  return lhs < rhs || lhs == rhs;
-}
-
-bool operator>(const PolicyValue& lhs, const PolicyValue& rhs) {
-  return rhs < lhs;
-}
-
-bool operator>=(const PolicyValue& lhs, const PolicyValue& rhs) {
-  return rhs < lhs || rhs == lhs;
-}
-
 void PolicyValue::SetToMax() {
   switch (type_) {
     case mojom::PolicyValueType::kBool:

third_party/blink/common/feature_policy/policy_value_unittest.cc

@@ -43,33 +43,21 @@ TEST_F(PolicyValueTest, TestCanCompareBoolValues) {
   PolicyValue false_value(false);
   PolicyValue true_value(true);
 
-  EXPECT_FALSE(false_value < false_value);
-  EXPECT_TRUE(false_value <= false_value);
   EXPECT_TRUE(false_value == false_value);
   EXPECT_FALSE(false_value != false_value);
-  EXPECT_TRUE(false_value >= false_value);
-  EXPECT_FALSE(false_value > false_value);
+  EXPECT_TRUE(false_value.IsCompatibleWith(false_value));
 
-  EXPECT_TRUE(false_value < true_value);
-  EXPECT_TRUE(false_value <= true_value);
   EXPECT_FALSE(false_value == true_value);
   EXPECT_TRUE(false_value != true_value);
-  EXPECT_FALSE(false_value >= true_value);
-  EXPECT_FALSE(false_value > true_value);
+  EXPECT_TRUE(false_value.IsCompatibleWith(true_value));
 
-  EXPECT_FALSE(true_value < false_value);
-  EXPECT_FALSE(true_value <= false_value);
   EXPECT_FALSE(true_value == false_value);
   EXPECT_TRUE(true_value != false_value);
-  EXPECT_TRUE(true_value >= false_value);
-  EXPECT_TRUE(true_value > false_value);
+  EXPECT_FALSE(true_value.IsCompatibleWith(false_value));
 
-  EXPECT_FALSE(true_value < true_value);
-  EXPECT_TRUE(true_value <= true_value);
   EXPECT_TRUE(true_value == true_value);
   EXPECT_FALSE(true_value != true_value);
-  EXPECT_TRUE(true_value >= true_value);
-  EXPECT_FALSE(true_value > true_value);
+  EXPECT_TRUE(true_value.IsCompatibleWith(true_value));
 }
 
 TEST_F(PolicyValueTest, TestCanCreateDoubleValues) {
@@ -100,33 +88,21 @@ TEST_F(PolicyValueTest, TestCanCompareDoubleValues) {
   PolicyValue low_value(1.0);
   PolicyValue high_value(2.0);
 
-  EXPECT_FALSE(low_value < low_value);
-  EXPECT_TRUE(low_value <= low_value);
   EXPECT_TRUE(low_value == low_value);
   EXPECT_FALSE(low_value != low_value);
-  EXPECT_TRUE(low_value >= low_value);
-  EXPECT_FALSE(low_value > low_value);
+  EXPECT_TRUE(low_value.IsCompatibleWith(low_value));
 
-  EXPECT_TRUE(low_value < high_value);
-  EXPECT_TRUE(low_value <= high_value);
   EXPECT_FALSE(low_value == high_value);
   EXPECT_TRUE(low_value != high_value);
-  EXPECT_FALSE(low_value >= high_value);
-  EXPECT_FALSE(low_value > high_value);
+  EXPECT_TRUE(low_value.IsCompatibleWith(high_value));
 
-  EXPECT_FALSE(high_value < low_value);
-  EXPECT_FALSE(high_value <= low_value);
   EXPECT_FALSE(high_value == low_value);
   EXPECT_TRUE(high_value != low_value);
-  EXPECT_TRUE(high_value >= low_value);
-  EXPECT_TRUE(high_value > low_value);
+  EXPECT_FALSE(high_value.IsCompatibleWith(low_value));
 
-  EXPECT_FALSE(high_value < high_value);
-  EXPECT_TRUE(high_value <= high_value);
   EXPECT_TRUE(high_value == high_value);
   EXPECT_FALSE(high_value != high_value);
-  EXPECT_TRUE(high_value >= high_value);
-  EXPECT_FALSE(high_value > high_value);
+  EXPECT_TRUE(high_value.IsCompatibleWith(high_value));
 }
 
 }  // namespace blink

third_party/blink/public/common/feature_policy/document_policy.h

@@ -109,10 +109,13 @@ class BLINK_COMMON_EXPORT DocumentPolicy {
       const DocumentPolicyFeatureState& policy,
       const DocumentPolicyFeatureInfoMap&);
 
-  // Merge two FeatureState map. Take stricter value when there is conflict.
+  // Merge two FeatureState map.
+  // When there is conflict:
+  // - take the stricter value if PolicyValue is comparable
+  // - take override_policy's value if PolicyValue is not comparable
   static DocumentPolicyFeatureState MergeFeatureState(
-      const DocumentPolicyFeatureState& policy1,
-      const DocumentPolicyFeatureState& policy2);
+      const DocumentPolicyFeatureState& base_policy,
+      const DocumentPolicyFeatureState& override_policy);
 
  private:
   friend class DocumentPolicyTest;

third_party/blink/public/common/feature_policy/policy_value.h

@@ -52,6 +52,12 @@ class BLINK_COMMON_EXPORT PolicyValue {
   void SetToMax();
   void SetToMin();
 
+  // Test whether this policy value is compatible with required policy value.
+  // Note: a.IsCompatibleWith(b) == true does not necessary indicate
+  // b.IsCompatibleWith(a) == false, because not all policy value types support
+  // strictness comparison, e.g. enum.
+  bool IsCompatibleWith(const PolicyValue& required) const;
+
  private:
   mojom::PolicyValueType type_;
   bool bool_value_ = false;
@@ -62,14 +68,6 @@ bool BLINK_COMMON_EXPORT operator==(const PolicyValue& lhs,
                                     const PolicyValue& rhs);
 bool BLINK_COMMON_EXPORT operator!=(const PolicyValue& lhs,
                                     const PolicyValue& rhs);
-bool BLINK_COMMON_EXPORT operator>(const PolicyValue& lhs,
-                                   const PolicyValue& rhs);
-bool BLINK_COMMON_EXPORT operator>=(const PolicyValue& lhs,
-                                    const PolicyValue& rhs);
-bool BLINK_COMMON_EXPORT operator<(const PolicyValue& lhs,
-                                   const PolicyValue& rhs);
-bool BLINK_COMMON_EXPORT operator<=(const PolicyValue& lhs,
-                                    const PolicyValue& rhs);
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_PUBLIC_COMMON_FEATURE_POLICY_POLICY_VALUE_H_

third_party/blink/renderer/core/loader/document_loader.cc

@@ -889,7 +889,7 @@ DocumentPolicy::ParsedDocumentPolicy DocumentLoader::CreateDocumentPolicy() {
             .value_or(DocumentPolicy::ParsedDocumentPolicy{})
             .feature_state;
     frame_->SetRequiredDocumentPolicy(DocumentPolicy::MergeFeatureState(
-        frame_policy_.required_document_policy, header_required_policy));
+        header_required_policy, frame_policy_.required_document_policy));
   }
 
   document_policy_parsing_messages_.AppendVector(header_logger.GetMessages());