Implementation of dwrite font proxy and removal of dwrite font cache

Closing this review in favor of the two new reviews.


This change uses IPC to communicate with the system font collection in the browser process for font enumeration, bypassing the dwrite font cache.

dwrite_font_proxy_message_filter is the browser-side implementation that will handle the IPC messages. dwrite_font_proxy is the renderer-side. Most of the rest is removing of the existing font cache and putting everything together.

Feel free to ignore the first 13 patch sets and start at #14.

BUG=525142

[Ilya Kulshin, 2015-10-15 18:49:25]

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

[commit-bot: I haz the power, 2015-10-15 18:50:06]

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

[commit-bot: I haz the power, 2015-10-15 18:50:07]

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

[commit-bot: I haz the power, 2015-10-15 18:50:08]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Ilya Kulshin, 2015-11-02 18:33:14]

Description was changed from

==========
Prototype implementation of font proxy.
This change uses IPC to communicate with the system font collection in
the browser process for font enumeration, bypassing the dwrite font
cache.

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

to

==========
This change uses IPC to communicate with the system font collection in the
browser process for font enumeration, bypassing the dwrite font cache.

dwrite_font_proxy_message_filter is the browser-side implementation that will
handle the IPC messages. dwrite_font_proxy is the renderer-side. Most of the
rest is removing of the existing font cache and 

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

[Ilya Kulshin, 2015-11-02 18:33:14]

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

[Ilya Kulshin, 2015-11-02 18:38:44]

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

[Ilya Kulshin, 2015-11-02 18:40:14]

Description was changed from

==========
This change uses IPC to communicate with the system font collection in the
browser process for font enumeration, bypassing the dwrite font cache.

dwrite_font_proxy_message_filter is the browser-side implementation that will
handle the IPC messages. dwrite_font_proxy is the renderer-side. Most of the
rest is removing of the existing font cache and 

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

to

==========
This change uses IPC to communicate with the system font collection in the
browser process for font enumeration, bypassing the dwrite font cache.

dwrite_font_proxy_message_filter is the browser-side implementation that will
handle the IPC messages. dwrite_font_proxy is the renderer-side. Most of the
rest is removing of the existing font cache and putting everything together.

Feel free to ignore the first 13 patch sets and start at #14.

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

[Ilya Kulshin, 2015-11-02 18:40:15]

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

[Ilya Kulshin, 2015-11-02 20:24:38]

ptal, apologies for the large size.

[jam, 2015-11-03 00:18:55]

did overall scan. since you added multiple reviewers, please specify which files
you want them to review.

please familiarze yourself with the style guide in
https://www.chromium.org/developers/coding-style and
https://google.github.io/styleguide/cppguide.html, there are a bunch of places
that go against the naming conventions.

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:47:
PostTaskAndReplyFn postTaskAndReply)
i dont think you need this extra complexity with this function pointer for the
unittests. you can use TestBrowserThreadBundle and the FILE_THREAD will just be
the current thread by default. you can then call 
base::RunLoop().RunUntilIdle(); to run the task

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:97:
base::Bind(base::IgnoreResult(&DWriteFontProxyMessageFilter::Send), this,
no need to use PostTaskAndReply. a messagefilter can call Send() from the FILE
thread (or any thread) to send the result.

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:116:
DWriteFontProxyMsg_FindFamily::WriteReplyParams(reply_message, UINT32_MAX);
no need to use IPC_DELAY_REPLY here since you always have the result
synchronously

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1004: #if
defined(OS_WIN)
there's another OS_WIN above. use that.

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
File content/common/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_init_win.h:14: typedef IPC::Sender*
(*GetSenderFn)();
we normally don't use function pointers, but instead use closures.

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_init_win.h:18: InitializeDWriteFontProxy(const
base::Callback<IPC::Sender*(void)>& sender);
document what the parameters are.

looking at this, I'm wondering why this is in content/common and it's using an
IPC::Sender? that is normally used for sending IPCs. i.e. it should be in
content/child, since content/common is code that is shared with content/browser
and content/[child processes]

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_init_win.h:22: void CONTENT_EXPORT
UninitializeDWriteFontProxy();
why is this needed?

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
File content/common/dwrite_font_proxy_win.h (right):

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_win.h:31: // Implements a direct write font
collection that uses IPC to the browser to do
this should be in content/child

content/common is for code that's also run in the browser process

[Ilya Kulshin, 2015-11-03 18:56:22]

Ptal. Fixed naming, let me know if there's something I missed.

nasko@ - content/browser/ + content/ppapi_plugin/
jschuh@ - content/common/sandbox_win.cc
jam@ - chrome/ + rest of content/ + ui/gfx/
asvitkine@ - tools/metrics/histograms/

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:47:
PostTaskAndReplyFn postTaskAndReply)
On 2015/11/03 00:18:54, jam wrote:
> i dont think you need this extra complexity with this function pointer for the
> unittests. you can use TestBrowserThreadBundle and the FILE_THREAD will just
be
> the current thread by default. you can then call 
> base::RunLoop().RunUntilIdle(); to run the task

Yes, that's exactly what I needed. Thanks for the pointer!

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:97:
base::Bind(base::IgnoreResult(&DWriteFontProxyMessageFilter::Send), this,
On 2015/11/03 00:18:54, jam wrote:
> no need to use PostTaskAndReply. a messagefilter can call Send() from the FILE
> thread (or any thread) to send the result.

Done.

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:116:
DWriteFontProxyMsg_FindFamily::WriteReplyParams(reply_message, UINT32_MAX);
On 2015/11/03 00:18:54, jam wrote:
> no need to use IPC_DELAY_REPLY here since you always have the result
> synchronously 

This is still a callback into direct write, and I'm not sure how much work that
will take. Most of the time it should be fast, but I don't know if there's a
risk of blocking the thread. The same is true of OnGetFamilyCount, but that one
seems even less likely to be slow.

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1004: #if
defined(OS_WIN)
On 2015/11/03 00:18:54, jam wrote:
> there's another OS_WIN above. use that.

Done.

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
File content/common/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_init_win.h:14: typedef IPC::Sender*
(*GetSenderFn)();
On 2015/11/03 00:18:54, jam wrote:
> we normally don't use function pointers, but instead use closures.

Oops, forgot to remove the typedef.

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_init_win.h:18: InitializeDWriteFontProxy(const
base::Callback<IPC::Sender*(void)>& sender);
On 2015/11/03 00:18:54, jam wrote:
> document what the parameters are.
> 
> looking at this, I'm wondering why this is in content/common and it's using an
> IPC::Sender? that is normally used for sending IPCs. i.e. it should be in
> content/child, since content/common is code that is shared with
content/browser
> and content/[child processes]

Done.

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_init_win.h:22: void CONTENT_EXPORT
UninitializeDWriteFontProxy();
On 2015/11/03 00:18:54, jam wrote:
> why is this needed?

Direct write requires that font collection and font file loaders be registered
prior to use, and subsequently unregistered. We also don't have a good signal
for when the proxy is no longer needed, other than platform uninitialize, which
is where this is called (refcounting wouldn't work, because direct write itself
holds a reference on the loaders). Given that the proxy lives for the life of
the process, it's probably fine to not unregister, but I'd rather unregister
when we have the opportunity to do so.

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
File content/common/dwrite_font_proxy_win.h (right):

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_win.h:31: // Implements a direct write font
collection that uses IPC to the browser to do
On 2015/11/03 00:18:54, jam wrote:
> this should be in content/child
> 
> content/common is for code that's also run in the browser process

Done.

[Alexei Svitkine (slow), 2015-11-03 19:06:46]

https://codereview.chromium.org/1378353006/diff/300001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1378353006/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:8922: +<histogram
name="DWriteFontProxy.LoaderType" enum="DWriteFontLoaderType">
There's already a DirectWrite.Fonts. prefix, so please re-use that one rather
than introducing a new one.

https://codereview.chromium.org/1378353006/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:8924: +  <summary>The codepath that was
used to load a font family.</summary>
Is this logged for every font load? Mention this in the description. Same below.

[gab, 2015-11-04 18:29:44]

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

[gab, 2015-11-04 18:29:45]

I can't see whether you changed this already (this is CL is SO big!), but just a
reminder from our chat yesterday that you'll want to also delete the unused font
cache file.

Also, could you perhaps split this CL a bit? (I'm not a reviewer so I guess I
don't have a say but +2500 -1700 sounds insane to me)
Perhaps (1) new unhooked classes with unittests (2) hook it up (3) remove the
now unused font cache?

[gab, 2015-11-04 18:45:05]

gab@chromium.org changed reviewers:
- gab@chromium.org

[Ilya Kulshin, 2015-11-04 19:03:33]

On 2015/11/04 18:29:45, gab wrote:
> I can't see whether you changed this already (this is CL is SO big!), but just
a
> reminder from our chat yesterday that you'll want to also delete the unused
font
> cache file.

Since this CL is already so big, I'm thinking of removing the font cache file in
a followup CL (assuming no objections to that plan).

> Also, could you perhaps split this CL a bit? (I'm not a reviewer so I guess I
> don't have a say but +2500 -1700 sounds insane to me)
> Perhaps (1) new unhooked classes with unittests (2) hook it up (3) remove the
> now unused font cache?

You're welcome to still add comments. Re: splitting it up, that would still
result in an approx +2500 line review, and another approx -1500 line review,
with mostly independent sets of files. I could do that if people prefer.

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

https://codereview.chromium.org/1378353006/diff/300001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1378353006/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:8922: +<histogram
name="DWriteFontProxy.LoaderType" enum="DWriteFontLoaderType">
On 2015/11/03 19:06:46, Alexei Svitkine (slow) wrote:
> There's already a DirectWrite.Fonts. prefix, so please re-use that one rather
> than introducing a new one.

Done.

https://codereview.chromium.org/1378353006/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:8924: +  <summary>The codepath that was
used to load a font family.</summary>
On 2015/11/03 19:06:46, Alexei Svitkine (slow) wrote:
> Is this logged for every font load? Mention this in the description. Same
below.

Done.

[gab, 2015-11-04 19:29:36]

On 2015/11/04 19:03:33, Ilya Kulshin wrote:
> On 2015/11/04 18:29:45, gab wrote:
> > I can't see whether you changed this already (this is CL is SO big!), but
just
> a
> > reminder from our chat yesterday that you'll want to also delete the unused
> font
> > cache file.
> 
> Since this CL is already so big, I'm thinking of removing the font cache file
in
> a followup CL (assuming no objections to that plan).

That SGTM.

> 
> > Also, could you perhaps split this CL a bit? (I'm not a reviewer so I guess
I
> > don't have a say but +2500 -1700 sounds insane to me)
> > Perhaps (1) new unhooked classes with unittests (2) hook it up (3) remove
the
> > now unused font cache?
> 
> You're welcome to still add comments. Re: splitting it up, that would still
> result in an approx +2500 line review, and another approx -1500 line review,
> with mostly independent sets of files. I could do that if people prefer.

Looks like you have many new classes with their own unittests. I would suggest
making independent CLs for each of those, then one more CL to hook it all up,
followed by the one to remove the old code (conceptually removal belongs with
addition, but I think it makes sense to split it to ease reviews and land them
close together).

[Ilya Kulshin, 2015-11-06 19:58:14]

Ping, just making sure this doesn't get buried. If you still need more time,
that's fine.

[nasko, 2015-11-06 20:57:42]

On 2015/11/06 19:58:14, Ilya Kulshin wrote:
> Ping, just making sure this doesn't get buried. If you still need more time,
> that's fine.

I am not actively looking at this CL, as jam@ can cover all of content and I
will be out for a few days, so you will get more traction by other reviewers.

[Alexei Svitkine (slow), 2015-11-06 22:11:46]

Suggest following gab@'s suggestion of splitting this up into separate CLs.

Meanwhile, here's some random things I spotted in the current CL.

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:175:
DCHECK(name[length - 1] == L'\0');
Can this be a DCHECK_EQ()

Same for other ones.

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:194: const
wchar_t* kFontsToIgnore[] = {
These should be in an anon namespace at the top of the file.

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:337:
UMA_HISTOGRAM_ENUMERATION("DirectWrite.Fonts.Proxy.LoaderType",
Please make a helper function to log this histogram that takes the enum as a
param.

This way, you don't repeat the string name (no chance of typo), but also because
each macro expands to a lot of code, so having a function reduces bloat.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:24: namespace {
Nit: Add an empty line below:

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h:27: }  // namespace
content
Nit: Add an empty line above.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:366:
UMA_HISTOGRAM_ENUMERATION("DirectWrite.Fonts.Proxy.LoadFamily",
Make a helper function for this histogram.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:392:
DCHECK(family_count <= 1);
DCHECK_LE

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:456: }
Please add missing empty lines here and below.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:463: void**
fragmentContext) {
Use hacker_style names. Please fix throughout.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.h (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:63:
IDWriteFontFileStream** fontFileStream) override;
Nit: Convention is to put all overridden methods together (with no blank lines
between them) in a section following a comment of the form:

// BaseClassName:

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:84:
base::Callback<IPC::Sender*(void)> sender_;
DISALLOW_COPY_AND_ASSIGN()

Please fix throughout.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win_unittest.cc (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win_unittest.cc:11: #include
"content/child/dwrite_font_proxy/dwrite_font_proxy_win.h"
This should be the first include

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:52: WCHAR*
stringBuffer,
hacker_style

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:63: if (index >=
strings_.size()) {
Nit: No {}s

[jam, 2015-11-09 16:32:09]

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1378353006/diff/260001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:116:
DWriteFontProxyMsg_FindFamily::WriteReplyParams(reply_message, UINT32_MAX);
On 2015/11/03 18:56:21, Ilya Kulshin wrote:
> On 2015/11/03 00:18:54, jam wrote:
> > no need to use IPC_DELAY_REPLY here since you always have the result
> > synchronously 
> 
> This is still a callback into direct write, and I'm not sure how much work
that
> will take. Most of the time it should be fast, but I don't know if there's a
> risk of blocking the thread. The same is true of OnGetFamilyCount, but that
one
> seems even less likely to be slow.

I'm not sure what you mean. All I'm saying is don't use
IPC_MESSAGE_HANDLER_DELAY_REPLY and just use IPC_MESSAGE_HANDLER. I'm not
suggesting you change which thread you dispatch the IPC on.

IPC_MESSAGE_HANDLER_DELAY_REPLY should only be used when the dispatching method
doesn't have the result synchronously, i.e. it needs to do post task to various
threads and wait for callbacks.

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
File content/common/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1378353006/diff/260001/content/common/dwrite_...
content/common/dwrite_font_proxy_init_win.h:22: void CONTENT_EXPORT
UninitializeDWriteFontProxy();
On 2015/11/03 18:56:21, Ilya Kulshin wrote:
> On 2015/11/03 00:18:54, jam wrote:
> > why is this needed?
> 
> Direct write requires that font collection and font file loaders be registered
> prior to use, and subsequently unregistered. We also don't have a good signal
> for when the proxy is no longer needed, other than platform uninitialize,
which
> is where this is called (refcounting wouldn't work, because direct write
itself
> holds a reference on the loaders). Given that the proxy lives for the life of
> the process, it's probably fine to not unregister, but I'd rather unregister
> when we have the opportunity to do so.

i don't see the point of unregistering this before process shut down.

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:81:
BrowserThread::PostTask(
I forgot to mention this earlier. no need to have an IPC dispatcher in a class
that derives from BrowserMessageFilter just for posting a task to another
thread. just override OverrideThreadForMessage.

[Ilya Kulshin, 2015-11-11 01:22:11]

Fixed more comments. Also split this change into two parts:

Issue 1438603002 deals with creating the new classes.
Issue 1432123002 deals with switching to the new code and removing the existing
font cache.

I will start new reviews for these two issues. If you have additional comments,
please comment on those issues instead.

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:81:
BrowserThread::PostTask(
On 2015/11/09 16:32:08, jam wrote:
> I forgot to mention this earlier. no need to have an IPC dispatcher in a class
> that derives from BrowserMessageFilter just for posting a task to another
> thread. just override OverrideThreadForMessage.

Thanks, that works much better.

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:175:
DCHECK(name[length - 1] == L'\0');
On 2015/11/06 22:11:45, Alexei Svitkine (slow) wrote:
> Can this be a DCHECK_EQ()
> 
> Same for other ones.

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:194: const
wchar_t* kFontsToIgnore[] = {
On 2015/11/06 22:11:45, Alexei Svitkine (slow) wrote:
> These should be in an anon namespace at the top of the file.

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:337:
UMA_HISTOGRAM_ENUMERATION("DirectWrite.Fonts.Proxy.LoaderType",
On 2015/11/06 22:11:45, Alexei Svitkine (slow) wrote:
> Please make a helper function to log this histogram that takes the enum as a
> param.
> 
> This way, you don't repeat the string name (no chance of typo), but also
because
> each macro expands to a lot of code, so having a function reduces bloat.

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:24: namespace {
On 2015/11/06 22:11:45, Alexei Svitkine (slow) wrote:
> Nit: Add an empty line below:

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h:27: }  // namespace
content
On 2015/11/06 22:11:45, Alexei Svitkine (slow) wrote:
> Nit: Add an empty line above.

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:366:
UMA_HISTOGRAM_ENUMERATION("DirectWrite.Fonts.Proxy.LoadFamily",
On 2015/11/06 22:11:46, Alexei Svitkine (slow) wrote:
> Make a helper function for this histogram.

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:392:
DCHECK(family_count <= 1);
On 2015/11/06 22:11:46, Alexei Svitkine (slow) wrote:
> DCHECK_LE

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:456: }
On 2015/11/06 22:11:46, Alexei Svitkine (slow) wrote:
> Please add missing empty lines here and below.

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:463: void**
fragmentContext) {
On 2015/11/06 22:11:45, Alexei Svitkine (slow) wrote:
> Use hacker_style names. Please fix throughout.

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.h (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:63:
IDWriteFontFileStream** fontFileStream) override;
On 2015/11/06 22:11:46, Alexei Svitkine (slow) wrote:
> Nit: Convention is to put all overridden methods together (with no blank lines
> between them) in a section following a comment of the form:
> 
> // BaseClassName:

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:84:
base::Callback<IPC::Sender*(void)> sender_;
On 2015/11/06 22:11:46, Alexei Svitkine (slow) wrote:
> DISALLOW_COPY_AND_ASSIGN()
> 
> Please fix throughout.

Added DISALLOW_ASSIGN. DISALLOW_COPY_AND_ASSIGN fails to compile due to use of
Microsoft::WRL::RuntimeClass.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win_unittest.cc (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win_unittest.cc:11: #include
"content/child/dwrite_font_proxy/dwrite_font_proxy_win.h"
On 2015/11/06 22:11:46, Alexei Svitkine (slow) wrote:
> This should be the first include

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc (right):

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:52: WCHAR*
stringBuffer,
On 2015/11/06 22:11:46, Alexei Svitkine (slow) wrote:
> hacker_style

Done.

https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:63: if (index >=
strings_.size()) {
On 2015/11/06 22:11:46, Alexei Svitkine (slow) wrote:
> Nit: No {}s

Done.

[Ilya Kulshin, 2015-12-22 00:42:12]

Description was changed from

==========
This change uses IPC to communicate with the system font collection in the
browser process for font enumeration, bypassing the dwrite font cache.

dwrite_font_proxy_message_filter is the browser-side implementation that will
handle the IPC messages. dwrite_font_proxy is the renderer-side. Most of the
rest is removing of the existing font cache and putting everything together.

Feel free to ignore the first 13 patch sets and start at #14.

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

to

==========
Closing this review in favor of the two new reviews.


This change uses IPC to communicate with the system font collection in the
browser process for font enumeration, bypassing the dwrite font cache.

dwrite_font_proxy_message_filter is the browser-side implementation that will
handle the IPC messages. dwrite_font_proxy is the renderer-side. Most of the
rest is removing of the existing font cache and putting everything together.

Feel free to ignore the first 13 patch sets and start at #14.

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