Convert FontMatchingMetrics to share key and value types in its HashMaps

Currently, each HashMap uses a unique type to represent each key and
value. These types are then converted to IdentifiableTokens just before
metrics are recorded. These types (mainly structs) can be confusing,
with fields remaining unused in certain code paths.

We convert FontMatchingMetrics to instead use an IdentifiableToken as
both its key and value, simplifying the work at publish time and
improving readability. To enable its use as a key, the IdentifiableToken
is wrapped in a simple struct.

This change does not change functionality, although the change in token
generation method will affect the exact hashes recorded. This new
approach will simplify a future refactor that does make functional
changes (see crrev.com/c/2376037).

Bug: 1121669
Change-Id: I6c7ba2b6f42c62d7ae007d2c9b9519323edc24cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2388002
Commit-Queue: Alex Turner <alexmt@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803978}

third_party/blink/public/common/privacy_budget/identifiability_metrics.h

@@ -111,6 +111,10 @@ uint64_t IdentifiabilityDigestHelper(T in, Targs... extra_in) {
       base::make_span(reinterpret_cast<uint8_t*>(digests), sizeof(digests)));
 }
 
+// The zero-length digest, i.e. the digest computed for no bytes.
+static constexpr uint64_t kIdentifiabilityDigestOfNoBytes =
+    0x9ae16a3b2f90404fULL;
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_PUBLIC_COMMON_PRIVACY_BUDGET_IDENTIFIABILITY_METRICS_H_

third_party/blink/public/common/privacy_budget/identifiable_token.h

@@ -95,6 +95,9 @@ class IdentifiableToken {
   // Representation type of the sample.
   using TokenType = int64_t;
 
+  // Required for use in certain data structures. Represents no bytes.
+  constexpr IdentifiableToken() : value_(kIdentifiabilityDigestOfNoBytes) {}
+
   // A byte buffer specified as a span.
   //
   // This is essentially the base case. If it were the base case, then

third_party/blink/renderer/platform/fonts/font_matching_metrics.cc

@@ -7,8 +7,11 @@
 #include "services/metrics/public/cpp/metrics_utils.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
+#include "third_party/blink/public/common/privacy_budget/identifiability_metric_builder.h"
 #include "third_party/blink/public/common/privacy_budget/identifiability_study_settings.h"
+#include "third_party/blink/public/common/privacy_budget/identifiable_surface.h"
 #include "third_party/blink/public/common/privacy_budget/identifiable_token.h"
+#include "third_party/blink/public/common/privacy_budget/identifiable_token_builder.h"
 #include "third_party/blink/renderer/platform/fonts/font_global_context.h"
 #include "third_party/blink/renderer/platform/privacy_budget/identifiability_digest_helpers.h"
 
@@ -92,12 +95,17 @@ void FontMatchingMetrics::ReportFontLookupByUniqueOrFamilyName(
     return;
   }
   OnFontLookup();
-  LocalFontLookupKey key(name, font_description.GetFontSelectionRequest());
-  if (font_lookups_.Contains(key))
+
+  IdentifiableTokenBuilder builder;
+  builder.AddToken(IdentifiabilityBenignStringToken(name))
+      .AddValue(font_description.GetFontSelectionRequest().GetHash());
+  IdentifiableTokenKey input_key(builder.GetToken());
+
+  if (font_lookups_.Contains(input_key))
     return;
-  int64_t hash = GetHashForFontData(resulting_font_data);
-  LocalFontLookupResult result{hash, check_type, is_loading_fallback};
-  font_lookups_.insert(key, result);
+  IdentifiableToken output_token(GetHashForFontData(resulting_font_data),
+                                 check_type, is_loading_fallback);
+  font_lookups_.insert(input_key, output_token);
 }
 
 void FontMatchingMetrics::ReportFontLookupByFallbackCharacter(
@@ -109,14 +117,17 @@ void FontMatchingMetrics::ReportFontLookupByFallbackCharacter(
     return;
   }
   OnFontLookup();
-  LocalFontLookupKey key(fallback_character,
-                         font_description.GetFontSelectionRequest());
-  if (font_lookups_.Contains(key))
+
+  IdentifiableTokenBuilder builder;
+  builder.AddValue(fallback_character)
+      .AddValue(font_description.GetFontSelectionRequest().GetHash());
+  IdentifiableTokenKey input_key(builder.GetToken());
+
+  if (font_lookups_.Contains(input_key))
     return;
-  int64_t hash = GetHashForFontData(resulting_font_data);
-  LocalFontLookupResult result{hash, check_type,
-                               false /* is_loading_fallback */};
-  font_lookups_.insert(key, result);
+  IdentifiableToken output_token(GetHashForFontData(resulting_font_data),
+                                 check_type, false);
+  font_lookups_.insert(input_key, output_token);
 }
 
 void FontMatchingMetrics::ReportLastResortFallbackFontLookup(
@@ -127,13 +138,16 @@ void FontMatchingMetrics::ReportLastResortFallbackFontLookup(
     return;
   }
   OnFontLookup();
-  LocalFontLookupKey key(font_description.GetFontSelectionRequest());
-  if (font_lookups_.Contains(key))
+
+  IdentifiableTokenBuilder builder;
+  builder.AddValue(font_description.GetFontSelectionRequest().GetHash());
+  IdentifiableTokenKey input_key(builder.GetToken());
+
+  if (font_lookups_.Contains(input_key))
     return;
-  int64_t hash = GetHashForFontData(resulting_font_data);
-  LocalFontLookupResult result{hash, check_type,
-                               false /* is_loading_fallback */};
-  font_lookups_.insert(key, result);
+  IdentifiableToken output_token(GetHashForFontData(resulting_font_data),
+                                 check_type, false);
+  font_lookups_.insert(input_key, output_token);
 }
 
 void FontMatchingMetrics::ReportFontFamilyLookupByGenericFamily(
@@ -145,10 +159,15 @@ void FontMatchingMetrics::ReportFontFamilyLookupByGenericFamily(
     return;
   }
   OnFontLookup();
-  GenericFontLookupKey key(generic_font_family_name, script,
-                           generic_family_type);
-  generic_font_lookups_.insert(key,
-                               AtomicStringHash::GetHash(resulting_font_name));
+
+  IdentifiableTokenBuilder builder;
+  builder.AddToken(IdentifiabilityBenignStringToken(generic_font_family_name))
+      .AddToken(IdentifiableToken(script))
+      .AddToken(IdentifiableToken(generic_family_type));
+  IdentifiableTokenKey input_key(builder.GetToken());
+
+  generic_font_lookups_.insert(
+      input_key, IdentifiabilityBenignStringToken(resulting_font_name));
 }
 
 void FontMatchingMetrics::PublishIdentifiabilityMetrics() {
@@ -157,31 +176,18 @@ void FontMatchingMetrics::PublishIdentifiabilityMetrics() {
   IdentifiabilityMetricBuilder builder(source_id_);
 
   for (const auto& entry : font_lookups_) {
-    const LocalFontLookupKey& key = entry.key;
-    const LocalFontLookupResult& result = entry.value;
-
-    IdentifiableToken input_token(key.name_hash, key.fallback_character,
-                                  key.font_selection_request_hash);
-    IdentifiableToken output_token(result.hash, result.check_type,
-                                   result.is_loading_fallback);
-
-    builder.Set(IdentifiableSurface::FromTypeAndToken(
-                    IdentifiableSurface::Type::kLocalFontLookup, input_token),
-                output_token);
+    builder.Set(
+        IdentifiableSurface::FromTypeAndToken(
+            IdentifiableSurface::Type::kLocalFontLookup, entry.key.token),
+        entry.value);
   }
   font_lookups_.clear();
 
   for (const auto& entry : generic_font_lookups_) {
-    const GenericFontLookupKey& key = entry.key;
-    const unsigned& result = entry.value;
-
-    IdentifiableToken input_token(key.generic_font_family_name_hash, key.script,
-                                  key.generic_family_type);
-    IdentifiableToken output_token(result);
-
-    builder.Set(IdentifiableSurface::FromTypeAndToken(
-                    IdentifiableSurface::Type::kGenericFontLookup, input_token),
-                output_token);
+    builder.Set(
+        IdentifiableSurface::FromTypeAndToken(
+            IdentifiableSurface::Type::kGenericFontLookup, entry.key.token),
+        entry.value);
   }
   generic_font_lookups_.clear();
 

third_party/blink/renderer/platform/fonts/font_matching_metrics.h

@@ -6,16 +6,14 @@
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_FONTS_FONT_MATCHING_METRICS_H_
 
 #include "services/metrics/public/cpp/ukm_source_id.h"
-#include "third_party/blink/public/common/privacy_budget/identifiability_metric_builder.h"
-#include "third_party/blink/public/common/privacy_budget/identifiability_metrics.h"
-#include "third_party/blink/public/common/privacy_budget/identifiable_surface.h"
+#include "third_party/blink/public/common/privacy_budget/identifiable_token.h"
 #include "third_party/blink/renderer/platform/fonts/font_description.h"
 #include "third_party/blink/renderer/platform/fonts/simple_font_data.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
 #include "third_party/blink/renderer/platform/timer.h"
+#include "third_party/blink/renderer/platform/wtf/hash_functions.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
 #include "third_party/blink/renderer/platform/wtf/text/atomic_string.h"
-#include "third_party/blink/renderer/platform/wtf/text/atomic_string_hash.h"
 
 namespace ukm {
 class UkmRecorder;
@@ -23,58 +21,56 @@ class UkmRecorder;
 
 namespace blink {
 
-struct LocalFontLookupKey {
-  unsigned name_hash{0};
-  UChar32 fallback_character{-1};
-  unsigned font_selection_request_hash{0};
-  bool is_deleted_value_{false};
-
-  LocalFontLookupKey() = default;
-  LocalFontLookupKey(const AtomicString& name,
-                     FontSelectionRequest font_selection_request)
-      : name_hash(AtomicStringHash::GetHash(name)),
-        font_selection_request_hash(font_selection_request.GetHash()) {}
-
-  LocalFontLookupKey(UChar32 fallback_character,
-                     FontSelectionRequest font_selection_request)
-      : fallback_character(fallback_character),
-        font_selection_request_hash(font_selection_request.GetHash()) {}
-
-  explicit LocalFontLookupKey(FontSelectionRequest font_selection_request)
-      : font_selection_request_hash(font_selection_request.GetHash()) {}
-
-  explicit LocalFontLookupKey(WTF::HashTableDeletedValueType)
-      : is_deleted_value_(true) {}
-
-  bool IsHashTableDeletedValue() const { return is_deleted_value_; }
-
-  bool operator==(const LocalFontLookupKey& other) const {
-    return name_hash == other.name_hash &&
-           fallback_character == other.fallback_character &&
-           font_selection_request_hash == other.font_selection_request_hash &&
-           is_deleted_value_ == other.is_deleted_value_;
+// A (generic) wrapper around IdentifiableToken to enable its use as a HashMap
+// key. The |token| represents the parameters by which a font was looked up.
+// However, if |is_deleted_value| or |is_empty_value|, this key represents an
+// object for HashMap's internal use only. In that case, |token| is left as a
+// default value.
+struct IdentifiableTokenKey {
+  IdentifiableToken token;
+  bool is_deleted_value = false;
+  bool is_empty_value = false;
+
+  IdentifiableTokenKey() : is_empty_value(true) {}
+  explicit IdentifiableTokenKey(const IdentifiableToken& token)
+      : token(token) {}
+  explicit IdentifiableTokenKey(WTF::HashTableDeletedValueType)
+      : is_deleted_value(true) {}
+
+  bool IsHashTableDeletedValue() const { return is_deleted_value; }
+
+  bool operator==(const IdentifiableTokenKey& other) const {
+    return token == other.token && is_deleted_value == other.is_deleted_value &&
+           is_empty_value == other.is_empty_value;
+  }
+  bool operator!=(const IdentifiableTokenKey& other) const {
+    return !(*this == other);
   }
 };
 
-struct LocalFontLookupKeyHash {
-  STATIC_ONLY(LocalFontLookupKeyHash);
-  static unsigned GetHash(const LocalFontLookupKey& key) {
-    unsigned hash_codes[4] = {key.name_hash, key.fallback_character,
-                              key.font_selection_request_hash,
-                              key.is_deleted_value_};
-    return StringHasher::HashMemory<sizeof(hash_codes)>(hash_codes);
+// A helper that defines the hash and equality functions that HashMap should use
+// internally for comparing IdentifiableTokenKeys.
+struct IdentifiableTokenKeyHash {
+  STATIC_ONLY(IdentifiableTokenKeyHash);
+  static unsigned GetHash(const IdentifiableTokenKey& key) {
+    IntHash<int64_t> hasher;
+    return hasher.GetHash(key.token.ToUkmMetricValue()) ^
+           hasher.GetHash((key.is_deleted_value << 1) + key.is_empty_value);
   }
-  static bool Equal(const LocalFontLookupKey& a, const LocalFontLookupKey& b) {
+  static bool Equal(const IdentifiableTokenKey& a,
+                    const IdentifiableTokenKey& b) {
     return a == b;
   }
-
   static const bool safe_to_compare_to_empty_or_deleted = true;
 };
 
-struct LocalFontLookupKeyHashTraits
-    : WTF::SimpleClassHashTraits<LocalFontLookupKey> {
-  STATIC_ONLY(LocalFontLookupKeyHashTraits);
+// A helper that defines the invalid 'empty value' that HashMap should use
+// internally.
+struct IdentifiableTokenKeyHashTraits
+    : WTF::SimpleClassHashTraits<IdentifiableTokenKey> {
+  STATIC_ONLY(IdentifiableTokenKeyHashTraits);
   static const bool kEmptyValueIsZero = false;
+  static IdentifiableTokenKey EmptyValue() { return IdentifiableTokenKey(); }
 };
 
 enum class LocalFontLookupType {
@@ -88,62 +84,6 @@ enum class LocalFontLookupType {
   kLastResortInFontFallbackIterator,
 };
 
-struct LocalFontLookupResult {
-  int64_t hash;  // 0 if font was not found
-  LocalFontLookupType check_type;
-  bool is_loading_fallback;
-};
-
-struct GenericFontLookupKey {
-  unsigned generic_font_family_name_hash;
-  UScriptCode script{UScriptCode::USCRIPT_INVALID_CODE};
-  FontDescription::GenericFamilyType generic_family_type;
-  bool is_deleted_value_{false};
-
-  GenericFontLookupKey() = default;
-  GenericFontLookupKey(const AtomicString& generic_font_family_name,
-                       UScriptCode script,
-                       FontDescription::GenericFamilyType generic_family_type)
-      : generic_font_family_name_hash(
-            AtomicStringHash::GetHash(generic_font_family_name)),
-        script(script),
-        generic_family_type(generic_family_type) {}
-
-  explicit GenericFontLookupKey(WTF::HashTableDeletedValueType)
-      : is_deleted_value_(true) {}
-
-  bool IsHashTableDeletedValue() const { return is_deleted_value_; }
-
-  bool operator==(const GenericFontLookupKey& other) const {
-    return generic_font_family_name_hash ==
-               other.generic_font_family_name_hash &&
-           script == other.script &&
-           generic_family_type == other.generic_family_type &&
-           is_deleted_value_ == other.is_deleted_value_;
-  }
-};
-
-struct GenericFontLookupKeyHash {
-  STATIC_ONLY(GenericFontLookupKeyHash);
-  static unsigned GetHash(const GenericFontLookupKey& key) {
-    unsigned hash_codes[4] = {key.generic_font_family_name_hash, key.script,
-                              key.generic_family_type, key.is_deleted_value_};
-    return StringHasher::HashMemory<sizeof(hash_codes)>(hash_codes);
-  }
-  static bool Equal(const GenericFontLookupKey& a,
-                    const GenericFontLookupKey& b) {
-    return a == b;
-  }
-
-  static const bool safe_to_compare_to_empty_or_deleted = true;
-};
-
-struct GenericFontLookupKeyHashTraits
-    : WTF::SimpleClassHashTraits<GenericFontLookupKey> {
-  STATIC_ONLY(GenericFontLookupKeyHashTraits);
-  static const bool kEmptyValueIsZero = false;
-};
-
 // Tracks and reports UKM metrics of attempted font family match attempts (both
 // successful and not successful) by the current frame.
 //
@@ -262,16 +202,17 @@ class PLATFORM_EXPORT FontMatchingMetrics {
   // otherwise.
   const bool top_level_ = false;
 
-  HashMap<LocalFontLookupKey,
-          LocalFontLookupResult,
-          LocalFontLookupKeyHash,
-          LocalFontLookupKeyHashTraits>
-      font_lookups_;
-  HashMap<GenericFontLookupKey,
-          unsigned,
-          GenericFontLookupKeyHash,
-          GenericFontLookupKeyHashTraits>
-      generic_font_lookups_;
+  // This HashMap generically stores details of font lookups, i.e. what was used
+  // to search for the font, and what the resulting font was. The key is an
+  // IdentifiableTokenKey representing a wrapper around a digest of the lookup
+  // parameters. The value is an IdentifiableToken representing either a digest
+  // of the returned typeface or 0, if no valid typeface was found.
+  using TokenToTokenHashMap = HashMap<IdentifiableTokenKey,
+                                      IdentifiableToken,
+                                      IdentifiableTokenKeyHash,
+                                      IdentifiableTokenKeyHashTraits>;
+  TokenToTokenHashMap font_lookups_;
+  TokenToTokenHashMap generic_font_lookups_;
 
   ukm::UkmRecorder* const ukm_recorder_;
   const ukm::SourceId source_id_;