PpdProvider v3: migrate Restrictions struct

This change moves the Restrictions struct (public in the declaration
of PpdProvider) into the PPD metadata parsing code. This is not the
change where we cut away the v2 PpdProvider, though, so we retain the
old struct definition as PpdProvider::LegacyRestrictions.

In addition, this change modifies the semantics of Restrictions;
metadata that can be restricted now always contains Restrictions
structs, but the struct's members (max and min milestones) are now
optional.

Bug: chromium:888189
Test: chromeos_unittests
Change-Id: I03a3825ec2150e8db93a2187d11285afcd44b07a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2393615
Commit-Queue: Kalvin Lee <kdlee@chromium.org>
Reviewed-by: Luum Habtemariam <luum@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808603}

chromeos/printing/ppd_metadata_matchers.h

@@ -10,7 +10,6 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/version.h"
 #include "chromeos/printing/ppd_metadata_parser.h"
-#include "chromeos/printing/ppd_provider.h"
 #include "testing/gmock/include/gmock/gmock-matchers.h"
 
 #ifndef CHROMEOS_PRINTING_PPD_METADATA_MATCHERS_H_
@@ -21,14 +20,15 @@ namespace chromeos {
 using ::testing::Eq;
 using ::testing::ExplainMatchResult;
 using ::testing::Field;
+using ::testing::Optional;
 using ::testing::StrEq;
 
 MATCHER_P(RestrictionsWithMinMilestone,
           integral_min_milestone,
           "is a Restrictions with min_milestone ``" +
               base::NumberToString(integral_min_milestone) + "''") {
-  return ExplainMatchResult(Field(&PpdProvider::Restrictions::min_milestone,
-                                  Eq(base::Version(base::NumberToString(
+  return ExplainMatchResult(Field(&Restrictions::min_milestone,
+                                  Optional(base::Version(base::NumberToString(
                                       double{integral_min_milestone})))),
                             arg, result_listener);
 }
@@ -37,8 +37,8 @@ MATCHER_P(RestrictionsWithMaxMilestone,
           integral_max_milestone,
           "is a Restrictions with max_milestone ``" +
               base::NumberToString(integral_max_milestone) + "''") {
-  return ExplainMatchResult(Field(&PpdProvider::Restrictions::max_milestone,
-                                  Eq(base::Version(base::NumberToString(
+  return ExplainMatchResult(Field(&Restrictions::max_milestone,
+                                  Optional(base::Version(base::NumberToString(
                                       double{integral_max_milestone})))),
                             arg, result_listener);
 }
@@ -59,6 +59,16 @@ MATCHER_P2(RestrictionsWithMinAndMaxMilestones,
              result_listener);
 }
 
+MATCHER(UnboundedRestrictions,
+        "is a Restrictions with neither min nor max milestones") {
+  return ExplainMatchResult(
+             Field(&Restrictions::min_milestone, Eq(base::nullopt)), arg,
+             result_listener) &&
+         ExplainMatchResult(
+             Field(&Restrictions::max_milestone, Eq(base::nullopt)), arg,
+             result_listener);
+}
+
 // Matches a ReverseIndexLeaf struct against its |manufacturer| and
 // |model| members.
 MATCHER_P2(ReverseIndexLeafLike,

chromeos/printing/ppd_metadata_parser.cc

@@ -59,29 +59,17 @@ base::Optional<base::Value> ParseJsonAndUnnestKey(
   return unnested;
 }
 
-// Returns a well-formed Restrictions struct from a dictionary |value|.
-// A well-formed Restrictions struct has at least one member that
-// for which calling base::Version::IsValid() will evaluate true.
-base::Optional<PpdProvider::Restrictions> ParseRestrictionsFromValue(
-    const base::Value& value) {
+// Returns a Restrictions struct from a dictionary |value|.
+Restrictions ParseRestrictionsFromValue(const base::Value& value) {
+  Restrictions restrictions;
   auto min_as_double = value.FindDoubleKey("minMilestone");
   auto max_as_double = value.FindDoubleKey("maxMilestone");
-  if (!min_as_double.has_value() && !max_as_double.has_value()) {
-    return base::nullopt;
-  }
 
-  // While we don't want to deliberately store trivial base::Version
-  // members into the Restrictions struct, take heed that a
-  // calling IsValid() on a default-constructed base::Version returns
-  // false.
-  PpdProvider::Restrictions restrictions;
-  bool restrictions_is_nontrivial = false;
   if (min_as_double.has_value()) {
     base::Version min_milestone =
         base::Version(base::NumberToString(int{min_as_double.value()}));
     if (min_milestone.IsValid()) {
       restrictions.min_milestone = min_milestone;
-      restrictions_is_nontrivial = true;
     }
   }
   if (max_as_double.has_value()) {
@@ -89,14 +77,9 @@ base::Optional<PpdProvider::Restrictions> ParseRestrictionsFromValue(
         base::Version(base::NumberToString(int{max_as_double.value()}));
     if (max_milestone.IsValid()) {
       restrictions.max_milestone = max_milestone;
-      restrictions_is_nontrivial = true;
     }
   }
-
-  if (restrictions_is_nontrivial) {
-    return restrictions;
-  }
-  return base::nullopt;
+  return restrictions;
 }
 
 // Returns a ParsedPrinter from a leaf |value| from Printers metadata.
@@ -175,6 +158,11 @@ base::Optional<ParsedIndexValues> UnnestPpdMetadata(const base::Value& value) {
 
 }  // namespace
 
+Restrictions::Restrictions() = default;
+Restrictions::~Restrictions() = default;
+Restrictions::Restrictions(const Restrictions&) = default;
+Restrictions& Restrictions::operator=(const Restrictions&) = default;
+
 ParsedPrinter::ParsedPrinter() = default;
 ParsedPrinter::~ParsedPrinter() = default;
 ParsedPrinter::ParsedPrinter(const ParsedPrinter&) = default;

chromeos/printing/ppd_metadata_parser.h

@@ -21,14 +21,25 @@
 #include "base/containers/flat_map.h"
 #include "base/optional.h"
 #include "base/strings/string_piece_forward.h"
+#include "base/version.h"
 #include "chromeos/chromeos_export.h"
-#include "chromeos/printing/ppd_provider.h"
 
 #ifndef CHROMEOS_PRINTING_PPD_METADATA_PARSER_H_
 #define CHROMEOS_PRINTING_PPD_METADATA_PARSER_H_
 
 namespace chromeos {
 
+// Defines the limitations on when we show a particular PPD.
+struct Restrictions {
+  Restrictions();
+  ~Restrictions();
+  Restrictions(const Restrictions&);
+  Restrictions& operator=(const Restrictions&);
+
+  base::Optional<base::Version> min_milestone;
+  base::Optional<base::Version> max_milestone;
+};
+
 struct CHROMEOS_EXPORT ReverseIndexLeaf {
   std::string manufacturer;
   std::string model;
@@ -43,7 +54,7 @@ struct CHROMEOS_EXPORT ParsedPrinter {
 
   std::string user_visible_printer_name;
   std::string effective_make_and_model;
-  base::Optional<PpdProvider::Restrictions> restrictions;
+  Restrictions restrictions;
 };
 
 // A single leaf value parsed from a forward index.
@@ -54,7 +65,7 @@ struct CHROMEOS_EXPORT ParsedIndexLeaf {
   ParsedIndexLeaf& operator=(const ParsedIndexLeaf&);
 
   std::string ppd_basename;
-  base::Optional<PpdProvider::Restrictions> restrictions;
+  Restrictions restrictions;
   std::string license;
 };
 

chromeos/printing/ppd_metadata_parser_unittest.cc

@@ -239,19 +239,17 @@ TEST(PpdMetadataParserTest, CanParsePrintersWithRestrictions) {
   const auto parsed = ParsePrinters(kPrintersJson);
   ASSERT_TRUE(parsed.has_value());
 
-  EXPECT_THAT(
-      *parsed,
-      UnorderedElementsAre(
-          AllOf(ParsedPrinterLike("Schäfers Klagelied", "d 121"),
-                Field(&ParsedPrinter::restrictions,
-                      Optional(RestrictionsWithMinMilestone(121)))),
-          AllOf(ParsedPrinterLike("Meeres Stille", "d 216"),
-                Field(&ParsedPrinter::restrictions,
-                      Optional(RestrictionsWithMaxMilestone(216)))),
-          AllOf(
-              ParsedPrinterLike("Heidenröslein", "d 257"),
-              Field(&ParsedPrinter::restrictions,
-                    Optional(RestrictionsWithMinAndMaxMilestones(216, 257))))));
+  EXPECT_THAT(*parsed,
+              UnorderedElementsAre(
+                  AllOf(ParsedPrinterLike("Schäfers Klagelied", "d 121"),
+                        Field(&ParsedPrinter::restrictions,
+                              RestrictionsWithMinMilestone(121))),
+                  AllOf(ParsedPrinterLike("Meeres Stille", "d 216"),
+                        Field(&ParsedPrinter::restrictions,
+                              RestrictionsWithMaxMilestone(216))),
+                  AllOf(ParsedPrinterLike("Heidenröslein", "d 257"),
+                        Field(&ParsedPrinter::restrictions,
+                              RestrictionsWithMinAndMaxMilestones(216, 257)))));
 }
 
 // Verifies that ParsePrinters() can parse printers and ignore
@@ -273,10 +271,11 @@ TEST(PpdMetadataParserTest, CanParsePrintersWithMalformedRestrictions) {
   const auto parsed = ParsePrinters(kPrintersJson);
   ASSERT_TRUE(parsed.has_value());
 
-  EXPECT_THAT(*parsed,
-              UnorderedElementsAre(AllOf(
-                  ParsedPrinterLike("Jägers Abendlied", "d 368"),
-                  Field(&ParsedPrinter::restrictions, Eq(base::nullopt)))));
+  EXPECT_THAT(
+      *parsed,
+      UnorderedElementsAre(
+          AllOf(ParsedPrinterLike("Jägers Abendlied", "d 368"),
+                Field(&ParsedPrinter::restrictions, UnboundedRestrictions()))));
 }
 
 // Verifies that ParsePrinters() returns base::nullopt rather than an
@@ -445,27 +444,26 @@ TEST(PpdMetadataParserTest, CanParseForwardIndexWithRestrictions) {
 
   const auto parsed = ParseForwardIndex(kJsonForwardIndex);
   ASSERT_TRUE(parsed.has_value());
-  EXPECT_THAT(parsed.value(),
+  EXPECT_THAT(
+      parsed.value(),
+      UnorderedElementsAre(
+          ParsedIndexEntryLike(
+              "nähe des geliebten",
+              UnorderedElementsAre(
+                  AllOf(ParsedIndexLeafWithPpdBasename("d-162.ppd.gz"),
+                        Field(&ParsedIndexLeaf::restrictions,
+                              RestrictionsWithMinMilestone(25))))),
+          ParsedIndexEntryLike(
+              "der fischer", UnorderedElementsAre(AllOf(
+                                 ParsedIndexLeafWithPpdBasename("d-225.ppd.gz"),
+                                 Field(&ParsedIndexLeaf::restrictions,
+                                       RestrictionsWithMaxMilestone(35))))),
+          ParsedIndexEntryLike(
+              "erster verlust",
               UnorderedElementsAre(
-                  ParsedIndexEntryLike(
-                      "nähe des geliebten",
-                      UnorderedElementsAre(AllOf(
-                          ParsedIndexLeafWithPpdBasename("d-162.ppd.gz"),
-                          Field(&ParsedIndexLeaf::restrictions,
-                                Optional(RestrictionsWithMinMilestone(25)))))),
-                  ParsedIndexEntryLike(
-                      "der fischer",
-                      UnorderedElementsAre(AllOf(
-                          ParsedIndexLeafWithPpdBasename("d-225.ppd.gz"),
-                          Field(&ParsedIndexLeaf::restrictions,
-                                Optional(RestrictionsWithMaxMilestone(35)))))),
-                  ParsedIndexEntryLike(
-                      "erster verlust",
-                      UnorderedElementsAre(AllOf(
-                          ParsedIndexLeafWithPpdBasename("d-226.ppd.gz"),
-                          Field(&ParsedIndexLeaf::restrictions,
-                                Optional(RestrictionsWithMinAndMaxMilestones(
-                                    45, 46))))))));
+                  AllOf(ParsedIndexLeafWithPpdBasename("d-226.ppd.gz"),
+                        Field(&ParsedIndexLeaf::restrictions,
+                              RestrictionsWithMinAndMaxMilestones(45, 46)))))));
 }
 
 // Verifies that ParseForwardIndex() can parse forward index metadata
@@ -519,18 +517,18 @@ TEST(PpdMetadataParserTest, CanParseForwardIndexWithMalformedRestrictions) {
               UnorderedElementsAre(
                   AllOf(ParsedIndexLeafWithPpdBasename("d-162.ppd.gz"),
                         Field(&ParsedIndexLeaf::restrictions,
-                              Optional(RestrictionsWithMaxMilestone(25)))))),
+                              RestrictionsWithMaxMilestone(25))))),
           ParsedIndexEntryLike(
-              "der fischer",
-              UnorderedElementsAre(
-                  AllOf(ParsedIndexLeafWithPpdBasename("d-225.ppd.gz"),
-                        Field(&ParsedIndexLeaf::restrictions,
-                              Optional(RestrictionsWithMinMilestone(35)))))),
+              "der fischer", UnorderedElementsAre(AllOf(
+                                 ParsedIndexLeafWithPpdBasename("d-225.ppd.gz"),
+                                 Field(&ParsedIndexLeaf::restrictions,
+                                       RestrictionsWithMinMilestone(35))))),
           ParsedIndexEntryLike(
               "erster verlust",
-              UnorderedElementsAre(AllOf(
-                  ParsedIndexLeafWithPpdBasename("d-226.ppd.gz"),
-                  Field(&ParsedIndexLeaf::restrictions, Eq(base::nullopt)))))));
+              UnorderedElementsAre(
+                  AllOf(ParsedIndexLeafWithPpdBasename("d-226.ppd.gz"),
+                        Field(&ParsedIndexLeaf::restrictions,
+                              UnboundedRestrictions()))))));
 }
 
 // Verifies that ParseForwardIndex() can parse forward index metadata

chromeos/printing/ppd_provider.cc

@@ -64,7 +64,7 @@ struct ReverseIndexJSON {
   std::string model;
 
   // Restrictions for this manufacturer
-  PpdProvider::Restrictions restrictions;
+  PpdProvider::LegacyRestrictions restrictions;
 };
 
 struct PpdLicenseJSON {
@@ -84,7 +84,7 @@ struct ManufacturersJSON {
   std::string reference;
 
   // Restrictions for this manufacturer
-  PpdProvider::Restrictions restrictions;
+  PpdProvider::LegacyRestrictions restrictions;
 };
 
 // Holds a metadata_v2 printers response
@@ -96,7 +96,7 @@ struct PrintersJSON {
   std::string effective_make_and_model;
 
   // Restrictions for this printer
-  PpdProvider::Restrictions restrictions;
+  PpdProvider::LegacyRestrictions restrictions;
 };
 
 // Holds a metadata_v2 ppd-index response
@@ -276,9 +276,9 @@ std::string ComputeLicense(const base::Value& dict) {
 }
 
 // Constructs and returns a printers' restrictions parsed from |dict|.
-PpdProvider::Restrictions ComputeRestrictions(const base::Value& dict) {
+PpdProvider::LegacyRestrictions ComputeRestrictions(const base::Value& dict) {
   DCHECK(dict.is_dict());
-  PpdProvider::Restrictions restrictions;
+  PpdProvider::LegacyRestrictions restrictions;
 
   const base::Value* min_milestone =
       dict.FindKeyOfType({"min_milestone"}, base::Value::Type::DOUBLE);
@@ -301,7 +301,7 @@ PpdProvider::Restrictions ComputeRestrictions(const base::Value& dict) {
 // |current_version|.
 bool IsPrinterRestricted(const PrintersJSON& printer,
                          const base::Version& current_version) {
-  const PpdProvider::Restrictions& restrictions = printer.restrictions;
+  const PpdProvider::LegacyRestrictions& restrictions = printer.restrictions;
 
   if (restrictions.min_milestone != base::Version("0.0") &&
       restrictions.min_milestone > current_version) {

chromeos/printing/ppd_provider.h

@@ -107,8 +107,9 @@ class CHROMEOS_EXPORT PpdProvider : public base::RefCounted<PpdProvider> {
   };
 
   // Defines the limitations on when we show a particular PPD
-  // TODO(crbug.com/888189): this belongs in the parsing header.
-  struct Restrictions {
+  // Not to be confused with the new Restrictions struct used in the
+  // v3 PpdProvider, defined in ppd_metadata_parser.h
+  struct LegacyRestrictions {
     // Minimum milestone for ChromeOS build
     base::Version min_milestone = base::Version("0.0");
 

chromeos/printing/ppd_provider_v3.cc

@@ -419,9 +419,11 @@ class PpdProviderImpl : public PpdProvider {
   // |restrictions|.
   bool CurrentVersionSatisfiesRestrictions(
       const Restrictions& restrictions) const {
-    if ((restrictions.min_milestone.IsValid() &&
+    if ((restrictions.min_milestone.has_value() &&
+         restrictions.min_milestone.value().IsValid() &&
          version_ < restrictions.min_milestone) ||
-        (restrictions.max_milestone.IsValid() &&
+        (restrictions.max_milestone.has_value() &&
+         restrictions.max_milestone.value().IsValid() &&
          version_ > restrictions.max_milestone)) {
       return false;
     }
@@ -458,8 +460,7 @@ class PpdProviderImpl : public PpdProvider {
 
     ResolvedPrintersList printers_available_to_our_version;
     for (const ParsedPrinter& printer : printers) {
-      if (!printer.restrictions.has_value() ||
-          CurrentVersionSatisfiesRestrictions(printer.restrictions.value())) {
+      if (CurrentVersionSatisfiesRestrictions(printer.restrictions)) {
         Printer::PpdReference ppd_reference;
         ppd_reference.effective_make_and_model =
             printer.effective_make_and_model;
@@ -488,9 +489,7 @@ class PpdProviderImpl : public PpdProvider {
     }
 
     for (const ParsedIndexLeaf& index_leaf : iter->second.values) {
-      if (!index_leaf.restrictions.has_value() ||
-          CurrentVersionSatisfiesRestrictions(
-              index_leaf.restrictions.value())) {
+      if (CurrentVersionSatisfiesRestrictions(index_leaf.restrictions)) {
         return &index_leaf;
       }
     }