Avoid loading system fonts in LayoutThemeMac::systemFont

Avoid loading system fonts in LayoutThemeMac::systemFont

Loading system fonts is buggy on macOS 10.12 Sierra. If we just assume
that system fonts on Mac are never italic or bold, they don't need to be
loaded at all. So, do that.

This reduces the memory footprint of each renderer process by ~600MB on
macOS 10.12 Beta (16A201w).

BUG=619981

[tapted, 2016-06-16 05:00:26]

Description was changed from

==========
Avoid loading system fonts in LayoutThemeMac::systemFont

Assumes system fonts on Mac are never italic or bold.

BUG=619981
==========

to

==========
Avoid loading system fonts in LayoutThemeMac::systemFont

Loading system fonts is buggy on macOS 10.12 Sierra. If we just assume
that system fonts on Mac are never italic or bold, they don't need to be
loaded at all. So, do that.

BUG=619981
==========

[tapted, 2016-06-16 06:37:16]

Description was changed from

==========
Avoid loading system fonts in LayoutThemeMac::systemFont

Loading system fonts is buggy on macOS 10.12 Sierra. If we just assume
that system fonts on Mac are never italic or bold, they don't need to be
loaded at all. So, do that.

BUG=619981
==========

to

==========
Avoid loading system fonts in LayoutThemeMac::systemFont

Loading system fonts is buggy on macOS 10.12 Sierra. If we just assume
that system fonts on Mac are never italic or bold, they don't need to be
loaded at all. So, do that.

This reduces the memory footprint of each renderer process by ~600MB on
macOS 10.12 Beta (16A201w).

BUG=619981
==========

[tapted, 2016-06-16 06:37:16]

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

[tapted, 2016-06-16 06:49:09]

Hi tkent, could you please take a look. Or do you know a good reviewer for this?

(+cc Nico, but he's travelling, and this might be something we'd want to merge
to m52)

I've verified with assert that, for each of these (on OSX 10.11)

[NSFont smallSystemFontSize] == [[NSFont systemFontOfSize:[NSFont
smallSystemFontSize]] pointSize]

System fonts are never italic on Mac, and if they get any altered font weight,
it's very slight (never "bold"). So, even if Apple decide it's an OS bug and it
gets fixed, there doesn't really seem to be any benefit to calling into system
libraries to create these fonts. Justs wastes computation and potentially a
whole bunch of memory.

But, I don't really know anything about how blink actually uses the information
from these methods...

[Nico, 2016-06-16 08:22:04]

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

[Nico, 2016-06-16 08:22:04]

I don't know if we have anyone really familiar with this code :-/ erikchen did
some font work a while ago, but I think not here (and he's away until Monday
anyhow).

Did you check what WebKit does here nowadays?

[tapted, 2016-06-16 09:09:52]

On 2016/06/16 08:22:04, Nico (traveling...slow) wrote:
> I don't know if we have anyone really familiar with this code :-/ erikchen did
> some font work a while ago, but I think not here (and he's away until Monday
> anyhow).
> 
> Did you check what WebKit does here nowadays?

So.. the code still seems to be around, but there is
https://bugs.webkit.org/show_bug.cgi?id=157464 "[OS X] Migrate our Font classes
entirely off of NSFont" which was closed a bit over a month ago. CL:
http://trac.webkit.org/changeset/200563 . Doesn't change these "theme" classes
though.


E.g. There's something very similar to setFontFromControlSize still in
ThemeMac.mm -
https://github.com/WebKit/webkit/blob/16b72e93c53118d57739b315e2112a049fa717b...

and.. the rest of the code still exists in RenderThemeMac.mm -
https://github.com/WebKit/webkit/blob/16b72e93c53118d57739b315e2112a049fa717b...

There might be better caching of the fonts in
https://github.com/WebKit/webkit/blob/2e1bfeaa097d035971fd1a419cf1b6a75565c7d...

However, the callsite in chrome at
blink::CSSPropertyParser::consumeSystemFont(bool) doesn't have any caching -
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parse...
but it looks like blink only calls this method once, so caching is unlilkely to
help.


It could be just that Safari rarely asks for a system font, but blink seems to
ask for one via the user agent style sheet.

Another thing might be some kind of sandbox exception that Safari has, but I
couldn't find anything about that in the WebKit code/changes.

[tapted, 2016-06-17 02:50:17]

On 2016/06/16 09:09:52, tapted wrote:
> On 2016/06/16 08:22:04, Nico (traveling...slow) wrote:
> > I don't know if we have anyone really familiar with this code :-/ erikchen
did
> > some font work a while ago, but I think not here (and he's away until Monday
> > anyhow).
> > 
> > Did you check what WebKit does here nowadays?
..
> Another thing might be some kind of sandbox exception that Safari has, but I
> couldn't find anything about that in the WebKit code/changes.

So I finally found the dang sandbox profiles. Looks like WebKit fixed it in the
most informative https://bugs.webkit.org/show_bug.cgi?id=151509 "Add font
service permission to the sandbox profile". So http://wkrev.com/192805 is the
fix we need. Uploaded our version to https://codereview.chromium.org/2075953002
and I've verified it makes the 600MB allocation go away. (Apparently without it
we spawn an entire Font server in each renderer process).

Let me know if there's anything useful in this CL... Seems we can get some kind
of renderer startup benefit if we don't always have to talk to the font server
just to load an unused NSFont for the default stylesheet

[tkent, 2016-06-20 23:03:41]

> that system fonts on Mac are never italic or bold, they don't need to be

If this is true, I think this CL is worth to be landed.