Add logic to switch to DirectWrite font proxy via field trial.

Add logic to switch to DirectWrite font proxy via field trial.

BUG=525142
patch from issue 1432123002 at patchset 180001 (http://crrev.com/1432123002#ps180001)

Committed: https://crrev.com/e186ab5f13cb800b27dd6996c177e9ae4048ae2b
Cr-Commit-Position: refs/heads/master@{#368758}

[Ilya Kulshin, 2015-12-29 21:45:56]

kulshin@chromium.org changed reviewers:
+ ananta@chromium.org, fdoray@chromium.org, jam@chromium.org,
jschuh@chromium.org

[Ilya Kulshin, 2015-12-29 21:45:56]

ptal.

Francois convinced me to do the switch via field trial rather than all at once.
Deleting the cache will come later, once we've got some experimental data.

[fdoray, 2015-12-30 01:15:08]

https://codereview.chromium.org/1557513002/diff/1/content/common/font_warmup_...
File content/common/font_warmup_win.cc (right):

https://codereview.chromium.org/1557513002/diff/1/content/common/font_warmup_...
content/common/font_warmup_win.cc:491: if
(content::ShouldUseDirectWriteFontProxyFieldTrial()) {
// If the font cache is used, |g_warmup_fontmgr| is expected to be initialized
// by GetPreSandboxWarmupFontMgr before this function is called. If the font
// proxy is used, |g_warmup_fontmgr| is initialized when this function is
// called. This function is never called more than once per process.
if (content::ShouldUseDirectWriteFontProxyFieldTrial()) {
  DCHECK(!g_warmup_fontmgr);
  g_warmup_fontmgr = SkFontMgr_New_DirectWrite();
}
DCHECK(g_warmup_fontmgr);

Can you confirm that the comment I wrote is right? If PatchGdiFontEnumeration is
called multiple times, the previous value of |g_warmup_fontmgr| is leaked.

https://codereview.chromium.org/1557513002/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1557513002/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:738: if
(content::ShouldUseDirectWriteFontProxyFieldTrial()) {
if (!...)

https://codereview.chromium.org/1557513002/diff/1/content/ppapi_plugin/ppapi_...
File content/ppapi_plugin/ppapi_plugin_main.cc (right):

https://codereview.chromium.org/1557513002/diff/1/content/ppapi_plugin/ppapi_...
content/ppapi_plugin/ppapi_plugin_main.cc:71: // as a member function because
PluginGlobals is not ref counted.
You can use base::Bind with a non-refcounted class using base::Unretained.
See
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

https://codereview.chromium.org/1557513002/diff/1/content/public/common/dwrit...
File content/public/common/dwrite_font_platform_win.h (right):

https://codereview.chromium.org/1557513002/diff/1/content/public/common/dwrit...
content/public/common/dwrite_font_platform_win.h:39: // Determines whether code
should use the DirectWrite font proxy codepathh.
*codepath

[Ilya Kulshin, 2015-12-30 01:59:47]

https://codereview.chromium.org/1557513002/diff/1/content/common/font_warmup_...
File content/common/font_warmup_win.cc (right):

https://codereview.chromium.org/1557513002/diff/1/content/common/font_warmup_...
content/common/font_warmup_win.cc:491: if
(content::ShouldUseDirectWriteFontProxyFieldTrial()) {
On 2015/12/30 01:15:08, fdoray wrote:
> // If the font cache is used, |g_warmup_fontmgr| is expected to be initialized
> // by GetPreSandboxWarmupFontMgr before this function is called. If the font
> // proxy is used, |g_warmup_fontmgr| is initialized when this function is
> // called. This function is never called more than once per process.
> if (content::ShouldUseDirectWriteFontProxyFieldTrial()) {
>   DCHECK(!g_warmup_fontmgr);
>   g_warmup_fontmgr = SkFontMgr_New_DirectWrite();
> }
> DCHECK(g_warmup_fontmgr);
> 
> Can you confirm that the comment I wrote is right? If PatchGdiFontEnumeration
is
> called multiple times, the previous value of |g_warmup_fontmgr| is leaked.

Good point about the leak. From tracing the code it looks like it's actually
called at least once per plugin, so I'll add a check to only initialize it if
it's not initialized already.

https://codereview.chromium.org/1557513002/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1557513002/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:738: if
(content::ShouldUseDirectWriteFontProxyFieldTrial()) {
On 2015/12/30 01:15:08, fdoray wrote:
> if (!...)

Done.

https://codereview.chromium.org/1557513002/diff/1/content/ppapi_plugin/ppapi_...
File content/ppapi_plugin/ppapi_plugin_main.cc (right):

https://codereview.chromium.org/1557513002/diff/1/content/ppapi_plugin/ppapi_...
content/ppapi_plugin/ppapi_plugin_main.cc:71: // as a member function because
PluginGlobals is not ref counted.
On 2015/12/30 01:15:08, fdoray wrote:
> You can use base::Bind with a non-refcounted class using base::Unretained.
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

Done.

https://codereview.chromium.org/1557513002/diff/1/content/public/common/dwrit...
File content/public/common/dwrite_font_platform_win.h (right):

https://codereview.chromium.org/1557513002/diff/1/content/public/common/dwrit...
content/public/common/dwrite_font_platform_win.h:39: // Determines whether code
should use the DirectWrite font proxy codepathh.
On 2015/12/30 01:15:08, fdoray wrote:
> *codepath

Done.

[fdoray, 2015-12-31 03:18:16]

lgtm with 2 comments

https://codereview.chromium.org/1557513002/diff/20001/content/common/font_war...
File content/common/font_warmup_win.cc (right):

https://codereview.chromium.org/1557513002/diff/20001/content/common/font_war...
content/common/font_warmup_win.cc:492: g_warmup_fontmgr =
SkFontMgr_New_DirectWrite();
Is this object leaked during shutdown? If we want to leak the object, can we say
it explicitly in a comment? Otherwise, should we use a
base::LazyInstance<skia::RefPtr<SkFontMgr>>?

https://codereview.chromium.org/1557513002/diff/20001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_plugin_main.cc (right):

https://codereview.chromium.org/1557513002/diff/20001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_plugin_main.cc:69: 
no new line

[jam, 2016-01-04 15:43:28]

which parts do you want me to review?

[Ilya Kulshin, 2016-01-07 18:51:15]

Patchset #3 (id:40001) has been deleted

[Ilya Kulshin, 2016-01-07 20:40:36]

Patchset #3 (id:60001) has been deleted

[Ilya Kulshin, 2016-01-08 00:44:23]

jam@: fdoray@ reviewed this change, and ananta@ reviewed most of it as well in
https://codereview.chromium.org/1432123002/, so I just need owners approval
(you're welcome to review it in detail as well, of course)

jschuh@: please review content/common/sandbox_win.cc

Apologies for the multitude of reviews, it seemed it would be more
straightforward to start a new review than try to roll back parts of the
previous review.

https://codereview.chromium.org/1557513002/diff/20001/content/common/font_war...
File content/common/font_warmup_win.cc (right):

https://codereview.chromium.org/1557513002/diff/20001/content/common/font_war...
content/common/font_warmup_win.cc:492: g_warmup_fontmgr =
SkFontMgr_New_DirectWrite();
On 2015/12/31 03:18:16, fdoray wrote:
> Is this object leaked during shutdown? If we want to leak the object, can we
say
> it explicitly in a comment? Otherwise, should we use a
> base::LazyInstance<skia::RefPtr<SkFontMgr>>?

The previous code was leaking it, I guess I hadn't thought of it as a concern
(it's just a single object that otherwise lives for the duration of the process
anyway). LazyInstance<RefPtr> runs into problems with lifetime and destruction,
so I added a comment.

https://codereview.chromium.org/1557513002/diff/20001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_plugin_main.cc (right):

https://codereview.chromium.org/1557513002/diff/20001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_plugin_main.cc:69: 
On 2015/12/31 03:18:16, fdoray wrote:
> no new line

Done.

[jam, 2016-01-11 05:05:46]

lgtm

https://codereview.chromium.org/1557513002/diff/100001/content/common/font_wa...
File content/common/font_warmup_win.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/common/font_wa...
content/common/font_warmup_win.cc:492: if
(content::ShouldUseDirectWriteFontProxyFieldTrial() && !g_warmup_fontmgr)
nit: no content::

https://codereview.chromium.org/1557513002/diff/100001/content/common/sandbox...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/common/sandbox...
content/common/sandbox_win.cc:741: if
(!content::ShouldUseDirectWriteFontProxyFieldTrial()) {
ditto

https://codereview.chromium.org/1557513002/diff/100001/content/ppapi_plugin/p...
File content/ppapi_plugin/ppapi_plugin_main.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/ppapi_plugin/p...
content/ppapi_plugin/ppapi_plugin_main.cc:145: if
(content::ShouldUseDirectWriteFontProxyFieldTrial()) {
ditto and also below

https://codereview.chromium.org/1557513002/diff/100001/content/renderer/rende...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/renderer/rende...
content/renderer/renderer_main_platform_delegate_win.cc:78: if
(content::ShouldUseDirectWriteFontProxyFieldTrial())
ditto and below

https://codereview.chromium.org/1557513002/diff/100001/content/test/layouttes...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/test/layouttes...
content/test/layouttest_support.cc:109: if
(!content::ShouldUseDirectWriteFontProxyFieldTrial())
also this file

[Ilya Kulshin, 2016-01-11 20:56:52]

https://codereview.chromium.org/1557513002/diff/100001/content/common/font_wa...
File content/common/font_warmup_win.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/common/font_wa...
content/common/font_warmup_win.cc:492: if
(content::ShouldUseDirectWriteFontProxyFieldTrial() && !g_warmup_fontmgr)
On 2016/01/11 05:05:45, jam wrote:
> nit: no content::

Done.

https://codereview.chromium.org/1557513002/diff/100001/content/common/sandbox...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/common/sandbox...
content/common/sandbox_win.cc:741: if
(!content::ShouldUseDirectWriteFontProxyFieldTrial()) {
On 2016/01/11 05:05:45, jam wrote:
> ditto

Done.

https://codereview.chromium.org/1557513002/diff/100001/content/ppapi_plugin/p...
File content/ppapi_plugin/ppapi_plugin_main.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/ppapi_plugin/p...
content/ppapi_plugin/ppapi_plugin_main.cc:145: if
(content::ShouldUseDirectWriteFontProxyFieldTrial()) {
On 2016/01/11 05:05:45, jam wrote:
> ditto and also below

Done.

https://codereview.chromium.org/1557513002/diff/100001/content/renderer/rende...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/renderer/rende...
content/renderer/renderer_main_platform_delegate_win.cc:78: if
(content::ShouldUseDirectWriteFontProxyFieldTrial())
On 2016/01/11 05:05:45, jam wrote:
> ditto and below

Done.

https://codereview.chromium.org/1557513002/diff/100001/content/test/layouttes...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/1557513002/diff/100001/content/test/layouttes...
content/test/layouttest_support.cc:109: if
(!content::ShouldUseDirectWriteFontProxyFieldTrial())
On 2016/01/11 05:05:45, jam wrote:
> also this file

Done.

[Ilya Kulshin, 2016-01-11 21:53:24]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2016-01-11 21:53:25]

The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1557513002/#ps120001
(title: "Remove extraneous namespace qualification")

[commit-bot: I haz the power, 2016-01-11 21:53:42]

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

[commit-bot: I haz the power, 2016-01-11 22:05:19]

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

[commit-bot: I haz the power, 2016-01-11 22:05:20]

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

[jschuh, 2016-01-12 00:04:37]

lgtm for sandbox_win.cc

[Ilya Kulshin, 2016-01-12 00:20:54]

The CQ bit was checked by kulshin@chromium.org

[commit-bot: I haz the power, 2016-01-12 00:23:05]

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

[commit-bot: I haz the power, 2016-01-12 01:29:36]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-01-12 01:31:06]

Description was changed from

==========
Add logic to switch to DirectWrite font proxy via field trial.

BUG=525142

patch from issue 1432123002 at patchset 180001
(http://crrev.com/1432123002#ps180001)
==========

to

==========
Add logic to switch to DirectWrite font proxy via field trial.

BUG=525142

patch from issue 1432123002 at patchset 180001
(http://crrev.com/1432123002#ps180001)

Committed: https://crrev.com/e186ab5f13cb800b27dd6996c177e9ae4048ae2b
Cr-Commit-Position: refs/heads/master@{#368758}
==========

[commit-bot: I haz the power, 2016-01-12 01:31:07]

Patchset 5 (id:??) landed as
https://crrev.com/e186ab5f13cb800b27dd6996c177e9ae4048ae2b
Cr-Commit-Position: refs/heads/master@{#368758}