Allow various font weights in gfx.

ui/base/resource/resource_bundle.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/base/resource/resource_bundle.h"
 
 #include <stdint.h>
 
 #include <limits>
 #include <utility>
@@ skipping 140 matching lines @@
     return gfx::ImageSkiaRep(image, scale);
   }
 
  private:
   ResourceBundle* rb_;
   const int resource_id_;
 
   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,
     Delegate* delegate,
     LoadResources load_resources) {
   InitSharedInstance(delegate);
   if (load_resources == LOAD_COMMON_RESOURCES)
     g_shared_instance_->LoadCommonResources();
   std::string result = g_shared_instance_->LoadLocaleResources(pref_locale);
   g_shared_instance_->InitDefaultFontList();
@@ skipping 355 matching lines @@
     msg = base::string16(reinterpret_cast<const base::char16*>(data.data()),
                          data.length() / 2);
   } else if (encoding == ResourceHandle::UTF8) {
     msg = base::UTF8ToUTF16(data);
   }
   return msg;
 }
 
 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;
 }
 
 const gfx::Font& ResourceBundle::GetFontWithDelta(int size_delta,
                                                   gfx::Font::FontStyle style) {
   return GetFontListWithDelta(size_delta, style).GetPrimaryFont();
 }
 
 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) {
     case SmallFont:
     case SmallBoldFont:
       size_delta = kSmallFontDelta;
       break;
     case MediumFont:
     case MediumBoldFont:
       size_delta = kMediumFontDelta;
       break;
     case LargeFont:
     case LargeBoldFont:
       size_delta = kLargeFontDelta;
       break;
     case BaseFont:
     case BoldFont:
       break;
   }
 
-  return GetFontListWithDelta(size_delta, font_style);
+  return GetFontListWithDelta(size_delta, gfx::Font::NORMAL, font_weight);
 }
 
 const gfx::Font& ResourceBundle::GetFont(FontStyle style) {
   return GetFontList(style).GetPrimaryFont();
 }
 
 void ResourceBundle::ReloadFonts() {
   base::AutoLock lock_scope(*images_and_fonts_lock_);
   InitDefaultFontList();
   font_cache_.clear();
@@ skipping 273 matching lines @@
 // static
 bool ResourceBundle::DecodePNG(const unsigned char* buf,
                                size_t size,
                                SkBitmap* bitmap,
                                bool* fell_back_to_1x) {
   *fell_back_to_1x = PNGContainsFallbackMarker(buf, size);
   return gfx::PNGCodec::Decode(buf, size, bitmap);
 }
 
 }  // namespace ui