blink fonts: Load Android SkFontMgr on Linux.

blink fonts: Load Android SkFontMgr on Linux.

The renderer running on the engine for blimp uses the linux font config,
while it needs to support the use of android fonts when connected to an
android client. The patch includes 2 changes necessary for this.

1) The default mode for the android SkFontMgr accesses files on disk
which is not possible in the renderer sandbox. So we create the font
manager in the zygote process, before initializing the sanbox. The font
manager uses the isolated mode, so it can acquire all necessary resources
on initialization and does not perform any disk I/O after it.

2) Add an API to blink::WebFontRendering on linux to override the default
font manager. This allows us to override the default behaviour in
FontCacheSkia for linux to direct all queries for fallback fonts to the
embedder provided font manager.

BUG=585668
Committed: https://crrev.com/12a558c16f853751ab702c84fbd0ffd7de565cdc
Cr-Commit-Position: refs/heads/master@{#379631}

[Khushal, 2016-02-10 04:31:46]

khushalsagar@chromium.org changed reviewers:
+ bungeman@chromium.org

[Khushal, 2016-02-10 04:31:46]

Ben, could you take a look at this?

This depends on a similiar change to the windows API for WebFontRendering,
https://codereview.chromium.org/1591883002/.

[Khushal, 2016-02-10 18:31:20]

Description was changed from

==========
blink fonts: Load Android SkFontMgr on Linux.

The renderer running on the engine for blimp uses the linux font config,
while it needs to support the use of android fonts when connected to an
android client. The patch includes 2 changes necessary for this.

1) The default mode for the android SkFontMgr accesses files on disk
which is not possible in the renderer sandbox. So we create the font
manager in the zygote process, before initializing the sanbox. The font
manager uses the isolated mode, so it can acquire all necessary resources
on initialization and does not perform any disk I/O after it.

2) Add an API to blink::WebFontRendering on linux to override the default
font manager. This allows us to override the default behaviour in
FontCacheSkia for linux to direct all queries for fallback fonts to the
embedder provided font manager.

BUG=585668
==========

to

==========
blink fonts: Load Android SkFontMgr on Linux.

The renderer running on the engine for blimp uses the linux font config,
while it needs to support the use of android fonts when connected to an
android client. The patch includes 2 changes necessary for this.

1) The default mode for the android SkFontMgr accesses files on disk
which is not possible in the renderer sandbox. So we create the font
manager in the zygote process, before initializing the sanbox. The font
manager uses the isolated mode, so it can acquire all necessary resources
on initialization and does not perform any disk I/O after it.

2) Add an API to blink::WebFontRendering on linux to override the default
font manager. This allows us to override the default behaviour in
FontCacheSkia for linux to direct all queries for fallback fonts to the
embedder provided font manager.

BUG=585668
==========

[Khushal, 2016-02-11 19:09:15]

khushalsagar@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Khushal, 2016-02-17 18:37:53]

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

[commit-bot: I haz the power, 2016-02-17 18:40:41]

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

[commit-bot: I haz the power, 2016-02-17 19:00:41]

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

[commit-bot: I haz the power, 2016-02-17 19:00:42]

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-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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...)

[Khushal, 2016-02-17 22:08:00]

khushalsagar@chromium.org changed reviewers:
+ eae@chromium.org

[eae, 2016-02-17 23:52:10]

https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:108: #if
OS(ANDROID) || OS(LINUX)
Do we need this on Android? The only caller I can find is witihin a NOT(ANDROID)
&& LINUX block.

https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:159: if
(m_fontManager) {
When would we not have a font manager? On windows we have this check (for now)
as we need to handle the GDI case where no font manager is available.

[Khushal, 2016-02-18 00:46:37]

https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:108: #if
OS(ANDROID) || OS(LINUX)
On 2016/02/17 23:52:10, eae wrote:
> Do we need this on Android? The only caller I can find is witihin a
NOT(ANDROID)
> && LINUX block.

Android calls it from FontCacheAndroid. The implementation for
FontCache::fallbackFontForCharacter for android is defined there.

https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:159: if
(m_fontManager) {
On 2016/02/17 23:52:10, eae wrote:
> When would we not have a font manager? On windows we have this check (for now)
> as we need to handle the GDI case where no font manager is available.

On linux the default font manager is backed by an implementation of the
FontConfigInterface. We use SandboxSupport to query system fonts using IPC when
looking for fallback fonts. The response returned includes an
FontConfigInterface(fci) index which is understood by this font manager.

The m_fontManager check is for the case where the embedder needs to override the
default behaviour and ask blink to use a custom provided font manager instead.

Right now we need this for the case where we want linux to emulate android fonts
and need to pass in the android SkFontMgr. The override is done based on a
content switch in zygote_main_linux.cc.

[eae, 2016-02-18 17:57:01]

On 2016/02/18 00:46:37, Khushal wrote:
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):
> 
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:108: #if
> OS(ANDROID) || OS(LINUX)
> On 2016/02/17 23:52:10, eae wrote:
> > Do we need this on Android? The only caller I can find is witihin a
> NOT(ANDROID)
> > && LINUX block.
> 
> Android calls it from FontCacheAndroid. The implementation for
> FontCache::fallbackFontForCharacter for android is defined there.
> 
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:159: if
> (m_fontManager) {
> On 2016/02/17 23:52:10, eae wrote:
> > When would we not have a font manager? On windows we have this check (for
now)
> > as we need to handle the GDI case where no font manager is available.
> 
> On linux the default font manager is backed by an implementation of the
> FontConfigInterface. We use SandboxSupport to query system fonts using IPC
when
> looking for fallback fonts. The response returned includes an
> FontConfigInterface(fci) index which is understood by this font manager.
> 
> The m_fontManager check is for the case where the embedder needs to override
the
> default behaviour and ask blink to use a custom provided font manager instead.
> 
> Right now we need this for the case where we want linux to emulate android
fonts
> and need to pass in the android SkFontMgr. The override is done based on a
> content switch in zygote_main_linux.cc.

Wouldn't m_fontManager still be set in that case though? Just be the passed in
font manager instead of the default one?

[Khushal, 2016-02-18 22:37:17]

On 2016/02/18 17:57:01, eae wrote:
> On 2016/02/18 00:46:37, Khushal wrote:
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:108: #if
> > OS(ANDROID) || OS(LINUX)
> > On 2016/02/17 23:52:10, eae wrote:
> > > Do we need this on Android? The only caller I can find is witihin a
> > NOT(ANDROID)
> > > && LINUX block.
> > 
> > Android calls it from FontCacheAndroid. The implementation for
> > FontCache::fallbackFontForCharacter for android is defined there.
> > 
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:159: if
> > (m_fontManager) {
> > On 2016/02/17 23:52:10, eae wrote:
> > > When would we not have a font manager? On windows we have this check (for
> now)
> > > as we need to handle the GDI case where no font manager is available.
> > 
> > On linux the default font manager is backed by an implementation of the
> > FontConfigInterface. We use SandboxSupport to query system fonts using IPC
> when
> > looking for fallback fonts. The response returned includes an
> > FontConfigInterface(fci) index which is understood by this font manager.
> > 
> > The m_fontManager check is for the case where the embedder needs to override
> the
> > default behaviour and ask blink to use a custom provided font manager
instead.
> > 
> > Right now we need this for the case where we want linux to emulate android
> fonts
> > and need to pass in the android SkFontMgr. The override is done based on a
> > content switch in zygote_main_linux.cc.
> 
> Wouldn't m_fontManager still be set in that case though? Just be the passed in
> font manager instead of the default one?

On linux we only have the m_fontManager if the embedder passed us one using
WebFontRendering::setSkiaFontManager. In the default case the m_fontManager
should be null and we would go to FontCache::getFontForCharacter and the
implementation for this in FontCacheLinux would query the system using the
SandboxSupport.
I've updated the patch to make it more explicit.

[Khushal, 2016-02-19 03:18:07]

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

[commit-bot: I haz the power, 2016-02-19 03:20:14]

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

[commit-bot: I haz the power, 2016-02-19 04:02:00]

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

[commit-bot: I haz the power, 2016-02-19 04:02:01]

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

[Khushal, 2016-02-19 09:10:56]

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

[commit-bot: I haz the power, 2016-02-19 09:11:44]

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

[commit-bot: I haz the power, 2016-02-19 13:07:03]

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

[commit-bot: I haz the power, 2016-02-19 13:07:04]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[eae, 2016-02-22 18:34:31]

On 2016/02/18 22:37:17, Khushal wrote:
> On 2016/02/18 17:57:01, eae wrote:
> > On 2016/02/18 00:46:37, Khushal wrote:
> > >
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:108: #if
> > > OS(ANDROID) || OS(LINUX)
> > > On 2016/02/17 23:52:10, eae wrote:
> > > > Do we need this on Android? The only caller I can find is witihin a
> > > NOT(ANDROID)
> > > > && LINUX block.
> > > 
> > > Android calls it from FontCacheAndroid. The implementation for
> > > FontCache::fallbackFontForCharacter for android is defined there.
> > > 
> > >
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:159: if
> > > (m_fontManager) {
> > > On 2016/02/17 23:52:10, eae wrote:
> > > > When would we not have a font manager? On windows we have this check
(for
> > now)
> > > > as we need to handle the GDI case where no font manager is available.
> > > 
> > > On linux the default font manager is backed by an implementation of the
> > > FontConfigInterface. We use SandboxSupport to query system fonts using IPC
> > when
> > > looking for fallback fonts. The response returned includes an
> > > FontConfigInterface(fci) index which is understood by this font manager.
> > > 
> > > The m_fontManager check is for the case where the embedder needs to
override
> > the
> > > default behaviour and ask blink to use a custom provided font manager
> instead.
> > > 
> > > Right now we need this for the case where we want linux to emulate android
> > fonts
> > > and need to pass in the android SkFontMgr. The override is done based on a
> > > content switch in zygote_main_linux.cc.
> > 
> > Wouldn't m_fontManager still be set in that case though? Just be the passed
in
> > font manager instead of the default one?
> 
> On linux we only have the m_fontManager if the embedder passed us one using
> WebFontRendering::setSkiaFontManager. In the default case the m_fontManager
> should be null and we would go to FontCache::getFontForCharacter and the
> implementation for this in FontCacheLinux would query the system using the
> SandboxSupport.
> I've updated the patch to make it more explicit.

I don't like that we're adding yet another branch to the logic. How about we get
the default font manager if none is specified? That way we can use the same skia
calls in both cases.

Also, this code bypasses the synthetic bold and italics handling. Is that
intentional?

[Khushal, 2016-02-22 20:58:07]

On 2016/02/22 18:34:31, eae wrote:
> On 2016/02/18 22:37:17, Khushal wrote:
> > On 2016/02/18 17:57:01, eae wrote:
> > > On 2016/02/18 00:46:37, Khushal wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:108: #if
> > > > OS(ANDROID) || OS(LINUX)
> > > > On 2016/02/17 23:52:10, eae wrote:
> > > > > Do we need this on Android? The only caller I can find is witihin a
> > > > NOT(ANDROID)
> > > > > && LINUX block.
> > > > 
> > > > Android calls it from FontCacheAndroid. The implementation for
> > > > FontCache::fallbackFontForCharacter for android is defined there.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1685053002/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:159: if
> > > > (m_fontManager) {
> > > > On 2016/02/17 23:52:10, eae wrote:
> > > > > When would we not have a font manager? On windows we have this check
> (for
> > > now)
> > > > > as we need to handle the GDI case where no font manager is available.
> > > > 
> > > > On linux the default font manager is backed by an implementation of the
> > > > FontConfigInterface. We use SandboxSupport to query system fonts using
IPC
> > > when
> > > > looking for fallback fonts. The response returned includes an
> > > > FontConfigInterface(fci) index which is understood by this font manager.
> > > > 
> > > > The m_fontManager check is for the case where the embedder needs to
> override
> > > the
> > > > default behaviour and ask blink to use a custom provided font manager
> > instead.
> > > > 
> > > > Right now we need this for the case where we want linux to emulate
android
> > > fonts
> > > > and need to pass in the android SkFontMgr. The override is done based on
a
> > > > content switch in zygote_main_linux.cc.
> > > 
> > > Wouldn't m_fontManager still be set in that case though? Just be the
passed
> in
> > > font manager instead of the default one?
> > 
> > On linux we only have the m_fontManager if the embedder passed us one using
> > WebFontRendering::setSkiaFontManager. In the default case the m_fontManager
> > should be null and we would go to FontCache::getFontForCharacter and the
> > implementation for this in FontCacheLinux would query the system using the
> > SandboxSupport.
> > I've updated the patch to make it more explicit.
> 
> I don't like that we're adding yet another branch to the logic. How about we
get
> the default font manager if none is specified? That way we can use the same
skia
> calls in both cases.

Unfortunately we can not do that here since the behavior of the default font
manager on linux is different from the android manager. On linux, it would
always return null because it expects the code to query the system and uses fci
when creating the typeface. Also, on android if we can't find the font for a
character we return the last resort font, whereas on linux the behaviour is
different.

I've added a comment explaining when m_fontManager is expected to be true and
why we see a different behaviour here in this case. Could you please take
another look. 

> 
> Also, this code bypasses the synthetic bold and italics handling. Is that
> intentional?

[Khushal, 2016-02-22 20:58:53]

khushalsagar@chromium.org changed reviewers:
+ reed@chromium.org, rickyz@chromium.org

[Khushal, 2016-02-22 21:01:59]

khushalsagar@chromium.org changed reviewers:
+ sievers@chromium.org

[Khushal, 2016-02-22 21:01:59]

+rickyz for content/zygote,
+seivers for content/common and content/browser
+reed2 for skia/

[Khushal, 2016-02-22 21:02:51]

khushalsagar@chromium.org changed reviewers:
+ reed@google.com
- reed@chromium.org

[no sievers, 2016-02-22 22:27:20]

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
File content/zygote/zygote_main_linux.cc (right):

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:49: #include
"third_party/WebKit/public/web/linux/WebFontRendering.h"
Although there is no DEPS file, it feels a bit odd to have renderer-specific is
in here.
+jln@

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:71: #include
"third_party/skia/include/ports/SkFontMgr_android.h"
Is 'port' meant to be public?

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:352: new
FontConfigIPC(GetSandboxFD()))->unref();
Isn't this the way to get around the sandbox on Linux?

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:374:
blink::WebFontRendering::setSkiaFontManager(SkFontMgr_New_Android(&custom));
Isn't this an implementation detail that it only uses the directory during
construction?

[no sievers, 2016-02-22 22:27:30]

sievers@chromium.org changed reviewers:
+ jln@chromium.org

[eae, 2016-02-22 22:56:47]

LGTM

[Khushal, 2016-02-22 23:46:00]

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
File content/zygote/zygote_main_linux.cc (right):

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:71: #include
"third_party/skia/include/ports/SkFontMgr_android.h"
On 2016/02/22 22:27:20, sievers wrote:
> Is 'port' meant to be public?

I think so. We do use the SkFontConfigInterface port here already.

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:352: new
FontConfigIPC(GetSandboxFD()))->unref();
On 2016/02/22 22:27:20, sievers wrote:
> Isn't this the way to get around the sandbox on Linux?

We actually needed to use the android specific implementation in this case which
currently assumes access to the file system for font files.
A similar approach to manage the sandbox is used with RAND_set_urandom_fd too.

https://codereview.chromium.org/1685053002/diff/100001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:374:
blink::WebFontRendering::setSkiaFontManager(SkFontMgr_New_Android(&custom));
On 2016/02/22 22:27:20, sievers wrote:
> Isn't this an implementation detail that it only uses the directory during
> construction?

We added a mode to the public API to make sure that it uses the directory only
during construction for this case. The custom flags expose this functionality as
a part of the public API now.

[Khushal, 2016-02-22 23:47:58]

khushalsagar@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Khushal, 2016-02-22 23:47:59]

+pfeldman@chromium.org for WebKit/public.

[pfeldman, 2016-02-23 23:22:09]

lgtm

[nyquist, 2016-02-25 01:13:09]

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

[nyquist, 2016-02-25 01:13:10]

https://codereview.chromium.org/1685053002/diff/100001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1685053002/diff/100001/content/public/common/...
content/public/common/content_switches.cc:43: const char kAndroidFontsLocation[]
         = "android-fonts-location";
Question from a CL depending on this one: Other places where we specify paths,
we use -path as a suffix.
$ git grep '\-location\"' -- *switches.cc | wc -l
0
vs
$ git grep '\-path\"' -- *switches.cc | wc -l
10
So there is precedence to change this to android-fonts-path it seems.

[Khushal, 2016-02-25 02:17:47]

https://codereview.chromium.org/1685053002/diff/100001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1685053002/diff/100001/content/public/common/...
content/public/common/content_switches.cc:43: const char kAndroidFontsLocation[]
         = "android-fonts-location";
On 2016/02/25 01:13:10, nyquist wrote:
> Question from a CL depending on this one: Other places where we specify paths,
> we use -path as a suffix.
> $ git grep '\-location\"' -- *switches.cc | wc -l
> 0
> vs
> $ git grep '\-path\"' -- *switches.cc | wc -l
> 10
> So there is precedence to change this to android-fonts-path it seems.

Oh, I wasn't sure about what name would be better. Changed it to path. Thanks,
it makes it much clearer.

[rickyz (no longer on Chrome), 2016-02-25 23:56:56]

Zygote changes lgtm, but would be good to see if jln@ has any additional
comments.

https://codereview.chromium.org/1685053002/diff/120001/content/zygote/zygote_...
File content/zygote/zygote_main_linux.cc (right):

https://codereview.chromium.org/1685053002/diff/120001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:71: #include
"third_party/skia/include/ports/SkFontMgr_android.h"
Move this up with the other skia include

https://codereview.chromium.org/1685053002/diff/120001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:358: switches::kAndroidFontsPath)) {
nit: Indent extra 4 spaces here (unfortunately, the rest of this file doesn't
look clang-format clean either, or I'd suggest just running that :-/)

https://codereview.chromium.org/1685053002/diff/120001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:366: SkFontMgr_Android_CustomFonts custom =
{
Can you initialize each member individually, like

custom.fSystemFontUse =
SkFontMgr_Android_CustomFonts::SystemFontUse::kOnlyCustom;

I think it's a little easier to read that way.

[Khushal, 2016-02-26 19:21:36]

Done. Thanks ricky!

https://codereview.chromium.org/1685053002/diff/120001/content/zygote/zygote_...
File content/zygote/zygote_main_linux.cc (right):

https://codereview.chromium.org/1685053002/diff/120001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:71: #include
"third_party/skia/include/ports/SkFontMgr_android.h"
On 2016/02/25 23:56:56, rickyz wrote:
> Move this up with the other skia include

Done.

https://codereview.chromium.org/1685053002/diff/120001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:358: switches::kAndroidFontsPath)) {
On 2016/02/25 23:56:56, rickyz wrote:
> nit: Indent extra 4 spaces here (unfortunately, the rest of this file doesn't
> look clang-format clean either, or I'd suggest just running that :-/)

Ran git cl format on this, looks like it was just me. Done.

https://codereview.chromium.org/1685053002/diff/120001/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:366: SkFontMgr_Android_CustomFonts custom =
{
On 2016/02/25 23:56:56, rickyz wrote:
> Can you initialize each member individually, like
> 
> custom.fSystemFontUse =
> SkFontMgr_Android_CustomFonts::SystemFontUse::kOnlyCustom;
> 
> I think it's a little easier to read that way.

Done.

[Khushal, 2016-02-26 22:34:56]

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

[commit-bot: I haz the power, 2016-02-26 22:36:11]

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

[commit-bot: I haz the power, 2016-02-26 23:29:15]

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

[commit-bot: I haz the power, 2016-02-26 23:29:17]

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

[Khushal, 2016-02-29 21:58:16]

friendly ping for skia/ review. :)

[bungeman-chromium, 2016-02-29 22:48:50]

https://codereview.chromium.org/1685053002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/1685053002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:65:
m_fontManager(nullptr)
nit: indentation here is odd

https://codereview.chromium.org/1685053002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/1685053002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:281: return
adoptRef(m_fontManager->legacyCreateTypeface(name.data(), style));
This is a little bit sneaky, but I think this should instead be

m_fontManager->matchFamilyStyle(name.data(), fontStyle(fontDescription))

It appears that legacyCreateTypeface was chosen here to mirror what
CreateFromName below would do. However, there is no real need to be limited that
way. By using matchFamilyStyle you should get better weight selection (and
probably better fallback).

The CreateFromName call below eventually should be a call to matchFamilyStyle on
the default font manager (and I think this is now possible, I'll have to see
what changes).

[Khushal, 2016-03-01 18:52:44]

Done. Thanks Ben!

https://codereview.chromium.org/1685053002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/1685053002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:65:
m_fontManager(nullptr)
On 2016/02/29 22:48:50, bungeman2 wrote:
> nit: indentation here is odd

Done.

https://codereview.chromium.org/1685053002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/1685053002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:281: return
adoptRef(m_fontManager->legacyCreateTypeface(name.data(), style));
On 2016/02/29 22:48:50, bungeman2 wrote:
> This is a little bit sneaky, but I think this should instead be
> 
> m_fontManager->matchFamilyStyle(name.data(), fontStyle(fontDescription))
> 
> It appears that legacyCreateTypeface was chosen here to mirror what
> CreateFromName below would do. However, there is no real need to be limited
that
> way. By using matchFamilyStyle you should get better weight selection (and
> probably better fallback).

You're right, I was trying to be consistent with the other uses of font manager
but I guess its not necessary. The legacyCreateTypeface for the
SkFontMgr_android currently routes the call to matchFamilyStyle itself so there
is no need to have this indirection. The SkFontHost_fontconfig implementation
though returns null for matchFamilyStyle and we have to use
legacyCreateTypeface.

> 
> The CreateFromName call below eventually should be a call to matchFamilyStyle
on
> the default font manager (and I think this is now possible, I'll have to see
> what changes).

[bungeman-chromium, 2016-03-01 20:09:48]

lgtm

[bungeman-skia, 2016-03-01 20:10:21]

On 2016/03/01 20:09:48, bungeman2 wrote:
> lgtm

OWNER lgtm too

[jln (very slow on Chromium), 2016-03-01 22:11:32]

jln@chromium.org changed reviewers:
+ mdempsky@chromium.org

[jln (very slow on Chromium), 2016-03-01 22:12:52]

I won't be able to go through this, so I'm fine with it given rickyz@'s review.
Adding mdempsky@ in case he wants to take a quick second look.

[David Trainor- moved to gerrit, 2016-03-04 20:46:25]

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

[commit-bot: I haz the power, 2016-03-04 20:46:55]

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

[commit-bot: I haz the power, 2016-03-04 20:51:36]

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

[commit-bot: I haz the power, 2016-03-04 20:51:38]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Khushal, 2016-03-05 01:49:12]

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

[commit-bot: I haz the power, 2016-03-05 01:49:37]

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

[commit-bot: I haz the power, 2016-03-05 03:09:31]

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

[commit-bot: I haz the power, 2016-03-05 03:09:33]

Dry run: This issue passed the CQ dry run.

[no sievers, 2016-03-07 18:44:35]

lgtm

[Khushal, 2016-03-07 18:45:14]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-03-07 18:45:15]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
pfeldman@chromium.org, rickyz@chromium.org, bungeman@chromium.org,
bungeman@google.com
Link to the patchset: https://codereview.chromium.org/1685053002/#ps220001
(title: "Fix rebase.")

[commit-bot: I haz the power, 2016-03-07 18:46:08]

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

[commit-bot: I haz the power, 2016-03-07 18:50:22]

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

[commit-bot: I haz the power, 2016-03-07 18:50:24]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Khushal, 2016-03-07 19:03:06]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-03-07 19:03:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
bungeman@chromium.org, eae@chromium.org, bungeman@google.com,
rickyz@chromium.org, sievers@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1685053002/#ps240001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-07 19:03:35]

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

[commit-bot: I haz the power, 2016-03-07 20:35:35]

Description was changed from

==========
blink fonts: Load Android SkFontMgr on Linux.

The renderer running on the engine for blimp uses the linux font config,
while it needs to support the use of android fonts when connected to an
android client. The patch includes 2 changes necessary for this.

1) The default mode for the android SkFontMgr accesses files on disk
which is not possible in the renderer sandbox. So we create the font
manager in the zygote process, before initializing the sanbox. The font
manager uses the isolated mode, so it can acquire all necessary resources
on initialization and does not perform any disk I/O after it.

2) Add an API to blink::WebFontRendering on linux to override the default
font manager. This allows us to override the default behaviour in
FontCacheSkia for linux to direct all queries for fallback fonts to the
embedder provided font manager.

BUG=585668
==========

to

==========
blink fonts: Load Android SkFontMgr on Linux.

The renderer running on the engine for blimp uses the linux font config,
while it needs to support the use of android fonts when connected to an
android client. The patch includes 2 changes necessary for this.

1) The default mode for the android SkFontMgr accesses files on disk
which is not possible in the renderer sandbox. So we create the font
manager in the zygote process, before initializing the sanbox. The font
manager uses the isolated mode, so it can acquire all necessary resources
on initialization and does not perform any disk I/O after it.

2) Add an API to blink::WebFontRendering on linux to override the default
font manager. This allows us to override the default behaviour in
FontCacheSkia for linux to direct all queries for fallback fonts to the
embedder provided font manager.

BUG=585668
==========

[commit-bot: I haz the power, 2016-03-07 20:35:37]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-03-07 20:37:10]

Description was changed from

==========
blink fonts: Load Android SkFontMgr on Linux.

The renderer running on the engine for blimp uses the linux font config,
while it needs to support the use of android fonts when connected to an
android client. The patch includes 2 changes necessary for this.

1) The default mode for the android SkFontMgr accesses files on disk
which is not possible in the renderer sandbox. So we create the font
manager in the zygote process, before initializing the sanbox. The font
manager uses the isolated mode, so it can acquire all necessary resources
on initialization and does not perform any disk I/O after it.

2) Add an API to blink::WebFontRendering on linux to override the default
font manager. This allows us to override the default behaviour in
FontCacheSkia for linux to direct all queries for fallback fonts to the
embedder provided font manager.

BUG=585668
==========

to

==========
blink fonts: Load Android SkFontMgr on Linux.

The renderer running on the engine for blimp uses the linux font config,
while it needs to support the use of android fonts when connected to an
android client. The patch includes 2 changes necessary for this.

1) The default mode for the android SkFontMgr accesses files on disk
which is not possible in the renderer sandbox. So we create the font
manager in the zygote process, before initializing the sanbox. The font
manager uses the isolated mode, so it can acquire all necessary resources
on initialization and does not perform any disk I/O after it.

2) Add an API to blink::WebFontRendering on linux to override the default
font manager. This allows us to override the default behaviour in
FontCacheSkia for linux to direct all queries for fallback fonts to the
embedder provided font manager.

BUG=585668

Committed: https://crrev.com/12a558c16f853751ab702c84fbd0ffd7de565cdc
Cr-Commit-Position: refs/heads/master@{#379631}
==========

[commit-bot: I haz the power, 2016-03-07 20:37:13]

Patchset 13 (id:??) landed as
https://crrev.com/12a558c16f853751ab702c84fbd0ffd7de565cdc
Cr-Commit-Position: refs/heads/master@{#379631}