Allow various font weights in gfx.

Allow various font weights in gfx. These changes make Chromium's gfx::Font more closely match native font APIs &
capabilities.

BUG=597533
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/998e890d117912669e3d31ff21b465ee3684966c
Cr-Commit-Position: refs/heads/master@{#397368}

[Mikus, 2016-03-21 14:38:03]

Description was changed from

==========
Allow various font weights in gfx.

BUG=
==========

to

==========
Allow various font weights in gfx.

BUG=
==========

[Mikus, 2016-03-21 14:38:03]

mboc@opera.com changed reviewers:
+ msw@chromium.org, sky@chromium.org

[Mikus, 2016-03-21 14:39:55]

How do I commit a joint Skia/views patch? This code uses the method
SkTypeface::CreateFromNameAndStyle I created.

[Mikus, 2016-03-21 14:40:49]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 14:41:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/1

[commit-bot: I haz the power, 2016-03-21 14:41:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-21 14:41:20]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[sky, 2016-03-21 17:13:22]

Skia is its own repo. I believe https://skia.googlesource.com/skia, but I could
be wrong. You'll need to land the skia changes first, roll into chrome, and then
get this reviewed.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font.h
File ui/gfx/font.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font.h#newcode41
ui/gfx/font.h:41: enum FontWeight {
enum class and just Weight. When you move to enum class you can get rid of
WEIGHT_ in the name too. So, bold becomes Font::Weight::BOLD.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc
File ui/gfx/font_list_impl.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:22: int weight) {
Can this takes a Font::FontWeight?

[msw, 2016-03-22 01:53:44]

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:175: return size_delta < rhs.size_delta ||
style < rhs.style ||
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;

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:589: styled.first->second =
sized.first->second.DeriveWithWeight(weight);
Can/should we avoid the second derive call and just call:
FontList::Derive(0, sized.first->second.GetFontStyle() | style, weight)

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/canvas_skia.cc#newco...
ui/gfx/canvas_skia.cc:126: render_text->SetWeight(font_list.GetFontWeight());
nit: move below SetStyle calls

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.cc#newcode64
ui/gfx/font_list.cc:64: // Font supports ITALIC; underline is supported via
RenderText.
nit: "ITALIC and weights"

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h
File ui/gfx/font_list.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h#newcode31
ui/gfx/font_list.h:31: //   "Italic Ultra-Light Light Normal Semi-Bold Bold
Ultra-Bold Heavy" are
nit: use quotes for each style, right? (ie. "Italic", "Ultra-Light", "Light"...)

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h#newcode47
ui/gfx/font_list.h:47: gfx::Font::FontWeight* weight);
nit: rename |weight_out|, and mention it in the comment

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h#newcode60
ui/gfx/font_list.h:60: // Creates a font list from font names, styles and size.
nit: "styles, size, and weight."

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h#newcode88
ui/gfx/font_list.h:88: // Returns a new FontList with the same font names but
resized and the given
nit: "and with the given style and weight"

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc
File ui/gfx/font_list_impl.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:39: description += "Normal ";
Can/should we omit normal here?

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:62: 
nit: remove blank line

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:95: font_weight_(gfx::Font::WEIGHT_INVALID) {
Shouldn't this init to |font_weight|?

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:142: int old_style;
I find it weird that this code ignores the description's style...

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.h
File ui/gfx/font_list_impl.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.h#new...
ui/gfx/font_list_impl.h:49: // Returns a new FontListImpl with the same font
names but resized and the
ditto nit: "and with the given style and weight."

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_unittest.cc
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_unittest.c...
ui/gfx/font_list_unittest.cc:121: Fonts_FromDescStringInFlexibleFormat
nit: leave 4-space indent, i think.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_unittest.c...
ui/gfx/font_list_unittest.cc:176: FontList(font.Derive(2, Font::NORMAL,
Font::WEIGHT_BOLD));
This likely doesn't compile now... restore the "FontList font_list ="

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_linux.cc
File ui/gfx/platform_font_linux.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_linux....
ui/gfx/platform_font_linux.cc:103: delegate->GetDefaultFontDescription(&family,
&size_pixels, &style,
Should this also get the weight from the system UI? Or should this check if
style specifies BOLD, and set weight from that? You probably need to update
chrome/browser/ui/libgtk2ui/gtk2_ui.cc

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win.cc...
ui/gfx/platform_font_win.cc:164: int font_style,
This only concerns italic now... change to |bool italic|?

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win.cc...
ui/gfx/platform_font_win.cc:571: skia_style |= SkTypeface::kBold;
This is ignored by GetMatchingDirectWriteFont now; update.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win.h
File ui/gfx/platform_font_win.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win.h#...
ui/gfx/platform_font_win.h:57: Font DeriveFontWithHeight(int height,
This seems to only be called from unit tests. Can you avoid updating it, and
maybe even remove it?

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win_un...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win_un...
ui/gfx/platform_font_win_unittest.cc:40: EXPECT_GT(derived_font.Derive(1, 0,
gfx::Font::WEIGHT_NORMAL).GetHeight(),
nint: shouldn't this be bold to match the old behavior?

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:96: SkTypeface::Style
ConvertFontStyleToSkiaTypefaceStyle(int font_style) {
Remove this function if it's no longer used.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:98: // skia_style |= (font_style & Font::BOLD) ?
SkTypeface::kBold : 0;
Remove this

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:438: style & gfx::Font::ITALIC
nit: if |style| only handles italic, make it a bool flag.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.h#newcode65
ui/gfx/render_text.h:65: void SetFontWithStyle(const Font& font,
nit: rename |SetFont|

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:195: // Creates an SkTypeface from a font and a
|gfx::Font::FontStyle|.
nit: "font, |gfx::Font::FontStyle| and weight."

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1267: run->font_style = (style.style(ITALIC) ?
Font::ITALIC : 0);
nit: convert to bool flag for italic.

https://codereview.chromium.org/1819753003/diff/1/ui/views/controls/button/la...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/views/controls/button/la...
ui/views/controls/button/label_button.cc:48: if (font_list.Get().GetFontWeight()
!= gfx::Font::WEIGHT_BOLD) {
Hmm, I wonder if this should be < (or something smarter), in case we're now
respecting the system weight values and the user's system font is bolder, this
would yield something with less weight... Similar below for SetFontList?

https://codereview.chromium.org/1819753003/diff/1/ui/views/controls/tabbed_pa...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:115: kInactiveWeight))),
Shouldn't this be kActiveWeight to match the old behavior?

[tapted, 2016-03-22 04:04:16]

tapted@chromium.org changed reviewers:
+ tapted@chromium.org

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

drive-by (noticed a few things, but didn't do a thorough review pass)

https://codereview.chromium.org/1819753003/diff/1/chrome/browser/ui/views/acc...
File chrome/browser/ui/views/accessibility/invert_bubble_view.cc (right):

https://codereview.chromium.org/1819753003/diff/1/chrome/browser/ui/views/acc...
chrome/browser/ui/views/accessibility/invert_bubble_view.cc:82:
original_font_list.Derive(2, gfx::Font::NORMAL, gfx::Font::WEIGHT_BOLD));
Maybe not for this CL, but this is a good example of something that should be
updated to the new `GetFontListWithDelta` style accessors... 2 is a magic number
here. `kMediumFontDelta + 2` is 5, so there should be a 

const int kInvertBubbleTitleFontDelta = 5;

then this should be something like

ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
const gfx::FontList& font = rb.GetFontListWithDelta(
    kInvertBubbleTitleFontDelta,
    gfx::Font::NORMAL,
    gfx::Font::WEIGHT_BOLD);
views::Label* title = new views::Label(base::string16(), font);

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:175: return size_delta < rhs.size_delta ||
style < rhs.style ||
On 2016/03/22 01:53:43, msw wrote:
> 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;

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;

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:563: const FontKey key = {size_delta, style,
weight};
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

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:578: const FontKey sized_key = {size_delta,
gfx::Font::NORMAL, weight};
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

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:582: if (key == sized_key)
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

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:585: auto styled =
font_cache_.insert(std::make_pair(sized_key, gfx::FontList()));
sized_key -> styled_key

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.h
File ui/gfx/font_list_impl.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.h#new...
ui/gfx/font_list_impl.h:16: class Font;
nit: this is redundant now

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font.h
File ui/gfx/platform_font.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font.h#newc...
ui/gfx/platform_font.h:51: virtual gfx::Font::FontWeight GetWeight() = 0;
You'll need a PlatformFontMac change for this too

[Mikus, 2016-03-22 14:19:20]

Description was changed from

==========
Allow various font weights in gfx.

BUG=
==========

to

==========
Allow various font weights in gfx.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Mikus, 2016-03-22 14:19:52]

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:175: return size_delta < rhs.size_delta ||
style < rhs.style ||
On 2016/03/22 04:04:17, tapted wrote:
> On 2016/03/22 01:53:43, msw wrote:
> > 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;
> 
> 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;
> 

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:175: return size_delta < rhs.size_delta ||
style < rhs.style ||
On 2016/03/22 01:53:43, msw wrote:
> 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;

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:563: const FontKey key = {size_delta, style,
weight};
On 2016/03/22 04:04:17, tapted wrote:
> 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

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

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:578: const FontKey sized_key = {size_delta,
gfx::Font::NORMAL, weight};
On 2016/03/22 04:04:17, tapted wrote:
> 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

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:582: if (key == sized_key)
On 2016/03/22 04:04:17, tapted wrote:
> 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

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:589: styled.first->second =
sized.first->second.DeriveWithWeight(weight);
On 2016/03/22 01:53:43, msw wrote:
> Can/should we avoid the second derive call and just call:
> FontList::Derive(0, sized.first->second.GetFontStyle() | style, weight)

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/canvas_skia.cc#newco...
ui/gfx/canvas_skia.cc:126: render_text->SetWeight(font_list.GetFontWeight());
On 2016/03/22 01:53:43, msw wrote:
> nit: move below SetStyle calls

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.cc#newcode64
ui/gfx/font_list.cc:64: // Font supports ITALIC; underline is supported via
RenderText.
On 2016/03/22 01:53:43, msw wrote:
> nit: "ITALIC and weights"

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h
File ui/gfx/font_list.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h#newcode31
ui/gfx/font_list.h:31: //   "Italic Ultra-Light Light Normal Semi-Bold Bold
Ultra-Bold Heavy" are
On 2016/03/22 01:53:43, msw wrote:
> nit: use quotes for each style, right? (ie. "Italic", "Ultra-Light",
"Light"...)

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h#newcode47
ui/gfx/font_list.h:47: gfx::Font::FontWeight* weight);
On 2016/03/22 01:53:43, msw wrote:
> nit: rename |weight_out|, and mention it in the comment

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list.h#newcode88
ui/gfx/font_list.h:88: // Returns a new FontList with the same font names but
resized and the given
On 2016/03/22 01:53:43, msw wrote:
> nit: "and with the given style and weight"

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc
File ui/gfx/font_list_impl.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:39: description += "Normal ";
On 2016/03/22 01:53:43, msw wrote:
> Can/should we omit normal here?

I guess there'd be nothing wrong with that.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:95: font_weight_(gfx::Font::WEIGHT_INVALID) {
On 2016/03/22 01:53:43, msw wrote:
> Shouldn't this init to |font_weight|?

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:142: int old_style;
On 2016/03/22 01:53:43, msw wrote:
> I find it weird that this code ignores the description's style...

It's just like with old_style, it's ignored as well. Since the weight is derived
from the style, it can be safely ignored as well.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.h
File ui/gfx/font_list_impl.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.h#new...
ui/gfx/font_list_impl.h:16: class Font;
On 2016/03/22 04:04:17, tapted wrote:
> nit: this is redundant now

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_unittest.cc
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_unittest.c...
ui/gfx/font_list_unittest.cc:121: Fonts_FromDescStringInFlexibleFormat
On 2016/03/22 01:53:43, msw wrote:
> nit: leave 4-space indent, i think.

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_unittest.c...
ui/gfx/font_list_unittest.cc:176: FontList(font.Derive(2, Font::NORMAL,
Font::WEIGHT_BOLD));
On 2016/03/22 01:53:43, msw wrote:
> This likely doesn't compile now... restore the "FontList font_list ="

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win.cc...
ui/gfx/platform_font_win.cc:571: skia_style |= SkTypeface::kBold;
On 2016/03/22 01:53:44, msw wrote:
> This is ignored by GetMatchingDirectWriteFont now; update.

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win_un...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win_un...
ui/gfx/platform_font_win_unittest.cc:40: EXPECT_GT(derived_font.Derive(1, 0,
gfx::Font::WEIGHT_NORMAL).GetHeight(),
On 2016/03/22 01:53:44, msw wrote:
> nint: shouldn't this be bold to match the old behavior?

Nope, it should be like this because the weight on the left was normal
(Derive(1,0) means, derive me a font of size 1 larger with no styling, which the
WEIGHT_NORMAL exactly means).

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:96: SkTypeface::Style
ConvertFontStyleToSkiaTypefaceStyle(int font_style) {
On 2016/03/22 01:53:44, msw wrote:
> Remove this function if it's no longer used.

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:98: // skia_style |= (font_style & Font::BOLD) ?
SkTypeface::kBold : 0;
On 2016/03/22 01:53:44, msw wrote:
> Remove this

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:438: style & gfx::Font::ITALIC
On 2016/03/22 01:53:44, msw wrote:
> nit: if |style| only handles italic, make it a bool flag.

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.h#newcode65
ui/gfx/render_text.h:65: void SetFontWithStyle(const Font& font,
On 2016/03/22 01:53:44, msw wrote:
> nit: rename |SetFont|

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:195: // Creates an SkTypeface from a font and a
|gfx::Font::FontStyle|.
On 2016/03/22 01:53:44, msw wrote:
> nit: "font, |gfx::Font::FontStyle| and weight."

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1267: run->font_style = (style.style(ITALIC) ?
Font::ITALIC : 0);
On 2016/03/22 01:53:44, msw wrote:
> nit: convert to bool flag for italic.

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/views/controls/button/la...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/views/controls/button/la...
ui/views/controls/button/label_button.cc:48: if (font_list.Get().GetFontWeight()
!= gfx::Font::WEIGHT_BOLD) {
On 2016/03/22 01:53:44, msw wrote:
> Hmm, I wonder if this should be < (or something smarter), in case we're now
> respecting the system weight values and the user's system font is bolder, this
> would yield something with less weight... Similar below for SetFontList?

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/views/controls/tabbed_pa...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:115: kInactiveWeight))),
On 2016/03/22 01:53:44, msw wrote:
> Shouldn't this be kActiveWeight to match the old behavior?

Done.

[msw, 2016-03-22 18:24:11]

It makes for a smoother review if you address all comments and ensure changes
compile before publishing replies.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc
File ui/gfx/font_list_impl.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/font_list_impl.cc#ne...
ui/gfx/font_list_impl.cc:142: int old_style;
On 2016/03/22 14:19:51, Mikus wrote:
> On 2016/03/22 01:53:43, msw wrote:
> > I find it weird that this code ignores the description's style...
> 
> It's just like with old_style, it's ignored as well. Since the weight is
derived
> from the style, it can be safely ignored as well.

Acknowledged; the current style and weight don't matter.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font.h
File ui/gfx/platform_font.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font.h#newc...
ui/gfx/platform_font.h:51: virtual gfx::Font::FontWeight GetWeight() = 0;
On 2016/03/22 04:04:17, tapted wrote:
> You'll need a PlatformFontMac change for this too

Ping! Please address this comment.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_linux.cc
File ui/gfx/platform_font_linux.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_linux....
ui/gfx/platform_font_linux.cc:103: delegate->GetDefaultFontDescription(&family,
&size_pixels, &style,
On 2016/03/22 01:53:44, msw wrote:
> Should this also get the weight from the system UI? Or should this check if
> style specifies BOLD, and set weight from that? You probably need to update
> chrome/browser/ui/libgtk2ui/gtk2_ui.cc

Ping! Please address this comment.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win_un...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_win_un...
ui/gfx/platform_font_win_unittest.cc:40: EXPECT_GT(derived_font.Derive(1, 0,
gfx::Font::WEIGHT_NORMAL).GetHeight(),
On 2016/03/22 14:19:51, Mikus wrote:
> On 2016/03/22 01:53:44, msw wrote:
> > nint: shouldn't this be bold to match the old behavior?
> 
> Nope, it should be like this because the weight on the left was normal
> (Derive(1,0) means, derive me a font of size 1 larger with no styling, which
the
> WEIGHT_NORMAL exactly means).

Acknowledged.

https://codereview.chromium.org/1819753003/diff/20001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer.cc:27: SkFontStyle::kNormal_Weight,
Shouldn't this param be a width, not a weight?

https://codereview.chromium.org/1819753003/diff/20001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:592: 0, sized.first->second.GetFontStyle() |
style, weight);
Isn't the style of the |sized| font gfx::Font::Normal? If so, you can probably
omit "sized.first->second.GetFontStyle() | ", and just pass |style|.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font.h
File ui/gfx/font.h (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font.h#newcode35
ui/gfx/font.h:35: enum FontStyle {
nit: it'd be nice to make this an enum class too, but that can be done later.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font.h#newcode136
ui/gfx/font.h:136: GFX_EXPORT std::ostream& operator<<(std::ostream& stream,
Is this intentional/necessary?

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_list_unitte...
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:29: if (weight == gfx::Font::WEIGHT_BLACK)
Update the enum values in this file...

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:95: // Weight of the font. WEIGHT_NORMAL by default.
nit: update enum value comment

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_unittest.cc
File ui/gfx/font_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_unittest.cc...
ui/gfx/font_unittest.cc:73: Font bold(cf.Derive(0, Font::NORMAL,
Font::WEIGHT_BOLD));
Update the enum values in this file.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
File ui/gfx/platform_font_linux.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
ui/gfx/platform_font_linux.cc:45: skia::RefPtr<SkTypeface> CreateSkTypeface(int
style,
optional nit: make |style| be a bool italic flag here too?

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
ui/gfx/platform_font_linux.cc:81: Font::FontWeight weight = Font::WEIGHT_NORMAL;
Update the enum values in this file...

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
File ui/gfx/platform_font_linux.h (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
ui/gfx/platform_font_linux.h:46: gfx::Font::FontWeight weight) const override;
Update the type name in this file.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:519: const bool italic = font_info.lfItalic != 0;
optional nit: remove this, inline at uses below.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win_unittest.cc:28: Font derived_font =
platform_font->DeriveFontWithHeight(
Remove tests of (or references to) the deleted DeriveFontWithHeight.
Also, update enum values in this file (if any still remain)

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:433: 
nit: remove blank line

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:517: SetStyle(ITALIC, (font_style & gfx::Font::ITALIC) !=
0);
Make these two SetStyle calls also assign directly (for performance):
  styles_[ITALIC].SetValue((font_style & gfx::Font::ITALIC) != 0);
  styles_[UNDERLINE].SetValue((font_style & gfx::Font::UNDERLINE) != 0);

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:193: // Creates an SkTypeface from a font
|gfx::Font::FontStyle| and weight.
nit: now it takes a bool for italic, not |gfx::Font::FontStyle|.
consider: "from a font, a flag for italic, and a font weight."

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:264:
render_text->SetWeight(gfx::Font::WEIGHT_BOLD);
Update enum values in this file.

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:64: if (font_list.Get().GetFontWeight()
< gfx::Font::Weight::BOLD) {
Remove this conditional now, it should handle any weight correctly.

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:184: cached_bold_font_list_ =
font_list.DeriveWithWeight(gfx::Font::Weight::BOLD);
Use GetValueBolderThan here too.

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/style...
File ui/views/controls/styled_label_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/style...
ui/views/controls/styled_label_unittest.cc:260: style_info.weight =
gfx::Font::WEIGHT_BOLD;
Update enum values in this file.

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/tabbe...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/tabbe...
ui/views/controls/tabbed_pane/tabbed_pane.cc:114: gfx::Font::NORMAL,
nit: wrap above (git cl format?)

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

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:563: const FontKey key = {size_delta, style,
weight};
On 2016/03/22 14:19:50, Mikus wrote:
> On 2016/03/22 04:04:17, tapted wrote:
> > 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
> 
> 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

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.

https://codereview.chromium.org/1819753003/diff/20001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:582: gfx::Font::Weight::NORMAL};
running `git cl format` should update this to a more chromey style

https://codereview.chromium.org/1819753003/diff/20001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:592: 0, sized.first->second.GetFontStyle() |
style, weight);
On 2016/03/22 18:24:10, msw wrote:
> Isn't the style of the |sized| font gfx::Font::Normal? If so, you can probably
> omit "sized.first->second.GetFontStyle() | ", and just pass |style|.

This confused me initially too. There's actually no guarantee that a
default-constructed FontList creates something with a "normal" style. If someone
has configured their default GTK font to be italics, then.... I guess that's
what they want for bold fonts too.

[Mikus, 2016-03-23 17:53:22]

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font.h
File ui/gfx/platform_font.h (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font.h#newc...
ui/gfx/platform_font.h:51: virtual gfx::Font::FontWeight GetWeight() = 0;
On 2016/03/22 18:24:10, msw wrote:
> On 2016/03/22 04:04:17, tapted wrote:
> > You'll need a PlatformFontMac change for this too
> 
> Ping! Please address this comment.

Done.

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_linux.cc
File ui/gfx/platform_font_linux.cc (right):

https://codereview.chromium.org/1819753003/diff/1/ui/gfx/platform_font_linux....
ui/gfx/platform_font_linux.cc:103: delegate->GetDefaultFontDescription(&family,
&size_pixels, &style,
On 2016/03/22 18:24:10, msw wrote:
> On 2016/03/22 01:53:44, msw wrote:
> > Should this also get the weight from the system UI? Or should this check if
> > style specifies BOLD, and set weight from that? You probably need to update
> > chrome/browser/ui/libgtk2ui/gtk2_ui.cc
> 
> Ping! Please address this comment.

Done.

https://codereview.chromium.org/1819753003/diff/20001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer.cc:27: SkFontStyle::kNormal_Weight,
On 2016/03/22 18:24:10, msw wrote:
> Shouldn't this param be a width, not a weight?

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:582: gfx::Font::Weight::NORMAL};
On 2016/03/22 23:31:32, tapted wrote:
> running `git cl format` should update this to a more chromey style

This is what clang actually does

https://codereview.chromium.org/1819753003/diff/20001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:592: 0, sized.first->second.GetFontStyle() |
style, weight);
On 2016/03/22 23:31:32, tapted wrote:
> On 2016/03/22 18:24:10, msw wrote:
> > Isn't the style of the |sized| font gfx::Font::Normal? If so, you can
probably
> > omit "sized.first->second.GetFontStyle() | ", and just pass |style|.
> 
> This confused me initially too. There's actually no guarantee that a
> default-constructed FontList creates something with a "normal" style. If
someone
> has configured their default GTK font to be italics, then.... I guess that's
> what they want for bold fonts too.

Acknowledged.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font.h
File ui/gfx/font.h (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font.h#newcode35
ui/gfx/font.h:35: enum FontStyle {
On 2016/03/22 18:24:11, msw wrote:
> nit: it'd be nice to make this an enum class too, but that can be done later.

Won't do in this CL.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font.h#newcode136
ui/gfx/font.h:136: GFX_EXPORT std::ostream& operator<<(std::ostream& stream,
On 2016/03/22 18:24:10, msw wrote:
> Is this intentional/necessary?

Yes. Probably I should add an #ifndef NDEBUG around this, though. The problem is
BreakList<FontWeight> needs an ostream operator << for debugging.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_list_unitte...
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:29: if (weight == gfx::Font::WEIGHT_BLACK)
On 2016/03/22 18:24:11, msw wrote:
> Update the enum values in this file...

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:95: // Weight of the font. WEIGHT_NORMAL by default.
On 2016/03/22 18:24:11, msw wrote:
> nit: update enum value comment

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_unittest.cc
File ui/gfx/font_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/font_unittest.cc...
ui/gfx/font_unittest.cc:73: Font bold(cf.Derive(0, Font::NORMAL,
Font::WEIGHT_BOLD));
On 2016/03/22 18:24:11, msw wrote:
> Update the enum values in this file.

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
File ui/gfx/platform_font_linux.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
ui/gfx/platform_font_linux.cc:45: skia::RefPtr<SkTypeface> CreateSkTypeface(int
style,
On 2016/03/22 18:24:11, msw wrote:
> optional nit: make |style| be a bool italic flag here too?

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
ui/gfx/platform_font_linux.cc:81: Font::FontWeight weight = Font::WEIGHT_NORMAL;
On 2016/03/22 18:24:11, msw wrote:
> Update the enum values in this file...

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
File ui/gfx/platform_font_linux.h (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_li...
ui/gfx/platform_font_linux.h:46: gfx::Font::FontWeight weight) const override;
On 2016/03/22 18:24:11, msw wrote:
> Update the type name in this file.

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:519: const bool italic = font_info.lfItalic != 0;
On 2016/03/22 18:24:11, msw wrote:
> optional nit: remove this, inline at uses below.

Won't do, I also think this is less error-prone as font_info is modified below.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:433: 
On 2016/03/22 18:24:11, msw wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:517: SetStyle(ITALIC, (font_style & gfx::Font::ITALIC) !=
0);
On 2016/03/22 18:24:11, msw wrote:
> Make these two SetStyle calls also assign directly (for performance):
>   styles_[ITALIC].SetValue((font_style & gfx::Font::ITALIC) != 0);
>   styles_[UNDERLINE].SetValue((font_style & gfx::Font::UNDERLINE) != 0);

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:193: // Creates an SkTypeface from a font
|gfx::Font::FontStyle| and weight.
On 2016/03/22 18:24:11, msw wrote:
> nit: now it takes a bool for italic, not |gfx::Font::FontStyle|.
> consider: "from a font, a flag for italic, and a font weight."

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:264:
render_text->SetWeight(gfx::Font::WEIGHT_BOLD);
On 2016/03/22 18:24:11, msw wrote:
> Update enum values in this file.

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:64: if (font_list.Get().GetFontWeight()
< gfx::Font::Weight::BOLD) {
On 2016/03/22 18:24:11, msw wrote:
> Remove this conditional now, it should handle any weight correctly.

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/style...
File ui/views/controls/styled_label_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/style...
ui/views/controls/styled_label_unittest.cc:260: style_info.weight =
gfx::Font::WEIGHT_BOLD;
On 2016/03/22 18:24:11, msw wrote:
> Update enum values in this file.

Done.

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/tabbe...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://codereview.chromium.org/1819753003/diff/20001/ui/views/controls/tabbe...
ui/views/controls/tabbed_pane/tabbed_pane.cc:114: gfx::Font::NORMAL,
On 2016/03/22 18:24:11, msw wrote:
> nit: wrap above (git cl format?)

This is what git cl format suggests

[tapted, 2016-03-23 23:00:07]

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
File ui/gfx/platform_font_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
ui/gfx/platform_font_mac.mm:70: NSFontSymbolicTraits traits = [[native_font
fontDescriptor] symbolicTraits];
Without a BUG= I can't grok the wider plan for this change, but NSFont does have
other weights. You can get them with something like

NSNumber* weight =
    [[native_font fontDescriptor] fontAttibutes][NSFontWeightTrait];

The value will be a floating point number between -1.0 and 1.0.

Is there a plan to add this? The CL is already big and you'll need Mac
reviewers, so perhaps file a new bug, and reference it here with a TODO

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
ui/gfx/platform_font_mac.mm:74: font_weight_ = Font::BOLD;
Font::Weight::BOLD? - what you have shouldn't compile

[Mikus, 2016-03-24 07:31:25]

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
File ui/gfx/platform_font_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
ui/gfx/platform_font_mac.mm:70: NSFontSymbolicTraits traits = [[native_font
fontDescriptor] symbolicTraits];
On 2016/03/23 23:00:06, tapted (OOO.travel to Apr 5th) wrote:
> Without a BUG= I can't grok the wider plan for this change, but NSFont does
have
> other weights. You can get them with something like
> 
> NSNumber* weight =
>     [[native_font fontDescriptor] fontAttibutes][NSFontWeightTrait];
> 
> The value will be a floating point number between -1.0 and 1.0.
> 
> Is there a plan to add this? The CL is already big and you'll need Mac
> reviewers, so perhaps file a new bug, and reference it here with a TODO

Done.

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
ui/gfx/platform_font_mac.mm:74: font_weight_ = Font::BOLD;
On 2016/03/23 23:00:06, tapted (OOO.travel to Apr 5th) wrote:
> Font::Weight::BOLD? - what you have shouldn't compile

Done.

[Mikus, 2016-03-24 07:43:13]

On 2016/03/24 07:31:25, Mikus wrote:
>
https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
> File ui/gfx/platform_font_mac.mm (right):
> 
>
https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
> ui/gfx/platform_font_mac.mm:70: NSFontSymbolicTraits traits = [[native_font
> fontDescriptor] symbolicTraits];
> On 2016/03/23 23:00:06, tapted (OOO.travel to Apr 5th) wrote:
> > Without a BUG= I can't grok the wider plan for this change, but NSFont does
> have
> > other weights. You can get them with something like
> > 
> > NSNumber* weight =
> >     [[native_font fontDescriptor] fontAttibutes][NSFontWeightTrait];
> > 
> > The value will be a floating point number between -1.0 and 1.0.
> > 
> > Is there a plan to add this? The CL is already big and you'll need Mac
> > reviewers, so perhaps file a new bug, and reference it here with a TODO
> 
> Done.
> 
>
https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
> ui/gfx/platform_font_mac.mm:74: font_weight_ = Font::BOLD;
> On 2016/03/23 23:00:06, tapted (OOO.travel to Apr 5th) wrote:
> > Font::Weight::BOLD? - what you have shouldn't compile
> 
> Done.

https://bugs.chromium.org/p/chromium/issues/detail?id=597533

[Mikus, 2016-03-24 07:43:18]

https://bugs.chromium.org/p/chromium/issues/detail?id=597533

[msw, 2016-03-25 01:33:10]

Mostly minor comments.

https://codereview.chromium.org/1819753003/diff/140001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1819753003/diff/140001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:829: *italic_out = default_font_style_ ==
gfx::Font::ITALIC;
nit: check (default_font_style_ & gfx::Font::ITALIC != 0)

https://codereview.chromium.org/1819753003/diff/140001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1353: // TODO(davemoore): Support weights
other than bold?
It'll be much easier to address this TODO now! (in a followup).

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/font_render_par...
File ui/gfx/font_render_params.cc (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/font_render_par...
ui/gfx/font_render_params.cc:63: weight(gfx::Font::Weight::NORMAL),
q: Should this init to invalid, like style, etc?

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/linux_font_dele...
File ui/gfx/linux_font_delegate.h (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/linux_font_dele...
ui/gfx/linux_font_delegate.h:36: // Returns details about the default UI font.
|italic| should be true if the
nit: |italic_out|, but you can probably just remove this sentence.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_l...
File ui/gfx/platform_font_linux_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux_unittest.cc:112: // Drop the old default font and
check that new settings are loaded.
nit: please also test italic in this same case.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_m...
File ui/gfx/platform_font_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_m...
ui/gfx/platform_font_mac.mm:26: gfx::Font::Weight font_weight) {
nit: no need for 'gfx::'

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_m...
ui/gfx/platform_font_mac.mm:78: font_weight_ = Font::Weight::NORMAL;
nit: no need for this else statement with the init above.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/render_text_mac.mm
File ui/gfx/render_text_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:71: // TODO(mboc): This accepts style, not italic.
You'll need to fix this before landing.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:218: bool italic = false;
nit: inline the flag check into init (and move this down), or the function call:
bool italic = (traits & kCTFontItalicTrait); OR:
renderer->SetFont(run.font, (traits & kCTFontItalicTrait), font_weight);

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:336: // TODO(mboc): Apply font weights below.
You'll need to fix this before landing.

https://codereview.chromium.org/1819753003/diff/140001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/textfield.h:200: // Set various text weights over
the entire text or a logical character range.
These functions are arguably unnecessary; they're only used in views_examples.
It's plausible that some textfield control (omnibox?) may want to set the font
weight, but it's tough to defend adding functionality with no imminent need. I'm
not really opposed to adding these, but you'll need to add unit tests.

[msw, 2016-03-25 01:41:31]

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
File ui/gfx/platform_font_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
ui/gfx/platform_font_mac.mm:70: NSFontSymbolicTraits traits = [[native_font
fontDescriptor] symbolicTraits];
On 2016/03/24 07:31:25, Mikus wrote:
> On 2016/03/23 23:00:06, tapted (OOO.travel to Apr 5th) wrote:
> > Without a BUG= I can't grok the wider plan for this change, but NSFont does
> have
> > other weights. You can get them with something like
> > 
> > NSNumber* weight =
> >     [[native_font fontDescriptor] fontAttibutes][NSFontWeightTrait];
> > 
> > The value will be a floating point number between -1.0 and 1.0.
> > 
> > Is there a plan to add this? The CL is already big and you'll need Mac
> > reviewers, so perhaps file a new bug, and reference it here with a TODO
> 
> Done.

Please cite the bug you filed in the BUG= line of the CL description.
I'm not entirely sure, but I think Trent was actually asking for a broader bug
describing the need for this change (even if it's just *needed* downstream for
now), a description of its benefits/effects (like enabling Chrome to respect
users' system font weights; neat!), and bigger TODOs (like *actually* respecting
users' system font weights). Either way, please file a bug like that (or broaden
the Mac one), and list it in BUG=; thanks!

[Mikus, 2016-03-25 11:43:12]

Description was changed from

==========
Allow various font weights in gfx.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow various font weights in gfx.

BUG=597533
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Mikus, 2016-03-25 11:49:01]

https://codereview.chromium.org/1819753003/diff/140001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1819753003/diff/140001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:829: *italic_out = default_font_style_ ==
gfx::Font::ITALIC;
On 2016/03/25 01:33:09, msw wrote:
> nit: check (default_font_style_ & gfx::Font::ITALIC != 0)

Done.

https://codereview.chromium.org/1819753003/diff/140001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1353: // TODO(davemoore): Support weights
other than bold?
On 2016/03/25 01:33:09, msw wrote:
> It'll be much easier to address this TODO now! (in a followup).

Acknowledged. Todo changed.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/font_render_par...
File ui/gfx/font_render_params.cc (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/font_render_par...
ui/gfx/font_render_params.cc:63: weight(gfx::Font::Weight::NORMAL),
On 2016/03/25 01:33:09, msw wrote:
> q: Should this init to invalid, like style, etc?

Done. And yes, it should.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/linux_font_dele...
File ui/gfx/linux_font_delegate.h (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/linux_font_dele...
ui/gfx/linux_font_delegate.h:36: // Returns details about the default UI font.
|italic| should be true if the
On 2016/03/25 01:33:10, msw wrote:
> nit: |italic_out|, but you can probably just remove this sentence.

Done.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_l...
File ui/gfx/platform_font_linux_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux_unittest.cc:112: // Drop the old default font and
check that new settings are loaded.
On 2016/03/25 01:33:10, msw wrote:
> nit: please also test italic in this same case.

Done.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_m...
File ui/gfx/platform_font_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_m...
ui/gfx/platform_font_mac.mm:26: gfx::Font::Weight font_weight) {
On 2016/03/25 01:33:10, msw wrote:
> nit: no need for 'gfx::'

Done.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/platform_font_m...
ui/gfx/platform_font_mac.mm:78: font_weight_ = Font::Weight::NORMAL;
On 2016/03/25 01:33:10, msw wrote:
> nit: no need for this else statement with the init above.

Done.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/render_text_mac.mm
File ui/gfx/render_text_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:71: // TODO(mboc): This accepts style, not italic.
On 2016/03/25 01:33:10, msw wrote:
> You'll need to fix this before landing.

Done.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:218: bool italic = false;
On 2016/03/25 01:33:10, msw wrote:
> nit: inline the flag check into init (and move this down), or the function
call:
> bool italic = (traits & kCTFontItalicTrait); OR:
> renderer->SetFont(run.font, (traits & kCTFontItalicTrait), font_weight);

Done.

https://codereview.chromium.org/1819753003/diff/140001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:336: // TODO(mboc): Apply font weights below.
On 2016/03/25 01:33:10, msw wrote:
> You'll need to fix this before landing.

Done for Bold, the rest will be done in a followup.

https://codereview.chromium.org/1819753003/diff/140001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/1819753003/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/textfield.h:200: // Set various text weights over
the entire text or a logical character range.
On 2016/03/25 01:33:10, msw wrote:
> These functions are arguably unnecessary; they're only used in views_examples.
> It's plausible that some textfield control (omnibox?) may want to set the font
> weight, but it's tough to defend adding functionality with no imminent need.
I'm
> not really opposed to adding these, but you'll need to add unit tests.

Done - removed.

[Mikus, 2016-03-25 11:51:26]

On 2016/03/25 01:41:31, msw wrote:
>
https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
> File ui/gfx/platform_font_mac.mm (right):
> 
>
https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
> ui/gfx/platform_font_mac.mm:70: NSFontSymbolicTraits traits = [[native_font
> fontDescriptor] symbolicTraits];
> On 2016/03/24 07:31:25, Mikus wrote:
> > On 2016/03/23 23:00:06, tapted (OOO.travel to Apr 5th) wrote:
> > > Without a BUG= I can't grok the wider plan for this change, but NSFont
does
> > have
> > > other weights. You can get them with something like
> > > 
> > > NSNumber* weight =
> > >     [[native_font fontDescriptor] fontAttibutes][NSFontWeightTrait];
> > > 
> > > The value will be a floating point number between -1.0 and 1.0.
> > > 
> > > Is there a plan to add this? The CL is already big and you'll need Mac
> > > reviewers, so perhaps file a new bug, and reference it here with a TODO
> > 
> > Done.
> 
> Please cite the bug you filed in the BUG= line of the CL description.
> I'm not entirely sure, but I think Trent was actually asking for a broader bug
> describing the need for this change (even if it's just *needed* downstream for
> now), a description of its benefits/effects (like enabling Chrome to respect
> users' system font weights; neat!), and bigger TODOs (like *actually*
respecting
> users' system font weights). Either way, please file a bug like that (or
broaden
> the Mac one), and list it in BUG=; thanks!

BUG= desc added.

[msw, 2016-03-25 22:02:12]

msw@chromium.org changed reviewers:
+ pkasting@chromium.org

[msw, 2016-03-25 22:02:14]

Just one remaining comment before I'd approve this. Trent, Scott, and +Peter
should still review (+any other required OWNERS). Please write a short CL
description; a few sentences or short paragraphs on the how/why of this change.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:81: // TODO(mboc): Get the
bold font.
You'll need to fix this before landing, afaict.

[Peter Kasting, 2016-03-26 00:56:52]

Only reviewed c/b/ui.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1081: .GetNativeFont();
Nit: Maybe:

namespace {

NSFont* GetFontForSizeAndStyle(ui::ResourceBundle::FontStyle size, int style) {
  return ui::ResourceBundle::GetSharedInstance().GetFontList(size)
      .Derive(1, style, gfx::Font::Weight::NORMAL)
      .GetPrimaryFont()
      .GetNativeFont();
}

}  // namespace

...

// static
NSFont* OmniboxViewMac::GetFieldFont(int style) {
  return GetFontForSizeAndStyle(ui::ResourceBundle::BaseFont, style);
}

...

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1355: query.weight =
gfx::Font::Weight::BOLD;
I think you can just do:

  query.weight =
      static_cast<gfx::Font::Weight>(pango_font_description_get_weight(desc));

...as Pango weights are 100-900 just like your enum values.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:818:
destination->ApplyWeight(gfx::Font::Weight::BOLD, range);
This looks wrong.  Should this be something like:

  destination->ApplyWeight(
      is_bold ? gfx::Font::Weight::BOLD : gfx::Font::Weight::NORMAL, range);

Or, if we don't need to toggle some regions explicitly to normal, maybe:

  if (is_bold)
    destination->ApplyWeight(gfx::Font::Weight::BOLD, range);

https://codereview.chromium.org/1819753003/diff/180001/ui/gfx/font.h
File ui/gfx/font.h (right):

https://codereview.chromium.org/1819753003/diff/180001/ui/gfx/font.h#newcode35
ui/gfx/font.h:35: enum FontStyle {
Didn't read previous conversations, so maybe you discussed this; should this
become "enum class Style" for parallel structure?  That would make some callers
read more clearly, since instead of:

gfx::Font::NORMAL, gfx::Font::Weight::BOLD

...as a parameter list, you get:

gfx::Font::Style::NORMAL, gfx::Font::Weight::BOLD

It may be that this would touch enough places that you'd want to do it
separately, but I raise it now in case you're already touching enough of those
places anyway to make this make sense.

[tapted, 2016-03-27 23:10:02]

I've only looked closely at resource_bundle and mac stuff. c/b/ui/cocoa needs a
bit more to avoid regressions

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
File ui/gfx/platform_font_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/60001/ui/gfx/platform_font_ma...
ui/gfx/platform_font_mac.mm:70: NSFontSymbolicTraits traits = [[native_font
fontDescriptor] symbolicTraits];
On 2016/03/25 01:41:31, msw wrote:
> On 2016/03/24 07:31:25, Mikus wrote:
> > On 2016/03/23 23:00:06, tapted (OOO.travel to Apr 5th) wrote:
> > > Without a BUG= I can't grok the wider plan for this change, but NSFont
does
> > have
> > > other weights. You can get them with something like
> > > 
> > > NSNumber* weight =
> > >     [[native_font fontDescriptor] fontAttibutes][NSFontWeightTrait];
> > > 
> > > The value will be a floating point number between -1.0 and 1.0.
> > > 
> > > Is there a plan to add this? The CL is already big and you'll need Mac
> > > reviewers, so perhaps file a new bug, and reference it here with a TODO
> > 
> > Done.
> 
> Please cite the bug you filed in the BUG= line of the CL description.
> I'm not entirely sure, but I think Trent was actually asking for a broader bug
> describing the need for this change (even if it's just *needed* downstream for
> now), a description of its benefits/effects (like enabling Chrome to respect
> users' system font weights; neat!), and bigger TODOs (like *actually*
respecting
> users' system font weights). Either way, please file a bug like that (or
broaden
> the Mac one), and list it in BUG=; thanks!

The bug still doesn't describe the _need_ for the change. You've set me as the
owner, and you're leaving the mac implementation incomplete. Describing the need
for the change allows us to determine what priority to attach to the remaining
parts, and find appropriate owners.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:81: // TODO(mboc): Get the
bold font.
On 2016/03/25 22:02:14, msw wrote:
> You'll need to fix this before landing, afaict.

Yes - please do this. See my suggestion below.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1075: NSFont*
OmniboxViewMac::GetFieldFont(int style) {
Callers only ever pass in BOLD or NORMAL to this method. And callers of
Large/Small appear to always pass Normal.

Rather than mechanically updating the `int style` to a `gfx::Font::Weight` I
think it would be nicer to have methods:

OmniboxViewMac::GetNormalFieldFont();
OmniboxViewMac::GetBoldFieldFont();
OmniboxViewMac::GetLargeFont();
OmniboxViewMac::GetSmallFont();


Then since it's changing anyway (and it's silly to take a base font and "just
add one to it"). Instead of GetFontList(LargeFont).Derive(1), it should be

const int kOmniboxLargeFontSizeDelta = 9;
return ui::ResourceBundle::GetSharedInstance()
    .GetFontWithDelta(kOmniboxLargeFontSizeDelta).GetNativeFont();


(`9` is `ResourceBundle::kLargeFontDelta + 1` but there's no benefit in coupling
them together since "Large" doesn't have any semantic meaning of its own)

Do the same for Small/Base.

[Mikus, 2016-03-29 10:55:38]

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:81: // TODO(mboc): Get the
bold font.
On 2016/03/27 23:10:02, tapted (travelling to Apr 5th) wrote:
> On 2016/03/25 22:02:14, msw wrote:
> > You'll need to fix this before landing, afaict.
> 
> Yes - please do this. See my suggestion below.

Done.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1075: NSFont*
OmniboxViewMac::GetFieldFont(int style) {
On 2016/03/27 23:10:02, tapted (travelling to Apr 5th) wrote:
> Callers only ever pass in BOLD or NORMAL to this method. And callers of
> Large/Small appear to always pass Normal.
> 
> Rather than mechanically updating the `int style` to a `gfx::Font::Weight` I
> think it would be nicer to have methods:
> 
> OmniboxViewMac::GetNormalFieldFont();
> OmniboxViewMac::GetBoldFieldFont();
> OmniboxViewMac::GetLargeFont();
> OmniboxViewMac::GetSmallFont();
> 
> 
> Then since it's changing anyway (and it's silly to take a base font and "just
> add one to it"). Instead of GetFontList(LargeFont).Derive(1), it should be
> 
> const int kOmniboxLargeFontSizeDelta = 9;
> return ui::ResourceBundle::GetSharedInstance()
>     .GetFontWithDelta(kOmniboxLargeFontSizeDelta).GetNativeFont();
> 
> 
> (`9` is `ResourceBundle::kLargeFontDelta + 1` but there's no benefit in
coupling
> them together since "Large" doesn't have any semantic meaning of its own)
> 
> Do the same for Small/Base.

Done.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1081: .GetNativeFont();
On 2016/03/26 00:56:51, Peter Kasting wrote:
> Nit: Maybe:
> 
> namespace {
> 
> NSFont* GetFontForSizeAndStyle(ui::ResourceBundle::FontStyle size, int style)
{
>   return ui::ResourceBundle::GetSharedInstance().GetFontList(size)
>       .Derive(1, style, gfx::Font::Weight::NORMAL)
>       .GetPrimaryFont()
>       .GetNativeFont();
> }
> 
> }  // namespace
> 
> ...
> 
> // static
> NSFont* OmniboxViewMac::GetFieldFont(int style) {
>   return GetFontForSizeAndStyle(ui::ResourceBundle::BaseFont, style);
> }
> 
> ...

Not really needed anymore.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1355: query.weight =
gfx::Font::Weight::BOLD;
On 2016/03/26 00:56:51, Peter Kasting wrote:
> I think you can just do:
> 
>   query.weight =
>       static_cast<gfx::Font::Weight>(pango_font_description_get_weight(desc));
> 
> ...as Pango weights are 100-900 just like your enum values.

Done.

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/1819753003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:818:
destination->ApplyWeight(gfx::Font::Weight::BOLD, range);
On 2016/03/26 00:56:51, Peter Kasting wrote:
> This looks wrong.  Should this be something like:
> 
>   destination->ApplyWeight(
>       is_bold ? gfx::Font::Weight::BOLD : gfx::Font::Weight::NORMAL, range);
> 
> Or, if we don't need to toggle some regions explicitly to normal, maybe:
> 
>   if (is_bold)
>     destination->ApplyWeight(gfx::Font::Weight::BOLD, range);

Done.

https://codereview.chromium.org/1819753003/diff/180001/ui/gfx/font.h
File ui/gfx/font.h (right):

https://codereview.chromium.org/1819753003/diff/180001/ui/gfx/font.h#newcode35
ui/gfx/font.h:35: enum FontStyle {
On 2016/03/26 00:56:51, Peter Kasting wrote:
> Didn't read previous conversations, so maybe you discussed this; should this
> become "enum class Style" for parallel structure?  That would make some
callers
> read more clearly, since instead of:
> 
> gfx::Font::NORMAL, gfx::Font::Weight::BOLD
> 
> ...as a parameter list, you get:
> 
> gfx::Font::Style::NORMAL, gfx::Font::Weight::BOLD
> 
> It may be that this would touch enough places that you'd want to do it
> separately, but I raise it now in case you're already touching enough of those
> places anyway to make this make sense.

Yes, this was raised previously, we've decided to make this an enum class in a
follow-up patchset.

[Mikus, 2016-03-29 10:58:56]

> The bug still doesn't describe the _need_ for the change. You've set me as the
> owner, and you're leaving the mac implementation incomplete. Describing the
need
> for the change allows us to determine what priority to attach to the remaining
> parts, and find appropriate owners.
> 

Actually there's a single motivation for this:

These changes make Chromium's gfx::Font more closely match native font APIs &
capabilities.

Wrote just that in the bug.

[msw, 2016-03-29 19:18:14]

lgtm with nits; please include the motivation sentence in the CL description.

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1077: const int
kOmniboxNormalFontSizeDelta = 1;
nit: these probably belong in a file-local anon namespace.

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1105: const int
kOmniboxSmallFontSizeDelta = 0;
ha! the old code was getting the small font (-1 delta) and then deriving one
size larger from that(+1 delta), so this should indeed be zero, but I wonder if
we should just avoid the deriving altogether now, and just return
rb.GetFont(gfx::Font::NORMAL), but maybe Trent likes the consistency of
kOmnibox*FontSizeDelta constants, etc.

[tapted, 2016-03-29 20:13:46]

mac stuff lgtm with msw's nit and the comments below (resource_bundle also lg,
but I'm not an owner there)

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1076: // This value should
be kept in sync with InstantPage::InitializeFonts.
InstantPage::InitializeFonts doesn't seem to exist any more, so when you move
the constants, these comments can be deleted.

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1105: const int
kOmniboxSmallFontSizeDelta = 0;
On 2016/03/29 19:18:14, msw wrote:
> ha! the old code was getting the small font (-1 delta) and then deriving one
> size larger from that(+1 delta), so this should indeed be zero, but I wonder
if
> we should just avoid the deriving altogether now, and just return
> rb.GetFont(gfx::Font::NORMAL), but maybe Trent likes the consistency of
> kOmnibox*FontSizeDelta constants, etc.

Yeah although it's common for code to use rb.GetFont(ResourceBundle::BaseFont),
I have a preference for the separate constants.
ResourceBundle::BaseFont/SmallFont, etc. aren't really any better than magic
numbers - they don't give any semantic meaning and it encourages this tendency
to take something "close" to what you actually need and derive something new,
which is much more expensive than relying on the cache in ResourceBundle.

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right):

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:118:
TEST_F(OmniboxViewMacTest, GetNormalFieldFont) {
Thanks for updating these! But I think we can just put them all in the same test
case and call it something like `GetFonts` -- slightly less work for the
trybots, and no real downside

[Mikus, 2016-03-30 13:43:24]

Description was changed from

==========
Allow various font weights in gfx.

BUG=597533
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow various font weights in gfx. These changes make Chromium's gfx::Font more
closely match native font APIs &
capabilities.

BUG=597533
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Mikus, 2016-03-30 13:58:24]

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1076: // This value should
be kept in sync with InstantPage::InitializeFonts.
On 2016/03/29 20:13:46, tapted (travelling to Apr 5th) wrote:
> InstantPage::InitializeFonts doesn't seem to exist any more, so when you move
> the constants, these comments can be deleted.

Done.

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1077: const int
kOmniboxNormalFontSizeDelta = 1;
On 2016/03/29 19:18:14, msw wrote:
> nit: these probably belong in a file-local anon namespace.

Done.

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:1105: const int
kOmniboxSmallFontSizeDelta = 0;
On 2016/03/29 20:13:46, tapted (travelling to Apr 5th) wrote:
> On 2016/03/29 19:18:14, msw wrote:
> > ha! the old code was getting the small font (-1 delta) and then deriving one
> > size larger from that(+1 delta), so this should indeed be zero, but I wonder
> if
> > we should just avoid the deriving altogether now, and just return
> > rb.GetFont(gfx::Font::NORMAL), but maybe Trent likes the consistency of
> > kOmnibox*FontSizeDelta constants, etc.
> 
> Yeah although it's common for code to use
rb.GetFont(ResourceBundle::BaseFont),
> I have a preference for the separate constants.
> ResourceBundle::BaseFont/SmallFont, etc. aren't really any better than magic
> numbers - they don't give any semantic meaning and it encourages this tendency
> to take something "close" to what you actually need and derive something new,
> which is much more expensive than relying on the cache in ResourceBundle.

Acknowledged.

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right):

https://codereview.chromium.org/1819753003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:118:
TEST_F(OmniboxViewMacTest, GetNormalFieldFont) {
On 2016/03/29 20:13:46, tapted (travelling to Apr 5th) wrote:
> Thanks for updating these! But I think we can just put them all in the same
test
> case and call it something like `GetFonts` -- slightly less work for the
> trybots, and no real downside

Done.

[Mikus, 2016-03-30 15:57:27]

Please review:

danakj:
  cc\layers\heads_up_display_layer.cc

asvitkine:
  content\browser\renderer_host\sandbox_ipc_linux.cc

sky:
  chrome\renderer\pepper\pepper_flash_renderer_host.cc
  components\favicon\core\fallback_icon_service.cc
  third_party\WebKit\Source\platform\fonts\skia\FontCacheSkia.cpp
  ui\app_list\views\search_result_view.cc
  ui\base\resource\resource_bundle.cc
  ui\base\resource\resource_bundle.h
  ui\views\controls\button\label_button.cc
  ui\views\controls\styled_label.cc
  ui\views\controls\styled_label.h
  ui\views\controls\styled_label_unittest.cc
  ui\views\controls\tabbed_pane\tabbed_pane.cc
  ui\views\examples\multiline_example.cc
  ui\views\examples\text_example.cc
  ui\views\examples\textfield_example.cc

[Mikus, 2016-03-30 15:58:21]

mboc@opera.com changed reviewers:
+ asvitkine@google.com, danakj@chromium.org

[sky, 2016-03-31 17:04:08]

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
ui/base/resource/resource_bundle.cc:175: if (size_delta != rhs.size_delta)
use std::tie

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
ui/base/resource/resource_bundle.h:256: gfx::Font::Weight weight =
gfx::Font::Weight::NORMAL);
The style guide says not to use defaults like this. You can either update all
call sites, or add a variant: GetFontWithDeltaAndWeight.

[Peter Kasting, 2016-03-31 21:04:49]

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
ui/base/resource/resource_bundle.h:256: gfx::Font::Weight weight =
gfx::Font::Weight::NORMAL);
On 2016/03/31 17:04:08, sky wrote:
> The style guide says not to use defaults like this. You can either update all
> call sites, or add a variant: GetFontWithDeltaAndWeight.

No longer true (style guide was recently updated).  See
http://google.github.io/styleguide/cppguide.html#Default_Arguments for the
current ruling.

I think defaults are likely OK here, but check the guidelines to be sure.

[sky, 2016-04-01 02:01:00]

My mistake.

On Thu, Mar 31, 2016 at 2:04 PM,  <pkasting@chromium.org> wrote:
>
>
https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
> File ui/base/resource/resource_bundle.h (right):
>
>
https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
> ui/base/resource/resource_bundle.h:256: gfx::Font::Weight weight =
> gfx::Font::Weight::NORMAL);
> On 2016/03/31 17:04:08, sky wrote:
>> The style guide says not to use defaults like this. You can either
> update all
>> call sites, or add a variant: GetFontWithDeltaAndWeight.
>
> No longer true (style guide was recently updated). See
> http://google.github.io/styleguide/cppguide.html#Default_Arguments for
> the current ruling.
>
> I think defaults are likely OK here, but check the guidelines to be
> sure.
>
> https://codereview.chromium.org/1819753003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mikus, 2016-04-04 12:16:25]

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
ui/base/resource/resource_bundle.cc:175: if (size_delta != rhs.size_delta)
On 2016/03/31 17:04:08, sky wrote:
> use std::tie

Done.

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/1819753003/diff/240001/ui/base/resource/resou...
ui/base/resource/resource_bundle.h:256: gfx::Font::Weight weight =
gfx::Font::Weight::NORMAL);
On 2016/03/31 21:04:49, Peter Kasting wrote:
> On 2016/03/31 17:04:08, sky wrote:
> > The style guide says not to use defaults like this. You can either update
all
> > call sites, or add a variant: GetFontWithDeltaAndWeight.
> 
> No longer true (style guide was recently updated).  See
> http://google.github.io/styleguide/cppguide.html#Default_Arguments for the
> current ruling.
> 
> I think defaults are likely OK here, but check the guidelines to be sure.

Acknowledged.

[sky, 2016-04-04 15:40:42]

LGTM

[Alexei Svitkine (slow), 2016-04-04 16:34:48]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-04-04 16:34:51]

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/font_list_impl.cc
File ui/gfx/font_list_impl.cc (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:22: gfx::Font::Weight weight) {
Nit: Move the start of "namespace gfx" above the anon namespace and remove the
"gfx::" prefixes in this function.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:53: default:
If the switch covers all the entries in the enum, remove the default: case so
that the compiler will warn if a new entry is added and is not handled.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_i...
File ui/gfx/platform_font_ios.h (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.h:40: gfx::Font::Weight weight);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
File ui/gfx/platform_font_linux.h (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.h:48: gfx::Font::Weight GetWeight() const override;
Remove gfx:: since this in gfx:: namespace. Please fix throughout.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.h:65: gfx::Font::Weight weight,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.h:75: gfx::Font::Weight weight,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
File ui/gfx/platform_font_linux_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux_unittest.cc:37: }
Add an empty line below.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_m...
File ui/gfx/platform_font_mac.h (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_m...
ui/gfx/platform_font_mac.h:28: gfx::Font::Weight GetWeight() const override;
No need for gfx:: prefix since this is in gfx:: namespace.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_w...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.cc:73: return static_cast<gfx::Font::Weight>(weight);
This requires that Windows font weights match the enum. Can you document this
requirement somewhere - e.g. where you declare the enum? Does Windows have a
corresponding enum? If so, maybe add static_asserts in the code that validate
that their values match.

[Mikus, 2016-04-05 16:18:34]

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/font_list_impl.cc
File ui/gfx/font_list_impl.cc (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:22: gfx::Font::Weight weight) {
On 2016/04/04 16:34:50, Alexei Svitkine wrote:
> Nit: Move the start of "namespace gfx" above the anon namespace and remove the
> "gfx::" prefixes in this function.

Done.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:53: default:
On 2016/04/04 16:34:50, Alexei Svitkine wrote:
> If the switch covers all the entries in the enum, remove the default: case so
> that the compiler will warn if a new entry is added and is not handled.

Done.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_i...
File ui/gfx/platform_font_ios.h (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.h:40: gfx::Font::Weight weight);
On 2016/04/04 16:34:51, Alexei Svitkine wrote:
> Remove gfx::

Done.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
File ui/gfx/platform_font_linux.h (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.h:48: gfx::Font::Weight GetWeight() const override;
On 2016/04/04 16:34:51, Alexei Svitkine wrote:
> Remove gfx:: since this in gfx:: namespace. Please fix throughout.

Done.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.h:65: gfx::Font::Weight weight,
On 2016/04/04 16:34:51, Alexei Svitkine wrote:
> Remove gfx::

Done.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.h:75: gfx::Font::Weight weight,
On 2016/04/04 16:34:51, Alexei Svitkine wrote:
> Remove gfx::

Done.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
File ui/gfx/platform_font_linux_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux_unittest.cc:37: }
On 2016/04/04 16:34:51, Alexei Svitkine wrote:
> Add an empty line below.

Done.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_m...
File ui/gfx/platform_font_mac.h (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_m...
ui/gfx/platform_font_mac.h:28: gfx::Font::Weight GetWeight() const override;
On 2016/04/04 16:34:51, Alexei Svitkine wrote:
> No need for gfx:: prefix since this is in gfx:: namespace.

Done.

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_w...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/1819753003/diff/260001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.cc:73: return static_cast<gfx::Font::Weight>(weight);
On 2016/04/04 16:34:51, Alexei Svitkine wrote:
> This requires that Windows font weights match the enum. Can you document this
> requirement somewhere - e.g. where you declare the enum? Does Windows have a
> corresponding enum? If so, maybe add static_asserts in the code that validate
> that their values match.

Done.

[Alexei Svitkine (slow), 2016-04-05 16:38:54]

lgtm % removing the gfx:: prefix from a million places inside gfx code that I
point out below

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font.cc
File ui/gfx/font.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font.cc#newcode80
ui/gfx/font.cc:80: gfx::Font::Weight Font::GetWeight() const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font.cc#newcode95
ui/gfx/font.cc:95: std::ostream& operator<<(std::ostream& stream, const
gfx::Font::Weight weight) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font.h
File ui/gfx/font.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font.h#newcode82
ui/gfx/font.h:82: Font Derive(int size_delta, int style, gfx::Font::Weight
weight) const;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font.h#newcode92
ui/gfx/font.h:92: gfx::Font::Weight GetWeight() const;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font.h#newcode140
ui/gfx/font.h:140: const gfx::Font::Weight weight);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.cc#ne...
ui/gfx/font_list.cc:65: *style_out = gfx::Font::NORMAL;
Remove gfx:: here and on the lines below.

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.cc#ne...
ui/gfx/font_list.cc:133: gfx::Font::Weight weight) const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.cc#ne...
ui/gfx/font_list.cc:145: FontList FontList::DeriveWithWeight(gfx::Font::Weight
weight) const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.cc#ne...
ui/gfx/font_list.cc:194: gfx::Font::Weight FontList::GetFontWeight() const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.h
File ui/gfx/font_list.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.h#new...
ui/gfx/font_list.h:47: gfx::Font::Weight* weight_out);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.h#new...
ui/gfx/font_list.h:95: gfx::Font::Weight weight) const;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.h#new...
ui/gfx/font_list.h:108: FontList DeriveWithWeight(gfx::Font::Weight weight)
const;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list.h#new...
ui/gfx/font_list.h:159: gfx::Font::Weight GetFontWeight() const;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl.cc
File ui/gfx/font_list_impl.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:71: font_weight_(gfx::Font::Weight::INVALID) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:81: gfx::Font::Weight font_weight)
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:99: font_weight_(gfx::Font::Weight::INVALID) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:123: gfx::Font::Weight weight) const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:136: gfx::Font::Weight old_weight;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:177: gfx::Font::Weight FontListImpl::GetFontWeight()
const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.cc:178: if (font_weight_ == gfx::Font::Weight::INVALID)
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl.h
File ui/gfx/font_list_impl.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.h:38: gfx::Font::Weight font_weight);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.h:53: gfx::Font::Weight weight) const;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.h:79: gfx::Font::Weight GetFontWeight() const;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_impl....
ui/gfx/font_list_impl.h:121: mutable gfx::Font::Weight font_weight_;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_unitt...
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_unitt...
ui/gfx/font_list_unittest.cc:29: if (weight == gfx::Font::Weight::BLACK)
Remove gfx:: by moving the "namespace gfx" above the anon namespace.

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_list_unitt...
ui/gfx/font_list_unittest.cc:58: gfx::Font::Weight weight =
gfx::Font::Weight::NORMAL;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_render_par...
File ui/gfx/font_render_params.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_render_par...
ui/gfx/font_render_params.cc:63: weight(gfx::Font::Weight::INVALID),
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_render_par...
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/font_render_par...
ui/gfx/font_render_params.h:96: gfx::Font::Weight weight;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font.h
File ui/gfx/platform_font.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font.h...
ui/gfx/platform_font.h:41: gfx::Font::Weight weight) const = 0;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font.h...
ui/gfx/platform_font.h:51: virtual gfx::Font::Weight GetWeight() const = 0;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
File ui/gfx/platform_font_ios.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.h:23: gfx::Font::Weight weight) const override;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.h:25: gfx::Font::Weight GetWeight() const override;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.h:47: gfx::Font::Weight weight);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.h:55: gfx::Font::Weight weight_;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
File ui/gfx/platform_font_ios.mm (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.mm:25: weight_ = gfx::Font::Weight::NORMAL;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.mm:34: gfx::Font::NORMAL, gfx::Font::Weight::NORMAL);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.mm:39: gfx::Font::Weight::NORMAL);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.mm:47: gfx::Font::Weight weight) const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.mm:72: gfx::Font::Weight PlatformFontIOS::GetWeight()
const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.mm:105: gfx::Font::Weight weight) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_i...
ui/gfx/platform_font_ios.mm:112: gfx::Font::Weight weight) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_l...
File ui/gfx/platform_font_linux.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.cc:149: gfx::Font::Weight weight) const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.cc:153: const int current_style = italic_ ?
gfx::Font::ITALIC : gfx::Font::NORMAL;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.cc:176: gfx::Font::Weight
PlatformFontLinux::GetWeight() const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.cc:236: gfx::Font::Weight weight,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux.cc:248: gfx::Font::Weight weight,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_l...
File ui/gfx/platform_font_linux_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_l...
ui/gfx/platform_font_linux_unittest.cc:56: gfx::Font::Weight weight_;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.cc:72: gfx::Font::Weight ToGfxFontWeight(int weight) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.cc:317: gfx::Font::Weight weight) const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.cc:333: gfx::Font::Weight PlatformFontWin::GetWeight()
const {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.cc:611: gfx::Font::Weight weight,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
File ui/gfx/platform_font_win.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:56: gfx::Font::Weight weight) const override;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:58: gfx::Font::Weight GetWeight() const override;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:106: gfx::Font::Weight weight,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:115: gfx::Font::Weight weight() const { return
weight_; }
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:142: const gfx::Font::Weight weight_;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:253: gfx::Font::Weight weight) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:261: paint_.setFakeBoldText(weight >=
gfx::Font::Weight::SEMIBOLD &&
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:385: const BreakList<gfx::Font::Weight>& weights,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:428: skia::RefPtr<SkTypeface> CreateSkiaTypeface(const
gfx::Font& font,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:430: gfx::Font::Weight weight) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:781: void RenderText::SetWeight(gfx::Font::Weight weight)
{
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:788: void RenderText::ApplyWeight(gfx::Font::Weight
weight, const Range& range) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:1026: weights_(gfx::Font::Weight::NORMAL),
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:65: void SetFont(const Font& font, bool italic,
gfx::Font::Weight weight);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:126: const BreakList<gfx::Font::Weight>& weights,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:134: gfx::Font::Weight weight() const { return
weight_->second; }
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:145: BreakList<gfx::Font::Weight> weights_;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:150: BreakList<gfx::Font::Weight>::const_iterator weight_;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:194: skia::RefPtr<SkTypeface> CreateSkiaTypeface(const
gfx::Font& font,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:196: gfx::Font::Weight weight);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:371: void SetWeight(gfx::Font::Weight weight);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:372: void ApplyWeight(gfx::Font::Weight weight, const
Range& range);
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:497: const BreakList<gfx::Font::Weight>& weights() const {
return weights_; }
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:753: BreakList<gfx::Font::Weight> weights_;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_har...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_har...
ui/gfx/render_text_harfbuzz.cc:580: weight(gfx::Font::Weight::NORMAL),
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_har...
ui/gfx/render_text_harfbuzz.cc:1438: query.style = run->italic ?
gfx::Font::ITALIC : 0;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_har...
File ui/gfx/render_text_harfbuzz.h (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_har...
ui/gfx/render_text_harfbuzz.h:82: gfx::Font::Weight weight;
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_mac.mm
File ui/gfx/render_text_mac.mm (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:68: skia::RefPtr<SkTypeface> CreateSkiaTypeface(const
gfx::Font& font,
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:70: gfx::Font::Weight weight) {
Remove gfx::

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:264:
render_text->SetWeight(gfx::Font::Weight::BOLD);
Remove gfx::

[Evan Stade, 2016-05-19 21:50:55]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2016-05-19 21:50:57]

what happened with this CL?

[danakj, 2016-05-19 21:54:59]

Oh, cc LGTM. 1 question

https://codereview.chromium.org/1819753003/diff/280001/cc/layers/heads_up_dis...
File cc/layers/heads_up_display_layer.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer.cc:26: SkFontStyle::kUpright_Slant);
why the upright?

[Mikus, 2016-05-20 07:11:33]

https://codereview.chromium.org/1819753003/diff/280001/cc/layers/heads_up_dis...
File cc/layers/heads_up_display_layer.cc (right):

https://codereview.chromium.org/1819753003/diff/280001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer.cc:26: SkFontStyle::kUpright_Slant);
On 2016/05/19 21:54:59, danakj wrote:
> why the upright?

This code has been changed on rebase. Now there are no changes to this file.

[Evan Stade, 2016-05-27 18:00:15]

On 2016/05/20 07:11:33, Mikus wrote:
>
https://codereview.chromium.org/1819753003/diff/280001/cc/layers/heads_up_dis...
> File cc/layers/heads_up_display_layer.cc (right):
> 
>
https://codereview.chromium.org/1819753003/diff/280001/cc/layers/heads_up_dis...
> cc/layers/heads_up_display_layer.cc:26: SkFontStyle::kUpright_Slant);
> On 2016/05/19 21:54:59, danakj wrote:
> > why the upright?
> 
> This code has been changed on rebase. Now there are no changes to this file.

ping once more, what or who is this CL waiting on?

[Alexei Svitkine (slow), 2016-05-27 18:01:57]

Sounds like this CL already has all the necessary owners lgtm's - seems like it
can just be committed.

[Evan Stade, 2016-05-27 18:03:50]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 18:04:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/300001

[commit-bot: I haz the power, 2016-05-27 18:11:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 18:11:36]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mikus, 2016-05-30 07:39:12]

On 2016/05/27 18:11:36, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
>   ios-simulator on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

I'm waiting on the Skia part in https://codereview.chromium.org/1818043002/

[Mikus, 2016-06-01 08:20:53]

The CQ bit was checked by mboc@opera.com

[Mikus, 2016-06-01 08:20:54]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
tapted@chromium.org, sky@chromium.org, danakj@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1819753003/#ps320001
(title: "Linux & iOS fixes")

[commit-bot: I haz the power, 2016-06-01 08:21:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/320001

[commit-bot: I haz the power, 2016-06-01 08:36:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 08:36:22]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Mikus, 2016-06-01 08:51:31]

The CQ bit was checked by mboc@opera.com

[Mikus, 2016-06-01 08:51:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
sky@chromium.org, danakj@chromium.org, msw@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1819753003/#ps340001
(title: "Compile problems on ChromeOS fixed (at least some of those)")

[commit-bot: I haz the power, 2016-06-01 08:51:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/340001

[commit-bot: I haz the power, 2016-06-01 09:06:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 09:06:50]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Mikus, 2016-06-01 09:14:14]

The CQ bit was checked by mboc@opera.com

[Mikus, 2016-06-01 09:14:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
sky@chromium.org, danakj@chromium.org, msw@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1819753003/#ps360001
(title: "More compile fixes")

[commit-bot: I haz the power, 2016-06-01 09:14:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/360001

[Mikus, 2016-06-01 09:27:59]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 09:28:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/380001

[commit-bot: I haz the power, 2016-06-01 09:43:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 09:43:13]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Mikus, 2016-06-01 11:19:30]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 11:19:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/400001

[commit-bot: I haz the power, 2016-06-01 11:48:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 11:48:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Mikus, 2016-06-01 12:57:24]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 12:57:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/420001

[commit-bot: I haz the power, 2016-06-01 13:33:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 13:34:01]

Dry run: Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Mikus, 2016-06-01 13:38:33]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 13:38:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/440001

[commit-bot: I haz the power, 2016-06-01 14:17:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 14:17:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mikus, 2016-06-01 15:42:37]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 15:43:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/460001

[commit-bot: I haz the power, 2016-06-01 16:23:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 16:23:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mikus, 2016-06-01 16:49:46]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 16:50:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/480001

[commit-bot: I haz the power, 2016-06-01 17:04:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 17:04:42]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[tapted, 2016-06-02 01:02:24]

https://codereview.chromium.org/1819753003/diff/480001/ui/gfx/render_text_mac.mm
File ui/gfx/render_text_mac.mm (left):

https://codereview.chromium.org/1819753003/diff/480001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:69: // Note: this is only used by RenderTextHarfbuzz.
looks like this comment got lost while resolving merge conflicts - please keep
it (if it changes, then that's bad since this function is computationally
expensive)

[Mikus, 2016-06-02 07:38:59]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 07:39:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/500001

[Mikus, 2016-06-02 07:47:34]

The CQ bit was unchecked by mboc@opera.com

[Mikus, 2016-06-02 07:48:34]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 07:48:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/520001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/520001

[Mikus, 2016-06-02 08:19:22]

The CQ bit was unchecked by mboc@opera.com

[Mikus, 2016-06-02 08:21:02]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 08:21:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/540001

[commit-bot: I haz the power, 2016-06-02 10:01:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 10:01:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Mikus, 2016-06-02 10:19:42]

The CQ bit was checked by mboc@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 10:20:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/560001

[Mikus, 2016-06-02 10:35:20]

https://codereview.chromium.org/1819753003/diff/480001/ui/gfx/render_text_mac.mm
File ui/gfx/render_text_mac.mm (left):

https://codereview.chromium.org/1819753003/diff/480001/ui/gfx/render_text_mac...
ui/gfx/render_text_mac.mm:69: // Note: this is only used by RenderTextHarfbuzz.
On 2016/06/02 01:02:23, tapted wrote:
> looks like this comment got lost while resolving merge conflicts - please keep
> it (if it changes, then that's bad since this function is computationally
> expensive)

Done.

[commit-bot: I haz the power, 2016-06-02 11:32:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 11:32:42]

Dry run: This issue passed the CQ dry run.

[Mikus, 2016-06-02 11:35:19]

The CQ bit was checked by mboc@opera.com

[Mikus, 2016-06-02 11:35:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
sky@chromium.org, danakj@chromium.org, msw@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1819753003/#ps560001
(title: "Add a lost comment and modify a render text unittest to not test black
because of test env font con…")

[commit-bot: I haz the power, 2016-06-02 11:35:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1819753003/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1819753003/560001

[commit-bot: I haz the power, 2016-06-02 11:40:50]

Description was changed from

==========
Allow various font weights in gfx. These changes make Chromium's gfx::Font more
closely match native font APIs &
capabilities.

BUG=597533
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow various font weights in gfx. These changes make Chromium's gfx::Font more
closely match native font APIs &
capabilities.

BUG=597533
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-02 11:40:54]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2016-06-02 11:42:08]

Description was changed from

==========
Allow various font weights in gfx. These changes make Chromium's gfx::Font more
closely match native font APIs &
capabilities.

BUG=597533
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow various font weights in gfx. These changes make Chromium's gfx::Font more
closely match native font APIs &
capabilities.

BUG=597533
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/998e890d117912669e3d31ff21b465ee3684966c
Cr-Commit-Position: refs/heads/master@{#397368}
==========

[commit-bot: I haz the power, 2016-06-02 11:42:11]

Patchset 29 (id:??) landed as
https://crrev.com/998e890d117912669e3d31ff21b465ee3684966c
Cr-Commit-Position: refs/heads/master@{#397368}