Allow various font weights in gfx.

ui/base/resource/resource_bundle.cc

Index: ui/base/resource/resource_bundle.cc
diff --git a/ui/base/resource/resource_bundle.cc b/ui/base/resource/resource_bundle.cc
index 8afa15b608988d97be8981edc0f61467667a3c13..29f09bb951e55f2aa4cd59decaf991b57b354179 100644
--- a/ui/base/resource/resource_bundle.cc
+++ b/ui/base/resource/resource_bundle.cc
@@ -158,6 +158,29 @@ class ResourceBundle::ResourceBundleImageSource : public gfx::ImageSkiaSource {
   DISALLOW_COPY_AND_ASSIGN(ResourceBundleImageSource);
 };
 
+struct ResourceBundle::FontKey {
+  FontKey(int in_size_delta,
+          gfx::Font::FontStyle in_style,
+          gfx::Font::FontWeight in_weight)
+      : size_delta(in_size_delta), style(in_style), weight(in_weight) {}
+
+  ~FontKey() {}
+
+  bool operator==(const FontKey& rhs) const {
+    return size_delta == rhs.size_delta && style == rhs.style &&
+           weight == rhs.weight;
+  }
+
+  bool operator<(const FontKey& rhs) const {
+    return size_delta < rhs.size_delta || style < rhs.style ||

[msw, 2016/03/22 01:53:43]

This key comparator may be problematic, the converse doesn't hold.
If A = (1, 2, 3) and B = (1, 3, 2), then (A < B) is true *and* (B < A) is true.

Maybe some sort of hierarchical ordering would be better? eg:
if (size_delta < rhs.size_delta)
 return true;
if (size_delta == rhs.size_delta) {
  if (style < rhs.style)
    return true;
  if (style == rhs.style)
    return weight < rhs.weight;
}
return false;

[tapted, 2016/03/22 04:04:17]

yeah - comparators need to obey "strict weak ordering" or there can be obscure
crashes in std::map's balancing (sorting) algorithms, depending what's inserted.

I typically write these like

if (size_delta != rhs.size_delta)
  return size_delta < rhs.size_delta;
if (style != rhs.style)
  return style < rhs.style;
return weight < rhs.weight;

[Mikus, 2016/03/22 14:19:50]

Done.

[Mikus, 2016/03/22 14:19:50]

Done.

+           weight < rhs.weight;
+  }
+
+  int size_delta;
+  gfx::Font::FontStyle style;
+  gfx::Font::FontWeight weight;
+};
+
 // static
 std::string ResourceBundle::InitSharedInstanceWithLocale(
     const std::string& pref_locale,
@@ -533,36 +556,38 @@ base::string16 ResourceBundle::GetLocalizedString(int message_id) {
 
 const gfx::FontList& ResourceBundle::GetFontListWithDelta(
     int size_delta,
-    gfx::Font::FontStyle style) {
+    gfx::Font::FontStyle style,
+    gfx::Font::FontWeight weight) {
   base::AutoLock lock_scope(*images_and_fonts_lock_);
 
-  typedef std::pair<int, gfx::Font::FontStyle> Key;
-  const Key styled_key(size_delta, style);
+  const FontKey key = {size_delta, style, weight};

[tapted, 2016/03/22 04:04:17]

These will call the constructor using C++11 "Uniform initialization syntax"
which is currently on the banned feature list --
https://chromium-cpp.appspot.com/#banned_list

[Mikus, 2016/03/22 14:19:50]

But this is a POD object, we can use this for structs.

Take a look at (1) in
http://en.cppreference.com/w/cpp/language/aggregate_initialization

there's no 'since c++11' note

[tapted, 2016/03/22 23:31:32]

You can't use aggregate initialization if there's a constructor. This is easy to
verify 

#include <stdio.h>
struct A { int a; int b; A(int aa, int bb) : a(aa), b(bb) {
  printf("constructor!\n"); }
};
int main() {
  A a = {1, 2};
}

$ clang++ test.cc
test.cc:6:5: error: non-aggregate type 'A' cannot be initialized with an
initializer list

$ clang++ test.cc -std=c++11
$ ./a.out
constructor!


You could also just try adding a fourth, dummy argument to the FontKey
constructor and note the compile error.

 
-  auto found = font_cache_.find(styled_key);
+  auto found = font_cache_.find(key);
   if (found != font_cache_.end())
     return found->second;
 
-  const Key base_key(0, gfx::Font::NORMAL);
+  const FontKey base_key = {0, gfx::Font::NORMAL, gfx::Font::WEIGHT_NORMAL};
   gfx::FontList& base = font_cache_[base_key];
-  if (styled_key == base_key)
+  if (key == base_key)
     return base;
 
   // Fonts of a given style are derived from the unstyled font of the same size.
   // Cache the unstyled font by first inserting a default-constructed font list.
   // Then, derive it for the initial insertion, or use the iterator that points
   // to the existing entry that the insertion collided with.
-  const Key sized_key(size_delta, gfx::Font::NORMAL);
+  const FontKey sized_key = {size_delta, gfx::Font::NORMAL, weight};

[tapted, 2016/03/22 04:04:17]

This needs to pass gfx::Font::WEIGHT_NORMAL, not |weight|. Otherwise a font with
no weight change is getting stored using a key _with_ a weight, on line 581

[Mikus, 2016/03/22 14:19:50]

Done.

   auto sized = font_cache_.insert(std::make_pair(sized_key, gfx::FontList()));
   if (sized.second)
     sized.first->second = base.DeriveWithSizeDelta(size_delta);
-  if (styled_key == sized_key)
+  if (key == sized_key)

[tapted, 2016/03/22 04:04:17]

Unless I've missed something, this should go back how it was. That is, was there
a problem setting both style and weight only in the last step? I think that's
how it should be done, and is consistent with the comment above on line 574

[Mikus, 2016/03/22 14:19:50]

Done.

     return sized.first->second;
 
-  auto styled = font_cache_.insert(std::make_pair(styled_key, gfx::FontList()));
+  auto styled = font_cache_.insert(std::make_pair(sized_key, gfx::FontList()));

[tapted, 2016/03/22 04:04:17]

sized_key -> styled_key

   DCHECK(styled.second);  // Otherwise font_cache_.find(..) would have found it.
   styled.first->second = sized.first->second.DeriveWithStyle(
       sized.first->second.GetFontStyle() | style);
+  styled.first->second = sized.first->second.DeriveWithWeight(weight);

[msw, 2016/03/22 01:53:43]

Can/should we avoid the second derive call and just call:
FontList::Derive(0, sized.first->second.GetFontStyle() | style, weight)

[Mikus, 2016/03/22 14:19:50]

Done.

+
   return styled.first->second;
 }
 
@@ -572,10 +597,10 @@ const gfx::Font& ResourceBundle::GetFontWithDelta(int size_delta,
 }
 
 const gfx::FontList& ResourceBundle::GetFontList(FontStyle legacy_style) {
-  gfx::Font::FontStyle font_style = gfx::Font::NORMAL;
+  gfx::Font::FontWeight font_weight = gfx::Font::WEIGHT_NORMAL;
   if (legacy_style == BoldFont || legacy_style == SmallBoldFont ||
       legacy_style == MediumBoldFont || legacy_style == LargeBoldFont)
-    font_style = gfx::Font::BOLD;
+    font_weight = gfx::Font::WEIGHT_BOLD;
 
   int size_delta = 0;
   switch (legacy_style) {
@@ -596,7 +621,7 @@ const gfx::FontList& ResourceBundle::GetFontList(FontStyle legacy_style) {
       break;
   }
 
-  return GetFontListWithDelta(size_delta, font_style);
+  return GetFontListWithDelta(size_delta, gfx::Font::NORMAL, font_weight);
 }
 
 const gfx::Font& ResourceBundle::GetFont(FontStyle style) {