Remove use of legacy SkFontMgr factories.

Remove use of legacy SkFontMgr factories.

This moves SkFontMgr creation and use to owning pointers.

BUG=skia:5077
Review-Url: https://codereview.chromium.org/2618443002
Cr-Commit-Position: refs/heads/master@{#442926}
Committed: https://chromium.googlesource.com/chromium/src/+/ba3e738c8508e560dd92abc7f3b9231b4033d4be

[bungeman-chromium, 2017-01-04 16:12:09]

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

[commit-bot: I haz the power, 2017-01-04 16:12:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 16:26:19]

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

[commit-bot: I haz the power, 2017-01-04 16:26:20]

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

[bungeman-chromium, 2017-01-04 17:43:44]

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

[commit-bot: I haz the power, 2017-01-04 17:43:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 18:30:41]

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

[commit-bot: I haz the power, 2017-01-04 18:30:42]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bungeman-chromium, 2017-01-06 20:46:00]

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

[commit-bot: I haz the power, 2017-01-06 20:46:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 21:33:56]

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

[commit-bot: I haz the power, 2017-01-06 21:33:57]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[bungeman-chromium, 2017-01-06 21:41:12]

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

[bungeman-chromium, 2017-01-06 21:41:13]

This is generally the kind of thing I want to do. It may need some more cleanup,
but please point out if there is anything especially egregious.

[f(malita), 2017-01-09 15:25:34]

lgtm

https://codereview.chromium.org/2618443002/diff/80001/content/child/font_warm...
File content/child/font_warmup_win.cc (right):

https://codereview.chromium.org/2618443002/diff/80001/content/child/font_warm...
content/child/font_warmup_win.cc:418: if (!g_warmup_fontmgr)
What would it take to convert g_warmup_fontmgr to sk_sp<SkFontMgr>?

Are sk_sp<> globals disallowed?

https://codereview.chromium.org/2618443002/diff/80001/content/child/font_warm...
File content/child/font_warmup_win_unittest.cc (right):

https://codereview.chromium.org/2618443002/diff/80001/content/child/font_warm...
content/child/font_warmup_win_unittest.cc:138:
content::SetPreSandboxWarmupFontMgrForTesting(sk_sp<SkFontMgr>());
nit: I think nullptr reads better here.

https://codereview.chromium.org/2618443002/diff/80001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2618443002/diff/80001/content/test/layouttest...
content/test/layouttest_support.cc:156: void
RegisterSideloadedTypefaces(sk_sp<SkFontMgr> fontmgr) {
looks like we're not passing a ref here - can this be const sk_sp<SkFontMgr>&?

https://codereview.chromium.org/2618443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp (right):

https://codereview.chromium.org/2618443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp:38: if
(s_staticFontManager) {
I don't think we need to conditional anymore, just

  m_fontManager = sk_ref_sp(s_staticFontManager);

(or move to initializer list)

[bungeman-chromium, 2017-01-09 16:51:11]

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

[commit-bot: I haz the power, 2017-01-09 16:51:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bungeman-chromium, 2017-01-09 19:15:28]

https://codereview.chromium.org/2618443002/diff/80001/content/child/font_warm...
File content/child/font_warmup_win.cc (right):

https://codereview.chromium.org/2618443002/diff/80001/content/child/font_warm...
content/child/font_warmup_win.cc:418: if (!g_warmup_fontmgr)
On 2017/01/09 15:25:34, f(malita) wrote:
> What would it take to convert g_warmup_fontmgr to sk_sp<SkFontMgr>?
> 
> Are sk_sp<> globals disallowed?

We've tried this before and had to revert. See
https://bugs.chromium.org/p/chromium/issues/detail?id=665274 . sk_sp<> globals
can't be used because it means registering an exit time destructor which means
an implicit initializer needs to be run in order to register the exit time
destructor. The sizes bot catches this. Skia isn't going to create a sk_sp_leaky
since it doesn't need it. (I believe there is exactly on place that could use
it, and I consider that use legacy.)

https://codereview.chromium.org/2618443002/diff/80001/content/child/font_warm...
File content/child/font_warmup_win_unittest.cc (right):

https://codereview.chromium.org/2618443002/diff/80001/content/child/font_warm...
content/child/font_warmup_win_unittest.cc:138:
content::SetPreSandboxWarmupFontMgrForTesting(sk_sp<SkFontMgr>());
On 2017/01/09 15:25:34, f(malita) wrote:
> nit: I think nullptr reads better here.

Done.

https://codereview.chromium.org/2618443002/diff/80001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2618443002/diff/80001/content/test/layouttest...
content/test/layouttest_support.cc:156: void
RegisterSideloadedTypefaces(sk_sp<SkFontMgr> fontmgr) {
On 2017/01/09 15:25:34, f(malita) wrote:
> looks like we're not passing a ref here - can this be const sk_sp<SkFontMgr>&?

I did it this way because previously the caller was leaking because this wasn't
taking ownership. Now that the the one caller of this has a smart pointer in
hand, maybe it should be up to them to handle that and let this just borrow that
ownership. In which case this should just get a non-owning raw pointer.

https://codereview.chromium.org/2618443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp (right):

https://codereview.chromium.org/2618443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp:38: if
(s_staticFontManager) {
On 2017/01/09 15:25:34, f(malita) wrote:
> I don't think we need to conditional anymore, just
> 
>   m_fontManager = sk_ref_sp(s_staticFontManager);
> 
> (or move to initializer list)

Done.

[commit-bot: I haz the power, 2017-01-09 19:50:03]

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

[commit-bot: I haz the power, 2017-01-09 19:50:03]

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

[bungeman-chromium, 2017-01-09 20:03:03]

bungeman@chromium.org changed reviewers:
+ asvitkine@chromium.org, jochen@chromium.org, nyquist@chromium.org

[Alexei Svitkine (slow), 2017-01-09 20:15:28]

ui/gfx lgtm

[jochen (gone - plz use gerrit), 2017-01-10 09:53:04]

which part do you want me to review?

can you please reference a tracking bug from the CL description?

https://codereview.chromium.org/2618443002/diff/100001/content/child/font_war...
File content/child/font_warmup_win.h (right):

https://codereview.chromium.org/2618443002/diff/100001/content/child/font_war...
content/child/font_warmup_win.h:13: #include
"third_party/skia/include/core/SkRefCnt.h"
you should be able to forward declare template<typename T> class sk_sp; no?

[bungeman-chromium, 2017-01-10 15:29:10]

Description was changed from

==========
Remove use of legacy SkFontMgr factories.

This moves SkFontMgr creation and use to owning pointers.
==========

to

==========
Remove use of legacy SkFontMgr factories.

This moves SkFontMgr creation and use to owning pointers.

BUG=skia:5077
==========

[bungeman-chromium, 2017-01-10 18:31:15]

Added CL to description.

jochen: I forgot how much you own. I added you for content/ and
third_party/WebKit/public .

https://codereview.chromium.org/2618443002/diff/100001/content/child/font_war...
File content/child/font_warmup_win.h (right):

https://codereview.chromium.org/2618443002/diff/100001/content/child/font_war...
content/child/font_warmup_win.h:13: #include
"third_party/skia/include/core/SkRefCnt.h"
On 2017/01/10 09:53:04, jochen wrote:
> you should be able to forward declare template<typename T> class sk_sp; no?

I had thought such things were frowned upon, but now I can find nothing to
support that. I'll forward declare where possible.

Done.

https://codereview.chromium.org/2618443002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/win/WebFontRendering.h (right):

https://codereview.chromium.org/2618443002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/win/WebFontRendering.h:9: #include
"third_party/skia/include/core/SkRefCnt.h"
Unfortunately, this can't really be forward declared at this time. The required
formatting is terrible, and the 'check-webkit-style' step will fail with an
error about 'sk_sp is incorrectly named.' The style checker should probably only
give such errors on definitions instead of declarations to be more correct.

[nyquist, 2017-01-10 22:44:09]

blimp lgtm

[jochen (gone - plz use gerrit), 2017-01-11 09:51:13]

lgtm

[bungeman-chromium, 2017-01-11 14:21:49]

The CQ bit was checked by bungeman@chromium.org

[bungeman-chromium, 2017-01-11 14:21:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2618443002/#ps120001
(title: "Forward declare when possible.")

[commit-bot: I haz the power, 2017-01-11 14:22:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 16:47:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484144509060410,
"parent_rev": "c896d034be2fa2ed5a22fbb31910f616ae85cb4f", "commit_rev":
"ba3e738c8508e560dd92abc7f3b9231b4033d4be"}

[commit-bot: I haz the power, 2017-01-11 16:49:04]

Description was changed from

==========
Remove use of legacy SkFontMgr factories.

This moves SkFontMgr creation and use to owning pointers.

BUG=skia:5077
==========

to

==========
Remove use of legacy SkFontMgr factories.

This moves SkFontMgr creation and use to owning pointers.

BUG=skia:5077

Review-Url: https://codereview.chromium.org/2618443002
Cr-Commit-Position: refs/heads/master@{#442926}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba3e738c8508e560dd92abc7f3b9...
==========

[commit-bot: I haz the power, 2017-01-11 16:49:06]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ba3e738c8508e560dd92abc7f3b9...