chromeos: Enable RenderTextHarfBuzz.

chromeos: Enable RenderTextHarfBuzz.

Enable RenderTextHarfBuzz on Chrome OS.

BUG=321868, 376077, 423791
Committed: https://crrev.com/7dc97b1cb620f2aba3be293af5019832d9bc00b7
Cr-Commit-Position: refs/heads/master@{#300348}

[Daniel Erat, 2014-10-07 18:22:50]

derat@chromium.org changed reviewers:
+ ckocagil@chromium.org, msw@chromium.org

[Daniel Erat, 2014-10-07 18:22:50]

i've verified that english and chinese tabs look fine on a lumpy build;
currently building link to verify it there as well.

[msw, 2014-10-07 18:29:01]

You might want to cite the meta bug or individual blockers for enabling on
Windows in a comment. Also, Cem left RTHB disabled on CrOS in
<https://codereview.chromium.org/630583002>, I wonder if there's a big blocker
there? If not, this LGTM with nits.

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

https://codereview.chromium.org/634983002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:401: #if defined(OS_LINUX)
nit: elif?

https://codereview.chromium.org/634983002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:414: return CreateNativeInstance();
nit q: will this trigger unreachable code warnings on Mac Views builds?

[Daniel Erat, 2014-10-08 02:03:43]

thanks, i've added a TODO referencing the meta bug. i didn't see any chrome os
blockers (both from my own testing and in the meta bug) and i'm extremely eager
to get this in, so i'm going to commit and either address things that come up or
revert if necessary.

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

https://codereview.chromium.org/634983002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:401: #if defined(OS_LINUX)
On 2014/10/07 18:29:01, msw wrote:
> nit: elif?

sure, done

https://codereview.chromium.org/634983002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:414: return CreateNativeInstance();
On 2014/10/07 18:29:01, msw wrote:
> nit q: will this trigger unreachable code warnings on Mac Views builds?

good point. i think that nested ifdefs are pretty hard to read, so i'm hopeful
that this works, but i'll change it if it doesn't.

[Daniel Erat, 2014-10-08 02:05:23]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-10-08 02:06:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634983002/20001

[commit-bot: I haz the power, 2014-10-08 03:24:44]

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

[commit-bot: I haz the power, 2014-10-08 03:24:44]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ckocagil, 2014-10-08 07:12:27]

On 2014/10/08 03:24:44, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

There's no blocker I'm aware of other than this mysterious crash.

[ckocagil, 2014-10-15 16:30:07]

Is it fine with you if I enable HarfBuzz on Linux and Win at
https://codereview.chromium.org/630583002/ and leave CrOS to this CL?

[Daniel Erat, 2014-10-15 16:37:11]

On 2014/10/15 16:30:07, ckocagil wrote:
> Is it fine with you if I enable HarfBuzz on Linux and Win at
> https://codereview.chromium.org/630583002/ and leave CrOS to this CL?

sure! can you enable it on win 8.1 as well now? is chrome os the only place
where it's still blocked?

[ckocagil, 2014-10-15 16:39:33]

On 2014/10/15 16:37:11, Daniel Erat wrote:
> On 2014/10/15 16:30:07, ckocagil wrote:
> > Is it fine with you if I enable HarfBuzz on Linux and Win at
> > https://codereview.chromium.org/630583002/ and leave CrOS to this CL?
> 
> sure! can you enable it on win 8.1 as well now? is chrome os the only place
> where it's still blocked?

https://code.google.com/p/chromium/issues/detail?id=402347#c42 says it is fixed
on Win 8.1. So yeah only CrOS is blocked.

[Daniel Erat, 2014-10-15 17:26:40]

On 2014/10/15 16:39:33, ckocagil wrote:
> On 2014/10/15 16:37:11, Daniel Erat wrote:
> > On 2014/10/15 16:30:07, ckocagil wrote:
> > > Is it fine with you if I enable HarfBuzz on Linux and Win at
> > > https://codereview.chromium.org/630583002/ and leave CrOS to this CL?
> > 
> > sure! can you enable it on win 8.1 as well now? is chrome os the only place
> > where it's still blocked?
> 
> https://code.google.com/p/chromium/issues/detail?id=402347#c42 says it is
fixed
> on Win 8.1. So yeah only CrOS is blocked.

so just to confirm: http://crbug.com/403892 and http://crbug.com/396415 don't
block this on windows?

[ckocagil, 2014-10-15 17:30:48]

On 2014/10/15 17:26:40, Daniel Erat wrote:
> so just to confirm: http://crbug.com/403892 and http://crbug.com/396415 don't
> block this on windows?

No, we don't consider either of them to be blockers.

[Daniel Erat, 2014-10-20 15:14:10]

ptal -- let me know how you feel about disabling the timing-out tests on chrome
os (they're disabled on windows now, too).

[ckocagil, 2014-10-20 15:21:33]

On 2014/10/20 15:14:10, Daniel Erat wrote:
> ptal -- let me know how you feel about disabling the timing-out tests on
chrome
> os (they're disabled on windows now, too).

Maybe you'd want to make sure Linux/CrOS doesn't ship with (or depend on) the
fontconfig version that causes this bug.

In either case this looks like a case of bad tests, so this lgtm.

[Daniel Erat, 2014-10-20 15:30:29]

On 2014/10/20 15:21:33, ckocagil wrote:
> On 2014/10/20 15:14:10, Daniel Erat wrote:
> > ptal -- let me know how you feel about disabling the timing-out tests on
> chrome
> > os (they're disabled on windows now, too).
> 
> Maybe you'd want to make sure Linux/CrOS doesn't ship with (or depend on) the
> fontconfig version that causes this bug.
> 
> In either case this looks like a case of bad tests, so this lgtm.

chrome os uses a much-newer fontconfig (2.11.1). i'm not able to repro the
failures on my trusty workstation (2.11.0), but i get timeouts when i use a
locally-built fontconfig at 2.8.0, which is the same old version that precise
(which the bots are running) ships with.

[Daniel Erat, 2014-10-20 15:30:37]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-10-20 15:30:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634983002/60001

[Daniel Erat, 2014-10-20 15:30:54]

The CQ bit was unchecked by derat@chromium.org

[Daniel Erat, 2014-10-20 15:31:16]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-10-20 15:31:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634983002/60001

[commit-bot: I haz the power, 2014-10-20 15:40:59]

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

[commit-bot: I haz the power, 2014-10-20 15:41:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Erat, 2014-10-20 16:21:22]

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

[Daniel Erat, 2014-10-20 16:21:23]

Evan, mind doing an owner review for the test-disabling?

[Evan Stade, 2014-10-20 16:48:33]

Is harfbuzz making the tests time out?

[Daniel Erat, 2014-10-20 16:58:49]

On 2014/10/20 16:48:33, Evan Stade wrote:
> Is harfbuzz making the tests time out?

i think that RenderTextHarfBuzz is doing something via skia that is causing
timeouts. i've added http://crbug.com/423791, which is the bug that i filed
about these tests.

[Daniel Erat, 2014-10-20 21:45:08]

On 2014/10/20 16:58:49, Daniel Erat wrote:
> On 2014/10/20 16:48:33, Evan Stade wrote:
> > Is harfbuzz making the tests time out?
> 
> i think that RenderTextHarfBuzz is doing something via skia that is causing
> timeouts. i've added http://crbug.com/423791, which is the bug that i filed
> about these tests.

any other comments? i'd like to get this turned on soon so we have plenty of
time to resolve potential issues before the branch.

[Evan Stade, 2014-10-20 21:49:36]

On 2014/10/20 21:45:08, Daniel Erat wrote:
> On 2014/10/20 16:58:49, Daniel Erat wrote:
> > On 2014/10/20 16:48:33, Evan Stade wrote:
> > > Is harfbuzz making the tests time out?
> > 
> > i think that RenderTextHarfBuzz is doing something via skia that is causing
> > timeouts. i've added http://crbug.com/423791, which is the bug that i filed
> > about these tests.
> 
> any other comments? i'd like to get this turned on soon so we have plenty of
> time to resolve potential issues before the branch.

lgtm I guess, but isn't the timeout a potential issue?

[Daniel Erat, 2014-10-20 22:02:08]

On 2014/10/20 21:49:36, Evan Stade wrote:
> On 2014/10/20 21:45:08, Daniel Erat wrote:
> > On 2014/10/20 16:58:49, Daniel Erat wrote:
> > > On 2014/10/20 16:48:33, Evan Stade wrote:
> > > > Is harfbuzz making the tests time out?
> > > 
> > > i think that RenderTextHarfBuzz is doing something via skia that is
causing
> > > timeouts. i've added http://crbug.com/423791, which is the bug that i
filed
> > > about these tests.
> > 
> > any other comments? i'd like to get this turned on soon so we have plenty of
> > time to resolve potential issues before the branch.
> 
> lgtm I guess, but isn't the timeout a potential issue?

yes, it is. it may be related to e.g. http://crbug.com/402570. given that
RenderTextPango has resulted in extremely laggy omnibox performance across
multiple releases of linux and chrome os (http://crbug.com/376077), i'm willing
to trade that real issue away for potential issues, though. :-/

[Daniel Erat, 2014-10-20 22:02:13]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-10-20 22:03:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634983002/60001

[commit-bot: I haz the power, 2014-10-20 22:08:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-20 22:09:33]

Patchset 4 (id:??) landed as
https://crrev.com/7dc97b1cb620f2aba3be293af5019832d9bc00b7
Cr-Commit-Position: refs/heads/master@{#300348}