RenderTextHarfBuzz: Set font render parameters in font data functions

RenderTextHarfBuzz: Set font render parameters in font data functions

- Properly pass font render settings to Skia font data functions. Otherwise Skia always returns rounded values.

- If subpixel positioning is off, round the glyph positions to match Pango's rounding logic.

BUG=402715, 402374, 402347
TEST=On Linux, character positions in UI text (address bar, infobars, tab titles) should be identical with --enable-harfbuzz-rendertext and --disable-harfbuzz-rendertext.
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290474

[Daniel Erat, 2014-08-15 18:34:25]

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

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1125: params.subpixel_rendering =
FontRenderParams::SUBPIXEL_RENDERING_NONE;
can you make ApplyRenderParams() check whether the background is transparent and
only do this there?

[ckocagil, 2014-08-15 19:05:57]

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

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1125: params.subpixel_rendering =
FontRenderParams::SUBPIXEL_RENDERING_NONE;
On 2014/08/15 18:34:25, Daniel Erat wrote:
> can you make ApplyRenderParams() check whether the background is transparent
and
> only do this there?

ApplyRenderParams doesn't know whether the background is transparent because it
isn't a RenderText member method. To make that work we'd have to move it into
RenderText and make it public so that it can be called from
<anon>::CreateHarfBuzzFont.

Another option is to add a "void
RenderText::AdjustFontRenderParams(FontRenderParams*)" that checks transparency
and sets the subpixel setting. We'd then call that whenever we get a
FontRenderParams.

[msw, 2014-08-15 19:21:17]

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

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:214: void
SkiaTextRenderer::SetFontRenderParams(FontRenderParams params,
Can we remove this function, inline its functionality into
SetFontFamilyWithStyle (and/or SetTypeface), and add a
set_background_is_transparent instead?

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

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:879: renderer.SetFontRenderParams(
Can we somehow use the run-specific |hb_font_data->paint_| setup in
CreateHarfBuzzFont, rather than setting a single FontRenderParams here based on
the PrimaryFont? (On Linux, the FontRenderParams can be font-specific).

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1125: params.subpixel_rendering =
FontRenderParams::SUBPIXEL_RENDERING_NONE;
On 2014/08/15 19:05:57, ckocagil wrote:
> On 2014/08/15 18:34:25, Daniel Erat wrote:
> > can you make ApplyRenderParams() check whether the background is transparent
> and
> > only do this there?
> 
> ApplyRenderParams doesn't know whether the background is transparent because
it
> isn't a RenderText member method. To make that work we'd have to move it into
> RenderText and make it public so that it can be called from
> <anon>::CreateHarfBuzzFont.
> 
> Another option is to add a "void
> RenderText::AdjustFontRenderParams(FontRenderParams*)" that checks
transparency
> and sets the subpixel setting. We'd then call that whenever we get a
> FontRenderParams.

I wonder if the pattern here is correct, given how different it is from the
other impls. Should we be using gfx::Font for things like GetFontRenderParams?

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1161: run->width = std::floor(run->width + 0.5f);
Whoa, this seems really bad... This rounds the run width each time a glyph's
width is added. Can we avoid this rounding altogether? If not, can we simply
round the overall run width after this glyph loop? Please explain why any
rounding is needed.

[msw, 2014-08-15 19:22:21]

+CC oshima for heads up ( see http://crbug.com/402715#c10 )

[ckocagil, 2014-08-15 22:28:01]

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

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:214: void
SkiaTextRenderer::SetFontRenderParams(FontRenderParams params,
On 2014/08/15 19:21:17, msw wrote:
> Can we remove this function, inline its functionality into
> SetFontFamilyWithStyle (and/or SetTypeface), and add a
> set_background_is_transparent instead?

We only call this on Win and Linux, so we have to put ifdefs around calls to
this function. Merging it into SetFontFamilyWithStyle would make that awkward.

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

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:879: renderer.SetFontRenderParams(
On 2014/08/15 19:21:17, msw wrote:
> Can we somehow use the run-specific |hb_font_data->paint_| setup in
> CreateHarfBuzzFont, rather than setting a single FontRenderParams here based
on
> the PrimaryFont? (On Linux, the FontRenderParams can be font-specific).

We're now calculating FontRenderParams once while shaping and store it in the
run, then use it when we need to draw.

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1125: params.subpixel_rendering =
FontRenderParams::SUBPIXEL_RENDERING_NONE;
On 2014/08/15 19:21:17, msw wrote:
> On 2014/08/15 19:05:57, ckocagil wrote:
> > On 2014/08/15 18:34:25, Daniel Erat wrote:
> > > can you make ApplyRenderParams() check whether the background is
transparent
> > and
> > > only do this there?
> > 
> > ApplyRenderParams doesn't know whether the background is transparent because
> it
> > isn't a RenderText member method. To make that work we'd have to move it
into
> > RenderText and make it public so that it can be called from
> > <anon>::CreateHarfBuzzFont.
> > 
> > Another option is to add a "void
> > RenderText::AdjustFontRenderParams(FontRenderParams*)" that checks
> transparency
> > and sets the subpixel setting. We'd then call that whenever we get a
> > FontRenderParams.
> 
> I wonder if the pattern here is correct, given how different it is from the
> other impls. Should we be using gfx::Font for things like GetFontRenderParams?

gfx::Font (as it currently is) is kinda heavyweight and other than a few things
like GetFontRenderParams, it is completely unnecessary to have in RTHB.

Ideally we'd have something like PlatformFontSkia. Then we'd be able to use
gfx::Font like other impls. Perhaps a TODO will suffice for now?

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1161: run->width = std::floor(run->width + 0.5f);
On 2014/08/15 19:21:17, msw wrote:
> Whoa, this seems really bad... This rounds the run width each time a glyph's
> width is added. Can we avoid this rounding altogether? If not, can we simply
> round the overall run width after this glyph loop? Please explain why any
> rounding is needed.

This rounding is needed to place the glyphs at integer values, which is what
Pango does. And it can't be just floor or ceil, it has to be rounded to the
nearest integer (I tried).

[Daniel Erat, 2014-08-15 22:31:30]

On 2014/08/15 22:28:01, ckocagil wrote:
> https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text.cc
> File ui/gfx/render_text.cc (right):
> 
>
https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text.cc#newcod...
> ui/gfx/render_text.cc:214: void
> SkiaTextRenderer::SetFontRenderParams(FontRenderParams params,
> On 2014/08/15 19:21:17, msw wrote:
> > Can we remove this function, inline its functionality into
> > SetFontFamilyWithStyle (and/or SetTypeface), and add a
> > set_background_is_transparent instead?
> 
> We only call this on Win and Linux, so we have to put ifdefs around calls to
> this function. Merging it into SetFontFamilyWithStyle would make that awkward.
> 
>
https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz.cc
> File ui/gfx/render_text_harfbuzz.cc (right):
> 
>
https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
> ui/gfx/render_text_harfbuzz.cc:879: renderer.SetFontRenderParams(
> On 2014/08/15 19:21:17, msw wrote:
> > Can we somehow use the run-specific |hb_font_data->paint_| setup in
> > CreateHarfBuzzFont, rather than setting a single FontRenderParams here based
> on
> > the PrimaryFont? (On Linux, the FontRenderParams can be font-specific).
> 
> We're now calculating FontRenderParams once while shaping and store it in the
> run, then use it when we need to draw.
> 
>
https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
> ui/gfx/render_text_harfbuzz.cc:1125: params.subpixel_rendering =
> FontRenderParams::SUBPIXEL_RENDERING_NONE;
> On 2014/08/15 19:21:17, msw wrote:
> > On 2014/08/15 19:05:57, ckocagil wrote:
> > > On 2014/08/15 18:34:25, Daniel Erat wrote:
> > > > can you make ApplyRenderParams() check whether the background is
> transparent
> > > and
> > > > only do this there?
> > > 
> > > ApplyRenderParams doesn't know whether the background is transparent
because
> > it
> > > isn't a RenderText member method. To make that work we'd have to move it
> into
> > > RenderText and make it public so that it can be called from
> > > <anon>::CreateHarfBuzzFont.
> > > 
> > > Another option is to add a "void
> > > RenderText::AdjustFontRenderParams(FontRenderParams*)" that checks
> > transparency
> > > and sets the subpixel setting. We'd then call that whenever we get a
> > > FontRenderParams.
> > 
> > I wonder if the pattern here is correct, given how different it is from the
> > other impls. Should we be using gfx::Font for things like
GetFontRenderParams?
> 
> gfx::Font (as it currently is) is kinda heavyweight and other than a few
things
> like GetFontRenderParams, it is completely unnecessary to have in RTHB.
> 
> Ideally we'd have something like PlatformFontSkia. Then we'd be able to use
> gfx::Font like other impls. Perhaps a TODO will suffice for now?

i'll add you to http://crbug.com/398885, my PlatformFontSkia bug. :-P

>
https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
> ui/gfx/render_text_harfbuzz.cc:1161: run->width = std::floor(run->width +
0.5f);
> On 2014/08/15 19:21:17, msw wrote:
> > Whoa, this seems really bad... This rounds the run width each time a glyph's
> > width is added. Can we avoid this rounding altogether? If not, can we simply
> > round the overall run width after this glyph loop? Please explain why any
> > rounding is needed.
> 
> This rounding is needed to place the glyphs at integer values, which is what
> Pango does. And it can't be just floor or ceil, it has to be rounded to the
> nearest integer (I tried).

[msw, 2014-08-16 00:30:11]

Please add a CL description, TEST= and run the trybots.

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

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:879: renderer.SetFontRenderParams(
On 2014/08/15 22:28:01, ckocagil wrote:
> On 2014/08/15 19:21:17, msw wrote:
> > Can we somehow use the run-specific |hb_font_data->paint_| setup in
> > CreateHarfBuzzFont, rather than setting a single FontRenderParams here based
> on
> > the PrimaryFont? (On Linux, the FontRenderParams can be font-specific).
> 
> We're now calculating FontRenderParams once while shaping and store it in the
> run, then use it when we need to draw.

Nice.

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1125: params.subpixel_rendering =
FontRenderParams::SUBPIXEL_RENDERING_NONE;
On 2014/08/15 22:28:01, ckocagil wrote:
> On 2014/08/15 19:21:17, msw wrote:
> > On 2014/08/15 19:05:57, ckocagil wrote:
> > > On 2014/08/15 18:34:25, Daniel Erat wrote:
> > > > can you make ApplyRenderParams() check whether the background is
> transparent
> > > and
> > > > only do this there?
> > > 
> > > ApplyRenderParams doesn't know whether the background is transparent
because
> > it
> > > isn't a RenderText member method. To make that work we'd have to move it
> into
> > > RenderText and make it public so that it can be called from
> > > <anon>::CreateHarfBuzzFont.
> > > 
> > > Another option is to add a "void
> > > RenderText::AdjustFontRenderParams(FontRenderParams*)" that checks
> > transparency
> > > and sets the subpixel setting. We'd then call that whenever we get a
> > > FontRenderParams.
> > 
> > I wonder if the pattern here is correct, given how different it is from the
> > other impls. Should we be using gfx::Font for things like
GetFontRenderParams?
> 
> gfx::Font (as it currently is) is kinda heavyweight and other than a few
things
> like GetFontRenderParams, it is completely unnecessary to have in RTHB.
> 
> Ideally we'd have something like PlatformFontSkia. Then we'd be able to use
> gfx::Font like other impls. Perhaps a TODO will suffice for now?

Acknowledged; if you see a specific reason to add a TODO, go for it.

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1161: run->width = std::floor(run->width + 0.5f);
On 2014/08/15 22:28:01, ckocagil wrote:
> On 2014/08/15 19:21:17, msw wrote:
> > Whoa, this seems really bad... This rounds the run width each time a glyph's
> > width is added. Can we avoid this rounding altogether? If not, can we simply
> > round the overall run width after this glyph loop? Please explain why any
> > rounding is needed.
> 
> This rounding is needed to place the glyphs at integer values, which is what
> Pango does. And it can't be just floor or ceil, it has to be rounded to the
> nearest integer (I tried).

Yuck, please add a comment then. Would we get the same results disabling
subpixel positioning instead? That might be a better [temporary] solution.

While Pango's glyph positioning does seem to look better in the Linux Desktop
screenshots I posted < http://crbug.com/402715 >, I think this will break
CanvasTestMac.*, and I wonder how it affects the look on Mac, Windows, and
Chrome OS (especially with high-dpi). Can you post native/HarfBuzz screenshots
on any of those OSes, similar to the ones I posted? I guess we really want to
achieve parity first and follow up with potential improvements.

https://codereview.chromium.org/480533002/diff/20001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/480533002/diff/20001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1122: run->render_params.subpixel_rendering =
Is this needed for the SkPaint's settings here, or is passing
background_is_transparent() to SkiaTextRenderer::SetFontRenderParams in
DrawVisualText sufficient?

[ckocagil, 2014-08-16 14:01:15]

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

https://codereview.chromium.org/480533002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1161: run->width = std::floor(run->width + 0.5f);
On 2014/08/16 00:30:11, msw wrote:
> On 2014/08/15 22:28:01, ckocagil wrote:
> > On 2014/08/15 19:21:17, msw wrote:
> > > Whoa, this seems really bad... This rounds the run width each time a
glyph's
> > > width is added. Can we avoid this rounding altogether? If not, can we
simply
> > > round the overall run width after this glyph loop? Please explain why any
> > > rounding is needed.
> > 
> > This rounding is needed to place the glyphs at integer values, which is what
> > Pango does. And it can't be just floor or ceil, it has to be rounded to the
> > nearest integer (I tried).
> 
> Yuck, please add a comment then. Would we get the same results disabling
> subpixel positioning instead? That might be a better [temporary] solution.
> 
> While Pango's glyph positioning does seem to look better in the Linux Desktop
> screenshots I posted < http://crbug.com/402715 >, I think this will break
> CanvasTestMac.*, and I wonder how it affects the look on Mac, Windows, and
> Chrome OS (especially with high-dpi). Can you post native/HarfBuzz screenshots
> on any of those OSes, similar to the ones I posted? I guess we really want to
> achieve parity first and follow up with potential improvements.

Subpixel positioning seems to be always disabled on Linux. The problem here is,
Skia's rounding logic doesn't match Pango's, so we have to roll our own.

I fixed this to only do the rounding if subpixel positioning is off. I'll add
the screenshots of different rounding methods to http://crbug.com/402715.

https://codereview.chromium.org/480533002/diff/20001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/480533002/diff/20001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1122: run->render_params.subpixel_rendering =
On 2014/08/16 00:30:11, msw wrote:
> Is this needed for the SkPaint's settings here, or is passing
> background_is_transparent() to SkiaTextRenderer::SetFontRenderParams in
> DrawVisualText sufficient?

I prefer using the exact same font render params for shaping and drawing.

[msw, 2014-08-16 20:02:47]

Okay, lgtm

[ckocagil, 2014-08-16 20:25:00]

Daniel, how about you?

[Daniel Erat, 2014-08-17 01:53:35]

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

https://codereview.chromium.org/480533002/diff/40001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:214: void
SkiaTextRenderer::SetFontRenderParams(FontRenderParams params,
i'd prefer that you left this as a const reference; it feels strange to me to
change the public interface to do a copy to accommodate a corner case of the
implementation.

i'd also still prefer that the
disable-subpixel-rendering-for-transparent-backgrounds logic only lived in one
place. regarding the earlier discussion: i meant that ApplyRenderParams() could
also take a background_is_transparent argument. is there a reason that that
wouldn't work?

void SetFontRenderParams(const FontRenderParams& params,
                         bool background_is_transparent) {
  ApplyRenderParams(params, background_is_transparent, &paint_);
}

void ApplyRenderParams(const FontRenderParams& params,
                       bool background_is_transparent,
                       SkPaint* paint) {
  ...
  paint->setLCDRenderText(!background_is_transparent &&
      params.subpixel_rendering != FontRenderParams::SUBPIXEL_RENDERING_NONE);
}

and then pass background_is_transparent through CreateHarfBuzzFont() as well, if
that doesn't seem too gross.

(i take it that there's no way to determine from the SkPaint that the
background's going to be transparent at this point, right?)

[ckocagil, 2014-08-18 20:01:22]

Mike: this CL also fixes http://crbug.com/402374 and http://crbug.com/402347.
Can these be due to the text elider using Pango, hence expecting Pango widths?

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

https://codereview.chromium.org/480533002/diff/40001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:214: void
SkiaTextRenderer::SetFontRenderParams(FontRenderParams params,
On 2014/08/17 01:53:35, Daniel Erat wrote:
> i'd prefer that you left this as a const reference; it feels strange to me to
> change the public interface to do a copy to accommodate a corner case of the
> implementation.
> 
> i'd also still prefer that the
> disable-subpixel-rendering-for-transparent-backgrounds logic only lived in one
> place. regarding the earlier discussion: i meant that ApplyRenderParams()
could
> also take a background_is_transparent argument. is there a reason that that
> wouldn't work?
> 
> void SetFontRenderParams(const FontRenderParams& params,
>                          bool background_is_transparent) {
>   ApplyRenderParams(params, background_is_transparent, &paint_);
> }
> 
> void ApplyRenderParams(const FontRenderParams& params,
>                        bool background_is_transparent,
>                        SkPaint* paint) {
>   ...
>   paint->setLCDRenderText(!background_is_transparent &&
>       params.subpixel_rendering != FontRenderParams::SUBPIXEL_RENDERING_NONE);
> }
> 
> and then pass background_is_transparent through CreateHarfBuzzFont() as well,
if
> that doesn't seem too gross.
> 
> (i take it that there's no way to determine from the SkPaint that the
> background's going to be transparent at this point, right?)

Done!

[Daniel Erat, 2014-08-18 20:03:17]

lgtm

thanks!

[ckocagil, 2014-08-19 00:28:01]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-08-19 00:29:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/480533002/60001

[msw, 2014-08-19 00:49:21]

On 2014/08/18 20:01:22, ckocagil wrote:
> Mike: this CL also fixes http://crbug.com/402374 and http://crbug.com/402347.
> Can these be due to the text elider using Pango, hence expecting Pango widths?
> 

Great! Still LGTM, but you should include those bug numbers in the BUG=.
It makes sense that hb_font_data's SkPaint needed that config for proper sizing.
(I don't think it was a Pango vs. HarfBuzz usage problem at all)

[commit-bot: I haz the power, 2014-08-19 01:52:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43351)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-19 01:57:05]

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

[commit-bot: I haz the power, 2014-08-19 01:57:06]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ckocagil, 2014-08-19 02:29:57]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-08-19 02:30:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/480533002/60001

[ckocagil, 2014-08-19 02:31:20]

The CQ bit was unchecked by ckocagil@chromium.org

[ckocagil, 2014-08-19 02:34:23]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-08-19 02:35:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/480533002/60001

[commit-bot: I haz the power, 2014-08-19 02:49:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43398)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-19 02:53:34]

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

[commit-bot: I haz the power, 2014-08-19 02:53:35]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ckocagil, 2014-08-19 03:10:19]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-08-19 03:12:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/480533002/60001

[ckocagil, 2014-08-19 03:25:58]

Compile failures seem related to these:
https://code.google.com/p/chromium/issues/detail?id=395477
https://code.google.com/p/chromium/issues/detail?id=399842

I'll land with NOTRY, hopefully this won't set the tree on fire.

[ckocagil, 2014-08-19 03:26:27]

The CQ bit was unchecked by ckocagil@chromium.org

[ckocagil, 2014-08-19 03:26:33]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-08-19 03:26:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/480533002/60001

[commit-bot: I haz the power, 2014-08-19 03:28:00]

Committed patchset #4 (60001) as 290474