Support sideloaded fonts via command line option for blink layout_tests

Support sideloaded fonts via command line option for blink layout_tests

Blink side is: https://codereview.chromium.org/235553003/ which
must land and roll first.

Adds support for --register-font-files which adds the given
file(s) to the sandbox policy (readonly), creates and warms up
the font, and passes it to blink.

Also, modify the setup code for layout tests so "Ahem" will be
found correctly.

R=jschuh@chromium.org,eae@chromium.org
BUG=333029
TEST=out\debug\content_shell d:\src\tmp\a.html --enable-direct-write --register-font-files=D:\src\cr3\src\content\shell\renderer\test_runner\resources\fonts\AHEM____.TTF
TEST=ninja -C out\Debug blink_tests && webkit\tools\layout_tests\run_webkit_tests fast/parser/fonts.html --debug --nocheck-sys-deps (with directwrite forced on)

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

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

[jschuh, 2014-04-11 20:28:40]

Clever. lgtm

[scottmg, 2014-04-12 16:39:09]

+jam OWNERS (for non-sandbox plumbing parts)

[jam, 2014-04-14 05:40:01]

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
content/common/sandbox_win.cc:587: files->clear();
nit: no need for return value and out parameter, just return the std::vector.
then you don't even need the "clear" call

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
content/common/sandbox_win.cc:679: base::UTF8ToWide(i->c_str()).c_str());
nit: the first c_str() isn't needed

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
File content/common/sandbox_win.h (right):

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
content/common/sandbox_win.h:40: bool
GetSideloadFontFiles(std::vector<std::string>* files);
nit: please add documentation for both of these methods

https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:102: // Blink layout
tests.
since this is only for layout tests, can you do this entire change in
content/shell? i.e. content embedders can modify the sandbox policy in
ContentBrowserClient::PreSpawnRenderer

[scottmg, 2014-04-14 15:48:58]

Thanks

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
content/common/sandbox_win.cc:587: files->clear();
On 2014/04/14 05:40:02, jam wrote:
> nit: no need for return value and out parameter, just return the std::vector.
> then you don't even need the "clear" call

Done.

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
content/common/sandbox_win.cc:679: base::UTF8ToWide(i->c_str()).c_str());
On 2014/04/14 05:40:02, jam wrote:
> nit: the first c_str() isn't needed

Done.

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
File content/common/sandbox_win.h (right):

https://codereview.chromium.org/231763003/diff/120001/content/common/sandbox_...
content/common/sandbox_win.h:40: bool
GetSideloadFontFiles(std::vector<std::string>* files);
On 2014/04/14 05:40:02, jam wrote:
> nit: please add documentation for both of these methods

Done.

https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:102: // Blink layout
tests.
On 2014/04/14 05:40:02, jam wrote:
> since this is only for layout tests, can you do this entire change in
> content/shell? i.e. content embedders can modify the sandbox policy in
> ContentBrowserClient::PreSpawnRenderer

It's mostly for Blink but is useful for testing to have it in Chrome. There's
also font managers which could use this, ref:
https://codereview.chromium.org/209163002#msg7

OTOH, it's nicer to keep it out of Chrome. Justin, any opinions either way?

[jschuh, 2014-04-14 15:58:51]

https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:102: // Blink layout
tests.
On 2014/04/14 15:48:58, scottmg wrote:
> On 2014/04/14 05:40:02, jam wrote:
> > since this is only for layout tests, can you do this entire change in
> > content/shell? i.e. content embedders can modify the sandbox policy in
> > ContentBrowserClient::PreSpawnRenderer
> 
> It's mostly for Blink but is useful for testing to have it in Chrome. There's
> also font managers which could use this, ref:
> https://codereview.chromium.org/209163002#msg7
> 
> OTOH, it's nicer to keep it out of Chrome. Justin, any opinions either way?

I'm inclined to say keep it in Chrome just to make testing easier, but I don't
feel strongly either way.

[jam, 2014-04-14 16:20:59]

https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:102: // Blink layout
tests.
On 2014/04/14 15:58:51, Justin Schuh wrote:
> On 2014/04/14 15:48:58, scottmg wrote:
> > On 2014/04/14 05:40:02, jam wrote:
> > > since this is only for layout tests, can you do this entire change in
> > > content/shell? i.e. content embedders can modify the sandbox policy in
> > > ContentBrowserClient::PreSpawnRenderer
> > 
> > It's mostly for Blink but is useful for testing to have it in Chrome.
There's
> > also font managers which could use this, ref:
> > https://codereview.chromium.org/209163002#msg7
> > 
> > OTOH, it's nicer to keep it out of Chrome. Justin, any opinions either way?
> 
> I'm inclined to say keep it in Chrome just to make testing easier, but I don't
> feel strongly either way.

note that testing could still be done using content_shell.

i prefer we don't put it outside content/shell for now, until we have a need
otherwise.

[scottmg, 2014-04-14 17:22:00]

On 2014/04/14 16:20:59, jam wrote:
>
https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
> File content/renderer/renderer_main_platform_delegate_win.cc (right):
> 
>
https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
> content/renderer/renderer_main_platform_delegate_win.cc:102: // Blink layout
> tests.
> On 2014/04/14 15:58:51, Justin Schuh wrote:
> > On 2014/04/14 15:48:58, scottmg wrote:
> > > On 2014/04/14 05:40:02, jam wrote:
> > > > since this is only for layout tests, can you do this entire change in
> > > > content/shell? i.e. content embedders can modify the sandbox policy in
> > > > ContentBrowserClient::PreSpawnRenderer
> > > 
> > > It's mostly for Blink but is useful for testing to have it in Chrome.
> There's
> > > also font managers which could use this, ref:
> > > https://codereview.chromium.org/209163002#msg7
> > > 
> > > OTOH, it's nicer to keep it out of Chrome. Justin, any opinions either
way?
> > 
> > I'm inclined to say keep it in Chrome just to make testing easier, but I
don't
> > feel strongly either way.
> 
> note that testing could still be done using content_shell.
> 
> i prefer we don't put it outside content/shell for now, until we have a need
> otherwise.

Ok, the need was the above message, but someone else can fight that battle if
they want.

I started trying to move it to content/shell (is ps#8 roughly what you meant?),
but I'm not sure where to do the pre-sandbox warmup that's in
content/renderer/renderer_main_platform_delegate_win.cc now. Is there somewhere
in shell that already has a hook at the right point?

[jam, 2014-04-14 18:27:59]

On 2014/04/14 17:22:00, scottmg wrote:
> On 2014/04/14 16:20:59, jam wrote:
> >
>
https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
> > File content/renderer/renderer_main_platform_delegate_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
> > content/renderer/renderer_main_platform_delegate_win.cc:102: // Blink layout
> > tests.
> > On 2014/04/14 15:58:51, Justin Schuh wrote:
> > > On 2014/04/14 15:48:58, scottmg wrote:
> > > > On 2014/04/14 05:40:02, jam wrote:
> > > > > since this is only for layout tests, can you do this entire change in
> > > > > content/shell? i.e. content embedders can modify the sandbox policy in
> > > > > ContentBrowserClient::PreSpawnRenderer
> > > > 
> > > > It's mostly for Blink but is useful for testing to have it in Chrome.
> > There's
> > > > also font managers which could use this, ref:
> > > > https://codereview.chromium.org/209163002#msg7
> > > > 
> > > > OTOH, it's nicer to keep it out of Chrome. Justin, any opinions either
> way?
> > > 
> > > I'm inclined to say keep it in Chrome just to make testing easier, but I
> don't
> > > feel strongly either way.
> > 
> > note that testing could still be done using content_shell.
> > 
> > i prefer we don't put it outside content/shell for now, until we have a need
> > otherwise.
> 
> Ok, the need was the above message, but someone else can fight that battle if
> they want.

what I meant was that for testing, content_shell can be used instead of chrome.

> 
> I started trying to move it to content/shell (is ps#8 roughly what you
meant?),
> but I'm not sure where to do the pre-sandbox warmup that's in
> content/renderer/renderer_main_platform_delegate_win.cc now. Is there
somewhere
> in shell that already has a hook at the right point?

you can try doing it in ShellContentRendererClient::ShellContentRendererClient()

[scottmg, 2014-04-14 20:19:09]

On 2014/04/14 18:27:59, jam wrote:
> On 2014/04/14 17:22:00, scottmg wrote:
> > On 2014/04/14 16:20:59, jam wrote:
> > >
> >
>
https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
> > > File content/renderer/renderer_main_platform_delegate_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/231763003/diff/120001/content/renderer/render...
> > > content/renderer/renderer_main_platform_delegate_win.cc:102: // Blink
layout
> > > tests.
> > > On 2014/04/14 15:58:51, Justin Schuh wrote:
> > > > On 2014/04/14 15:48:58, scottmg wrote:
> > > > > On 2014/04/14 05:40:02, jam wrote:
> > > > > > since this is only for layout tests, can you do this entire change
in
> > > > > > content/shell? i.e. content embedders can modify the sandbox policy
in
> > > > > > ContentBrowserClient::PreSpawnRenderer
> > > > > 
> > > > > It's mostly for Blink but is useful for testing to have it in Chrome.
> > > There's
> > > > > also font managers which could use this, ref:
> > > > > https://codereview.chromium.org/209163002#msg7
> > > > > 
> > > > > OTOH, it's nicer to keep it out of Chrome. Justin, any opinions either
> > way?
> > > > 
> > > > I'm inclined to say keep it in Chrome just to make testing easier, but I
> > don't
> > > > feel strongly either way.
> > > 
> > > note that testing could still be done using content_shell.
> > > 
> > > i prefer we don't put it outside content/shell for now, until we have a
need
> > > otherwise.
> > 
> > Ok, the need was the above message, but someone else can fight that battle
if
> > they want.
> 
> what I meant was that for testing, content_shell can be used instead of
chrome.

I understood, but that message was referring to end users testing fonts, not
chrome devs.

> 
> > 
> > I started trying to move it to content/shell (is ps#8 roughly what you
> meant?),
> > but I'm not sure where to do the pre-sandbox warmup that's in
> > content/renderer/renderer_main_platform_delegate_win.cc now. Is there
> somewhere
> > in shell that already has a hook at the right point?
> 
> you can try doing it in
ShellContentRendererClient::ShellContentRendererClient()

Done. PTanotherL.

[jam, 2014-04-14 22:48:18]

lgtm

https://codereview.chromium.org/231763003/diff/190001/content/public/renderer...
File content/public/renderer/render_font_warmup_win.h (right):

https://codereview.chromium.org/231763003/diff/190001/content/public/renderer...
content/public/renderer/render_font_warmup_win.h:25: 
nit: extra line

[scottmg, 2014-04-14 22:50:20]

https://codereview.chromium.org/231763003/diff/190001/content/public/renderer...
File content/public/renderer/render_font_warmup_win.h (right):

https://codereview.chromium.org/231763003/diff/190001/content/public/renderer...
content/public/renderer/render_font_warmup_win.h:25: 
On 2014/04/14 22:48:18, jam wrote:
> nit: extra line

Done.

[scottmg, 2014-04-14 22:50:24]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-04-14 22:52:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/231763003/210001

[commit-bot: I haz the power, 2014-04-14 23:52:45]

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

[commit-bot: I haz the power, 2014-04-14 23:52:46]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[scottmg, 2014-04-14 23:55:28]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-04-14 23:56:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/231763003/210001

[commit-bot: I haz the power, 2014-04-15 08:44:01]

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

[commit-bot: I haz the power, 2014-04-15 08:44:02]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[eae, 2014-04-15 09:21:11]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-04-15 09:22:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/231763003/210001

[commit-bot: I haz the power, 2014-04-15 14:30:41]

Change committed as 263842

[leviw_travelin_and_unemployed, 2014-04-15 16:07:11]

Was this expected to change layout test results? It seems to have caused quite a
few failures (or at least expectation changes). Here are a few:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=css...

[scottmg, 2014-04-15 16:13:22]

On 2014/04/15 16:07:11, Levi wrote:
> Was this expected to change layout test results? It seems to have caused quite
a
> few failures (or at least expectation changes). Here are a few:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=css...

Hm, no, it wasn't. I guess it must be retrieving that font data during layout
tests even when in GDI mode. I will disable it right now.

[scottmg, 2014-04-15 16:42:20]

On 2014/04/15 16:13:22, scottmg wrote:
> On 2014/04/15 16:07:11, Levi wrote:
> > Was this expected to change layout test results? It seems to have caused
quite
> a
> > few failures (or at least expectation changes). Here are a few:
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=css...
> 
> Hm, no, it wasn't. I guess it must be retrieving that font data during layout
> tests even when in GDI mode. I will disable it right now.

This was disabled temporarily by
https://src.chromium.org/viewvc/chrome?view=rev&revision=263875

[scottmg, 2014-04-15 22:54:29]

Relanding with guard (for XP), diff is:

https://codereview.chromium.org/231763003/diff2/210001:230001/content/shell/r...

[scottmg, 2014-04-15 22:55:29]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-04-15 22:56:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/231763003/230001

[commit-bot: I haz the power, 2014-04-16 01:31:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/231763003/230001

[commit-bot: I haz the power, 2014-04-16 05:40:44]

Change committed as 264126