Switch to direct write font proxy and remove the font cache.

This review is closed. Please review https://codereview.chromium.org/1557513002/ instead.

Switch to direct write font proxy and remove the font cache.

BUG=525142

[Ilya Kulshin, 2015-11-11 02:19:50]

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

[Ilya Kulshin, 2015-11-11 02:19:51]

This is part 2 of the split of issue 1378353006.
Part 1 is issue 1438603002.

ptal:
jschuh@ - content/common/sandbox_win.cc
jam@ - content/, chrome/

[jam, 2015-11-11 23:03:55]

Justin: can you review the whole patch? then i can rubberstamp

[Ilya Kulshin, 2015-11-12 23:39:35]

kulshin@chromium.org changed reviewers:
+ ananta@chromium.org

[Ilya Kulshin, 2015-11-12 23:39:35]

+ananta, for direct write stuff.

[Peter Kasting, 2015-11-12 23:41:10]

Description was changed from

==========
Switch to direct write font proxy and remove the font cache.

BUG=
==========

to

==========
Switch to direct write font proxy and remove the font cache.

BUG=525142
==========

[Ilya Kulshin, 2015-12-02 22:57:14]

Ping... With part 1 (https://codereview.chromium.org/1438603002/) hopefully
wrapping up soon, would be good to get some feedback on part 2.

[Ilya Kulshin, 2015-12-07 18:49:47]

ptal, just want to make sure this doesn't get buried. Would be great to get some
feedback (even if it's "everything looks good, but waiting for part 1 to land").

[ananta, 2015-12-09 23:14:28]

Looks good

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

https://codereview.chromium.org/1432123002/diff/80001/content/common/font_war...
content/common/font_warmup_win.cc:207: ppapi::ProxyAutoLock lock;
Why do we need this?

https://codereview.chromium.org/1432123002/diff/80001/content/common/font_war...
content/common/font_warmup_win.cc:382: void PatchServiceManagerCalls() {
We can remove this and all the IAT intercepts now that no one calls them?

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

https://codereview.chromium.org/1432123002/diff/80001/content/test/layouttest...
content/test/layouttest_support.cc:94: void
DoPreSandboxWarmupForTypeface(SkTypeface* typeface) {
Why do we need this now?

[Ilya Kulshin, 2015-12-15 00:05:57]

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

https://codereview.chromium.org/1432123002/diff/80001/content/common/font_war...
content/common/font_warmup_win.cc:207: ppapi::ProxyAutoLock lock;
On 2015/12/09 23:14:28, ananta wrote:
> Why do we need this?

This is called from the ppapi process, which needs to take a lock on the proxy
so it can do the IPC to the browser.

https://codereview.chromium.org/1432123002/diff/80001/content/common/font_war...
content/common/font_warmup_win.cc:382: void PatchServiceManagerCalls() {
On 2015/12/09 23:14:28, ananta wrote:
> We can remove this and all the IAT intercepts now that no one calls them?

Unfortunately not quite. The new code still needs to do the patch. We may be
able to get rid of this once we switch skia to properly use custom collections
(instead of a fake system collection), but that needs more investigation.

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

https://codereview.chromium.org/1432123002/diff/80001/content/test/layouttest...
content/test/layouttest_support.cc:94: void
DoPreSandboxWarmupForTypeface(SkTypeface* typeface) {
On 2015/12/09 23:14:28, ananta wrote:
> Why do we need this now?

I thought it was needed at some point, but looks like it's not needed anymore.
Removed.

[ananta, 2015-12-16 00:51:05]

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

https://codereview.chromium.org/1432123002/diff/120001/content/common/font_wa...
content/common/font_warmup_win.cc:207: ppapi::ProxyAutoLock lock;
Is this function only invoked from ppapi?

https://codereview.chromium.org/1432123002/diff/120001/content/common/font_wa...
content/common/font_warmup_win.cc:381: // These are all IAT patched.
Where is this function called in your patch?

[Ilya Kulshin, 2015-12-16 01:07:08]

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

https://codereview.chromium.org/1432123002/diff/120001/content/common/font_wa...
content/common/font_warmup_win.cc:207: ppapi::ProxyAutoLock lock;
On 2015/12/16 00:51:05, ananta wrote:
> Is this function only invoked from ppapi?

Yes (and unittests). This is only used by the FakeGdiObjectFactory, which only
gets pulled in from PatchGdiFontEnumeration.

https://codereview.chromium.org/1432123002/diff/120001/content/common/font_wa...
content/common/font_warmup_win.cc:381: // These are all IAT patched.
On 2015/12/16 00:51:05, ananta wrote:
> Where is this function called in your patch?

Issue https://codereview.chromium.org/1438603002/ added a call in
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc.

[ananta, 2015-12-16 23:37:00]

lgtm

[Ilya Kulshin, 2015-12-17 21:38:02]

The CQ bit was checked by kulshin@chromium.org

[commit-bot: I haz the power, 2015-12-17 21:40:26]

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

[commit-bot: I haz the power, 2015-12-17 21:52:55]

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

[commit-bot: I haz the power, 2015-12-17 21:52:56]

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...)

[fdoray, 2015-12-23 13:45:02]

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

[fdoray, 2015-12-23 13:45:02]

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:210: // Attempt to delte the font
cache and ignore any errors.
*delete

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:211:
::DeleteFile(path.AsUTF16Unsafe().c_str());
base::DeleteFile? base/files/file_util.h

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:344: // TODO(kulshin): remove this
code after several releases.
I would have been curious to see the impact of this change on startup and page
load time. A field trial could have been used for that -> x% of users use the
old font cache and x% use the new technique. I'm happy to help if you decide to
do a field trial.

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:349: content::BrowserThread::IO,
FROM_HERE,
content::BrowserThread::FILE is the right thread for this. IO is for
"non-blocking IO, i.e. IPC and network". content/public/browser/browser_thread.h

[Ilya Kulshin, 2015-12-28 22:47:06]

Working on adding field trial logic.

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:210: // Attempt to delte the font
cache and ignore any errors.
On 2015/12/23 13:45:02, fdoray wrote:
> *delete

Done.

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:211:
::DeleteFile(path.AsUTF16Unsafe().c_str());
On 2015/12/23 13:45:02, fdoray wrote:
> base::DeleteFile? base/files/file_util.h

Done.

https://codereview.chromium.org/1432123002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:349: content::BrowserThread::IO,
FROM_HERE,
On 2015/12/23 13:45:02, fdoray wrote:
> content::BrowserThread::FILE is the right thread for this. IO is for
> "non-blocking IO, i.e. IPC and network".
content/public/browser/browser_thread.h

Done.

[jschuh, 2016-01-08 01:19:54]

lgtm on sandbox_win.cc parts

[Ilya Kulshin, 2016-01-08 01:57:03]

Description was changed from

==========
Switch to direct write font proxy and remove the font cache.

BUG=525142
==========

to

==========
This review is closed. Please review https://codereview.chromium.org/1557513002/
instead.

Switch to direct write font proxy and remove the font cache.

BUG=525142
==========