Create direct write font proxy classes and unit tests.

Create direct write font proxy classes and unit tests.

BUG=525142
Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

Committed: https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}

Committed: https://crrev.com/4039fd5b32b74b01866ca4c1ce73998f19320de1
Cr-Commit-Position: refs/heads/master@{#365122}

[Ilya Kulshin, 2015-11-11 02:17:35]

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

[Ilya Kulshin, 2015-11-11 02:17:35]

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

ptal:
asvitkine@ - tools/metrics/histograms/
jam@ - content/, ui/gfx/win/, ipc/

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

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

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

readding jschuh. he should review the core direct write font logic

i mostly before was looking at how this is hooked up.

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:111: case
DWriteFontProxyMsg_GetFontFiles::ID:
can just be

if (IPC_MESSAGE_CLASS(message) == DWriteFontProxyMsgStart)
  *thread = BrowserThread::FILE;

since you're doing this for all messages in that class

[jschuh, 2015-11-12 23:38:53]

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

[jschuh, 2015-11-12 23:38:53]

ananta@  - I'll give this a pass and cover the IPC security stuff as well, but
could you review the DWrite logic as the person most familiar?

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

+ananta

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

Description was changed from

==========
Create direct write font proxy classes and unit tests.

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

to

==========
Create direct write font proxy classes and unit tests.

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

[ananta, 2015-11-12 23:42:36]

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

[ananta, 2015-11-12 23:42:37]

+scottmg

[ananta, 2015-11-13 00:08:10]

Have some questions about the approach.

Based on a cursory reading of the patch it looks like we are retaining the
mechanics of the custom font collection for now and are using that to initialize
DirectWrite by fetching relevant font info from the browser. So every renderer
during startup is going to initialize DirectWrite and do a bunch of sync calls
to the browser to get font family and related information. Is this performing
better than the current cache based approach?

Is this a shortterm approach to get us to the final goal which is to avoid
enumerating the system font collection in skia and just retrieve information
from the browser about specific fonts we need?

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:128: if
(SUCCEEDED(hr))
I don't think the SUCCEEDED check is correct. You need to check if exists is
true

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:143:
uint32 family_index,
I am worried about trusting the family_index coming in from the renderer. jschuh
thoughts?

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:196:
uint32 family_index,
Ditto about the family index

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.h (right):

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:97:
GetFontCollection(IDWriteFontCollection** font_collection) override;
new line after each method and fix alignment

[Ilya Kulshin, 2015-11-13 00:41:12]

Regarding ananta@'s concerns:
> Have some questions about the approach.

> Based on a cursory reading of the patch it looks like we are retaining the
> mechanics of the custom font collection for now and are using that to
initialize
> DirectWrite by fetching relevant font info from the browser. So every renderer
> during startup is going to initialize DirectWrite and do a bunch of sync calls
> to the browser to get font family and related information. Is this performing
> better than the current cache based approach?

> Is this a shortterm approach to get us to the final goal which is to avoid
> enumerating the system font collection in skia and just retrieve information
> from the browser about specific fonts we need?

Yes, we are retaining the approach of using a custom collection. The major
difference is that instead of loading every font installed on the system (as the
cache was doing) we are only loading the files for the font families that end up
being used. It's been hard to get useful and accurate perf numbers, but my
latest testing indicates that this approach results in a small boost in renderer
startup. The overall time to first paint is slightly improved for pages that use
relatively few font families, and slightly worsened for pages that use lots of
font families. This change also eliminates the hangs associated with building
the font cache in the first place.

We considered alternative solutions for loading fonts, but there were concerns
with our ability to maintain display parity. With this approach that is not as
much of a concern since we are still using the same underlying system. So I
think we expect this approach to stick around at least for a while.

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:128: if
(SUCCEEDED(hr))
On 2015/11/13 00:08:10, ananta wrote:
> I don't think the SUCCEEDED check is correct. You need to check if exists is
> true

If the API succeeds and the family does not exist, index is set to UINT32_MAX,
which is also what the IPC message uses to indicate failure. I'll change the
code to be more explicit (but do the same thing) anyway.

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:143:
uint32 family_index,
On 2015/11/13 00:08:10, ananta wrote:
> I am worried about trusting the family_index coming in from the renderer.
jschuh
> thoughts?

If the family_index is out of bounds, GetFontFamily will return a failure code
and the browser just won't get any data.

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:196:
uint32 family_index,
On 2015/11/13 00:08:10, ananta wrote:
> Ditto about the family index

As with the family names, an invalid index results in zero files being returned.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.h (right):

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:97:
GetFontCollection(IDWriteFontCollection** font_collection) override;
On 2015/11/13 00:08:10, ananta wrote:
> new line after each method and fix alignment

Alignment and line wrapping are done by "git cl format". I agree that it looks a
little silly, but fixing that is out of scope of this cl.

According to asvitkine@ in
https://codereview.chromium.org/1378353006/diff/320001/content/child/dwrite_f...,
the convention is for no new line between subsequent overrides. If you disagree,
let's resolve this via email.

[scottmg, 2015-11-13 00:51:46]

Just some superficial things for now, still trying to absorb all this.

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:52: //
weights,
Reformat this comment.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right):

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:43: //
content_browsertests, and possibly other high-stress cases.
I'm worried about this, does this mean we're falling back to non-proxied
sometimes then?

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h:16: void
CONTENT_EXPORT
The CONTENT_EXPORTs go before void.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right):

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:34:
UMA_HISTOGRAM_ENUMERATION("DirectWrite.Fonts.Proxy.LoadFamily", result,
Glad to see these UMA added here! Can we add something that tracks how long
things are taking, since that's the big motivation for making this change?

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:41: DCHECK(family_name
!= nullptr);
DCHECK_NE

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:208: this
/*collectonLoader*/, reinterpret_cast<const void*>(&index),
collectionLoader

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:464: HRESULT
FontFileStream::RuntimeClassInitialize(
Newline.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc#...
ui/gfx/win/direct_write.cc:7: #include <dwrite.h>
You can remove this, now that it's in the header.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc#...
ui/gfx/win/direct_write.cc:27: base::win::ScopedComPtr<IDWriteFactory> factory_;
Normally g_factory, instead of factory_.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc#...
ui/gfx/win/direct_write.cc:28: }
Add newline before }.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc#...
ui/gfx/win/direct_write.cc:109: base::win::ScopedComPtr<IDWriteFactory>
factory_ref = factory_;
It seems like

  factory_->AddRef();
  *factory = factory_;

is what's happening and that's probably more clear.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.h
File ui/gfx/win/direct_write.h (right):

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.h#n...
ui/gfx/win/direct_write.h:23: // Returns the direct write factory, if using
direct write, or null otherwise.
"direct write" -> "DirectWrite" in a bunch of places.

[Ilya Kulshin, 2015-11-14 00:25:34]

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:52: //
weights,
On 2015/11/13 00:51:46, scottmg wrote:
> Reformat this comment.

Done.

https://codereview.chromium.org/1438603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:111: case
DWriteFontProxyMsg_GetFontFiles::ID:
On 2015/11/11 23:03:15, jam wrote:
> can just be
> 
> if (IPC_MESSAGE_CLASS(message) == DWriteFontProxyMsgStart)
>   *thread = BrowserThread::FILE;
> 
> since you're doing this for all messages in that class

Done.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right):

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:43: //
content_browsertests, and possibly other high-stress cases.
On 2015/11/13 00:51:46, scottmg wrote:
> I'm worried about this, does this mean we're falling back to non-proxied
> sometimes then?

It still uses the proxy, but if it tries and fails to communicate with the
system collection (as opposed to being blocked completely by
PatchServiceManagerCalls) it falls back to really weird behavior, like loading
each font style into a separate family. I haven't been able to come up with a
good explanation for that behavior, but calling PatchServiceManagerCalls fixes
it.

Note that this call also exists in the existing font cache case.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h:16: void
CONTENT_EXPORT
On 2015/11/13 00:51:46, scottmg wrote:
> The CONTENT_EXPORTs go before void.

Done.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right):

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:34:
UMA_HISTOGRAM_ENUMERATION("DirectWrite.Fonts.Proxy.LoadFamily", result,
On 2015/11/13 00:51:46, scottmg wrote:
> Glad to see these UMA added here! Can we add something that tracks how long
> things are taking, since that's the big motivation for making this change?

There is the Startup.FirstWebContents.NonEmptyPaint, which is what I've been
using for measuring perf. I also have some trace events.

I'll add some histograms to measure perf, but we won't be able to use the new
histograms to tell if this results in an improvement, since the font cache lacks
comparable instrumentation.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:41: DCHECK(family_name
!= nullptr);
On 2015/11/13 00:51:46, scottmg wrote:
> DCHECK_NE

Unfortunately, I haven't been able to get DCHECK_NE to compile when nullptr is
involved. If anyone knows of a solution, I'd be glad to try it.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:208: this
/*collectonLoader*/, reinterpret_cast<const void*>(&index),
On 2015/11/13 00:51:46, scottmg wrote:
> collectionLoader

Done.

https://codereview.chromium.org/1438603002/diff/1/content/child/dwrite_font_p...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:464: HRESULT
FontFileStream::RuntimeClassInitialize(
On 2015/11/13 00:51:46, scottmg wrote:
> Newline.

Done.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc#...
ui/gfx/win/direct_write.cc:7: #include <dwrite.h>
On 2015/11/13 00:51:46, scottmg wrote:
> You can remove this, now that it's in the header.

Done.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc#...
ui/gfx/win/direct_write.cc:27: base::win::ScopedComPtr<IDWriteFactory> factory_;
On 2015/11/13 00:51:46, scottmg wrote:
> Normally g_factory, instead of factory_.

Done.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc#...
ui/gfx/win/direct_write.cc:28: }
On 2015/11/13 00:51:46, scottmg wrote:
> Add newline before }.

"git cl format" insists on deleting that newline.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.cc#...
ui/gfx/win/direct_write.cc:109: base::win::ScopedComPtr<IDWriteFactory>
factory_ref = factory_;
On 2015/11/13 00:51:46, scottmg wrote:
> It seems like
> 
>   factory_->AddRef();
>   *factory = factory_;
> 
> is what's happening and that's probably more clear.

Unfortunately, ScopedComPtr explicitly blocks AddRef and Release. What I
actually want is Microsoft::WRL::ComPtr::CopyTo. Which probably means I should
just use ComPtr in the first place.

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.h
File ui/gfx/win/direct_write.h (right):

https://codereview.chromium.org/1438603002/diff/1/ui/gfx/win/direct_write.h#n...
ui/gfx/win/direct_write.h:23: // Returns the direct write factory, if using
direct write, or null otherwise.
On 2015/11/13 00:51:46, scottmg wrote:
> "direct write" -> "DirectWrite" in a bunch of places.

Done.

[ananta, 2015-11-16 23:03:03]

https://codereview.chromium.org/1438603002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/20001/content/browser/rendere...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:66:
DWriteFontProxyMessageFilter::DWriteFontProxyMessageFilter()
The Browser message filter is invoked on the IO thread?. I don't think it is
safe to be using the DirectWrite factory
initialized on the UI thread on the IO thread?. Even if that works it violates
COM thread safety rules.

[Ilya Kulshin, 2015-11-17 22:49:36]

ptal.

https://codereview.chromium.org/1438603002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/20001/content/browser/rendere...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:66:
DWriteFontProxyMessageFilter::DWriteFontProxyMessageFilter()
On 2015/11/16 23:03:03, ananta wrote:
> The Browser message filter is invoked on the IO thread?. I don't think it is
> safe to be using the DirectWrite factory
> initialized on the UI thread on the IO thread?. Even if that works it violates
> COM thread safety rules.

Done.

[scottmg, 2015-11-18 00:11:36]

Do we do SetCrashKeyValue("font_key_name", ...) anywhere?
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/dwr...

It might be useful to see if we get crashes on any particular fonts. (That'll
show up as the last value it's set to in crash/ dumps.)

Otherwise, I think we're ready to go for it. I'd like to see followups that
remove the need to vtable patch by abstracting the "DirectWrite"ness out of the
stuff that's used in the renderer, and eventually even avoid loading dwrite.dll
in the renderers. But this is moving us in a good direction.

[Ilya Kulshin, 2015-11-18 22:43:02]

Added crash key logging to family loading, which is where the bulk of this code
runs. Most of the rest is IPCs or callthroughs to direct write, which I think is
much less likely to be font-file specific, but I can add logging there as well
if we're seeing problems in those areas.

[scottmg, 2015-11-18 22:45:48]

https://codereview.chromium.org/1438603002/diff/80001/content/child/dwrite_fo...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right):

https://codereview.chromium.org/1438603002/diff/80001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:373:
base::WideToUTF8(family_name_));
I think we're more likely to see a crash in the browser now, right? Not in the
renderer? This won't be visible in the dump if the browser crashes while
servicing this request.

[jschuh, 2015-11-18 23:20:20]

lgtm for ipc security

[scottmg, 2015-11-18 23:35:44]

lgtm

https://codereview.chromium.org/1438603002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/80001/content/browser/rendere...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:259:
local_loader->GetFilePathLengthFromKey(key, key_size, &path_length);
Check HR here.

[jam, 2015-11-20 17:39:32]

https://codereview.chromium.org/1438603002/diff/80001/content/child/dwrite_fo...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1438603002/diff/80001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
please add an owners file in this new directory with you/scottmg/justin as
owners

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

https://codereview.chromium.org/1438603002/diff/80001/content/common/font_war...
content/common/font_warmup_win.cc:414: // Directwrite connects to the font cache
service to retrieve information about
why is this code in content/common? i see it's only called from content/renderer
and content/ppapi_plugin. since content/common is code that is shared between
child and browser process, and this is only called from child processes, it
should just be in content/child

https://codereview.chromium.org/1438603002/diff/80001/content/renderer/dwrite...
File content/renderer/dwrite_font_proxy_init_win.cc (right):

https://codereview.chromium.org/1438603002/diff/80001/content/renderer/dwrite...
content/renderer/dwrite_font_proxy_init_win.cc:87: // Directwrite connects to
the font cache service to retrieve information about
why is there copied code here? i.e. why isn't this sharing the other code?

[Ilya Kulshin, 2015-11-20 22:10:57]

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

[Ilya Kulshin, 2015-11-20 22:10:58]

ptal.

+asvitkine for tools/metrics/histograms

https://codereview.chromium.org/1438603002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
(right):

https://codereview.chromium.org/1438603002/diff/80001/content/browser/rendere...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:259:
local_loader->GetFilePathLengthFromKey(key, key_size, &path_length);
On 2015/11/18 23:35:44, scottmg wrote:
> Check HR here.

Done.

https://codereview.chromium.org/1438603002/diff/80001/content/child/dwrite_fo...
File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h (right):

https://codereview.chromium.org/1438603002/diff/80001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2015/11/20 17:39:31, jam wrote:
> please add an owners file in this new directory with you/scottmg/justin as
> owners

Done.

https://codereview.chromium.org/1438603002/diff/80001/content/child/dwrite_fo...
File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (right):

https://codereview.chromium.org/1438603002/diff/80001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:373:
base::WideToUTF8(family_name_));
On 2015/11/18 22:45:48, scottmg wrote:
> I think we're more likely to see a crash in the browser now, right? Not in the
> renderer? This won't be visible in the dump if the browser crashes while
> servicing this request.

The renderer does not have easy access to the family name (it can get it, it
just requires some extra code). Let's see if we actually get crashes in that
code before adding that logging.

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

https://codereview.chromium.org/1438603002/diff/80001/content/common/font_war...
content/common/font_warmup_win.cc:414: // Directwrite connects to the font cache
service to retrieve information about
On 2015/11/20 17:39:31, jam wrote:
> why is this code in content/common? i see it's only called from
content/renderer
> and content/ppapi_plugin. since content/common is code that is shared between
> child and browser process, and this is only called from child processes, it
> should just be in content/child

Mostly because it was here when I found it. Unless you want it moved
immediately, I'll move it in a follow-up change.

Created review 1455693009  and bug 559333 to track misc work items after
switching to dwrite font proxy.

https://codereview.chromium.org/1438603002/diff/80001/content/renderer/dwrite...
File content/renderer/dwrite_font_proxy_init_win.cc (right):

https://codereview.chromium.org/1438603002/diff/80001/content/renderer/dwrite...
content/renderer/dwrite_font_proxy_init_win.cc:87: // Directwrite connects to
the font cache service to retrieve information about
On 2015/11/20 17:39:31, jam wrote:
> why is there copied code here? i.e. why isn't this sharing the other code?

This whole file was moved to content/child/. Not sure why it's still showing up
in content/renderer.

[Alexei Svitkine (slow), 2015-11-23 16:25:40]

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

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:71:
DCHECK(SHGetSpecialFolderPath(NULL /* hwndOwner - reserved */,
I don't think this is what you want. This code will be compiled out if DCHECKs
are off. Assign to a local variable and dcheck on that instead.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:102:
DCHECK_CURRENTLY_ON(BrowserThread::FILE);
DCHECKs should be at the start of the function.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:107:
uint32 index;
Nit: Initialize here or some tools may warn.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:152: for
(unsigned int index = 0; index < string_count; index++) {
++index

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:226: for
(unsigned int file_index = 0; file_index < file_count; file_index++) {
This function is pretty long. Can it be refactored by splitting parts off to
helper functions?

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h
(right):

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h:29: 
Needs an explicit out-of-line dtor.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h:51: };
DISALLOW_COPY_AND_ASSIGN

Please add throughout.

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

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:119: DCHECK(font_face
!= nullptr);
Nit: I think you can just DCHECK(font_face); - please fix throughout.

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:123: if (family !=
nullptr && family->GetFontFromFontFace(font_face, font)) {
Nit: family && family->...

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_localized_strings_win.h (right):

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.h:50:
DISALLOW_ASSIGN(DWriteLocalizedStrings);
Nit: Empty line above this.

[jam, 2015-11-24 00:11:31]

rubberstamp lgtm, deferring to other reviewers (my comments have been addressed)

[Ilya Kulshin, 2015-11-24 01:45:27]

ptal.

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

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:71:
DCHECK(SHGetSpecialFolderPath(NULL /* hwndOwner - reserved */,
On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> I don't think this is what you want. This code will be compiled out if DCHECKs
> are off. Assign to a local variable and dcheck on that instead.

Nice catch.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:102:
DCHECK_CURRENTLY_ON(BrowserThread::FILE);
On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> DCHECKs should be at the start of the function.

Removed the thread dchecks, since InitializeDirectWrite will dcheck the thread
anyway. The collection_ dcheck needs to be after InitializeDirectWrite, since on
the first call collection_ will be null and will be initialized by
InitializeDirectWrite.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:107:
uint32 index;
On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> Nit: Initialize here or some tools may warn.

Done.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:152: for
(unsigned int index = 0; index < string_count; index++) {
On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> ++index

Done.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:226: for
(unsigned int file_index = 0; file_index < file_count; file_index++) {
On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> This function is pretty long. Can it be refactored by splitting parts off to
> helper functions?

Done. Factored out the contents of each loop into a function.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h
(right):

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h:29: 
On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> Needs an explicit out-of-line dtor.

Done. Can you elaborate on the requirement for non-inline dtor?

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h:51: };
On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> DISALLOW_COPY_AND_ASSIGN
> 
> Please add throughout.

Can you elaborate on what you mean by this comment? This class already has a
DISALLOW_COPY_AND_ASSIGN declaration. Other classes in this CL also have similar
declarations (some classes require a copy constructor, so they just use
DISALLOW_ASSIGN instead).

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

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:119: DCHECK(font_face
!= nullptr);
On 2015/11/23 16:25:40, Alexei Svitkine (slow) wrote:
> Nit: I think you can just DCHECK(font_face); - please fix throughout.

Done. Is the implicit conversion to bool actually mandated by the style guide,
or is it still left as a matter of preference?

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:123: if (family !=
nullptr && family->GetFontFromFontFace(font_face, font)) {
On 2015/11/23 16:25:40, Alexei Svitkine (slow) wrote:
> Nit: family && family->...

Done.

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_localized_strings_win.h (right):

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.h:50:
DISALLOW_ASSIGN(DWriteLocalizedStrings);
On 2015/11/23 16:25:40, Alexei Svitkine (slow) wrote:
> Nit: Empty line above this.

Done.

[Alexei Svitkine (slow), 2015-11-24 16:42:36]

Sorry, more comments. It's still a big change...

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h
(right):

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h:29: 
On 2015/11/24 01:45:27, Ilya Kulshin wrote:
> On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> > Needs an explicit out-of-line dtor.
> 
> Done. Can you elaborate on the requirement for non-inline dtor?

Sure. If a class has any non-POD members, then the dtor will have a non-trivial
amount of generated code for it. If it's not out of line, that code will be
generated every time this header file is included and later merged away at the
link stage. This slows down build times. I believe there's even a warning for
this on other platforms by a clang plugin, but I guess that doesn't run yet on
Windows.

https://codereview.chromium.org/1438603002/diff/120001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h:51: };
On 2015/11/24 01:45:27, Ilya Kulshin wrote:
> On 2015/11/23 16:25:39, Alexei Svitkine (slow) wrote:
> > DISALLOW_COPY_AND_ASSIGN
> > 
> > Please add throughout.
> 
> Can you elaborate on what you mean by this comment? This class already has a
> DISALLOW_COPY_AND_ASSIGN declaration. Other classes in this CL also have
similar
> declarations (some classes require a copy constructor, so they just use
> DISALLOW_ASSIGN instead).

Woops, I missed it - mainly because convention is to have a blank line before
it, so it's easy to catch.

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

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:119: DCHECK(font_face
!= nullptr);
On 2015/11/24 01:45:27, Ilya Kulshin wrote:
> On 2015/11/23 16:25:40, Alexei Svitkine (slow) wrote:
> > Nit: I think you can just DCHECK(font_face); - please fix throughout.
> 
> Done. Is the implicit conversion to bool actually mandated by the style guide,
> or is it still left as a matter of preference?

It's probably a preference that most of Chromium follows.

https://codereview.chromium.org/1438603002/diff/120001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:122: for (auto& family
: families_) {
Nit: const auto&

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

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:71: BOOL
result = SHGetSpecialFolderPath(NULL /* hwndOwner - reserved */,
Nit: nullptr

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:75:
windows_fonts_path_ = base::i18n::FoldCase(font_path_chars.data());
Nit: Can you make a helper function in the anon namespace that wraps this logic?
then you can just initialize this member in the init list.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:113:
*family_index = exists ? index : UINT32_MAX;
Nit: Line 106 already set it to UINT32_MAX. So you can just put the exists in
the if.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:152: for
(unsigned int index = 0; index < string_count; ++index) {
Shouldn't this be UINT32, since that's the type of the GetLocaleNameLength()
param?

Fix throughout.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:162:
DCHECK_EQ(locale[length - 1], L'\0');
Nit: Put the expected value on the left.

Fix throughout.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:236: hr =
font->CreateFontFace(&font_face);
Nit: Merge with the line above.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:279:
font_files[file_index].Get()))
Nit: {} since if is multi-line.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:317:
DCHECK(false);
Why DCHECK(false)? If you still like to keep it, use NOTREACHED()

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
File
content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc
(right):

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc:90:
for (auto& pair : names) {
Nit: const

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc:113:
for (base::string16& file : files) {
Nit: const

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

https://codereview.chromium.org/1438603002/diff/140001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:122: for (auto& family
: families_) {
Nit: const

https://codereview.chromium.org/1438603002/diff/140001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:123: if (family &&
family->GetFontFromFontFace(font_face, font)) {
Nit: No {}'s

https://codereview.chromium.org/1438603002/diff/140001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:383: HRESULT hr;
Nit: I'd just declare this on line 390 and then on line 410.

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

https://codereview.chromium.org/1438603002/diff/140001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win_unittest.cc:181: void
CheckLocale(IDWriteLocalizedStrings* strings,
Non-const params should be last.

Also, should this be in the anon namespace?

https://codereview.chromium.org/1438603002/diff/140001/content/common/dwrite_...
File content/common/dwrite_font_proxy_messages.h (right):

https://codereview.chromium.org/1438603002/diff/140001/content/common/dwrite_...
content/common/dwrite_font_proxy_messages.h:22: uint32 /* out index */)
I believe for new code, the preference is to use uint32_t types. Please switch
to this (including the handlers and other dependent code.)

https://codereview.chromium.org/1438603002/diff/140001/content/test/dwrite_fo...
File content/test/dwrite_font_fake_sender_win.h (right):

https://codereview.chromium.org/1438603002/diff/140001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:20: IPC::Sender*
CreateFakeCollectionSender();
Forward decls should come before this. Add a comment?

https://codereview.chromium.org/1438603002/diff/140001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:22: class FakeFontCollection;
Nit: Add an empty line below.

https://codereview.chromium.org/1438603002/diff/140001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:43: 
DISALLOW_COPY...

Please put on all classes you define that don't need to be copyable, including
test classes.

[Ilya Kulshin, 2015-11-25 02:40:48]

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

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:71: BOOL
result = SHGetSpecialFolderPath(NULL /* hwndOwner - reserved */,
On 2015/11/24 16:42:35, Alexei Svitkine (slow) wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:75:
windows_fonts_path_ = base::i18n::FoldCase(font_path_chars.data());
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Nit: Can you make a helper function in the anon namespace that wraps this
logic?
> then you can just initialize this member in the init list.

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:113:
*family_index = exists ? index : UINT32_MAX;
On 2015/11/24 16:42:35, Alexei Svitkine (slow) wrote:
> Nit: Line 106 already set it to UINT32_MAX. So you can just put the exists in
> the if.

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:152: for
(unsigned int index = 0; index < string_count; ++index) {
On 2015/11/24 16:42:35, Alexei Svitkine (slow) wrote:
> Shouldn't this be UINT32, since that's the type of the GetLocaleNameLength()
> param?
> 
> Fix throughout.

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:162:
DCHECK_EQ(locale[length - 1], L'\0');
On 2015/11/24 16:42:35, Alexei Svitkine (slow) wrote:
> Nit: Put the expected value on the left.
> 
> Fix throughout.

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:236: hr =
font->CreateFontFace(&font_face);
On 2015/11/24 16:42:35, Alexei Svitkine (slow) wrote:
> Nit: Merge with the line above.

For things like "HRESULT hr" which are assigned multiple times and are only used
immediately after, I find it more convenient to declare the variable separately
from the initial assignment. That makes subsequent code modification more
convenient, as it allows calls to be inserted or removed without causing compile
errors.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:279:
font_files[file_index].Get()))
On 2015/11/24 16:42:35, Alexei Svitkine (slow) wrote:
> Nit: {} since if is multi-line.

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:317:
DCHECK(false);
On 2015/11/24 16:42:35, Alexei Svitkine (slow) wrote:
> Why DCHECK(false)? If you still like to keep it, use NOTREACHED()

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
File
content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc
(right):

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc:90:
for (auto& pair : names) {
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Nit: const

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win_unittest.cc:113:
for (base::string16& file : files) {
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Nit: const

Done.

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

https://codereview.chromium.org/1438603002/diff/140001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:122: for (auto& family
: families_) {
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Nit: const

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:123: if (family &&
family->GetFontFromFontFace(font_face, font)) {
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Nit: No {}'s

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:383: HRESULT hr;
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Nit: I'd just declare this on line 390 and then on line 410.

For things like "HRESULT hr" which are often assigned multiple times, I find it
more convenient to split declaration and assignment, which makes it more
convenient to add/remove calls in the future.

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

https://codereview.chromium.org/1438603002/diff/140001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win_unittest.cc:181: void
CheckLocale(IDWriteLocalizedStrings* strings,
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Non-const params should be last.
> 
> Also, should this be in the anon namespace?

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/common/dwrite_...
File content/common/dwrite_font_proxy_messages.h (right):

https://codereview.chromium.org/1438603002/diff/140001/content/common/dwrite_...
content/common/dwrite_font_proxy_messages.h:22: uint32 /* out index */)
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> I believe for new code, the preference is to use uint32_t types. Please switch
> to this (including the handlers and other dependent code.)

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/test/dwrite_fo...
File content/test/dwrite_font_fake_sender_win.h (right):

https://codereview.chromium.org/1438603002/diff/140001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:20: IPC::Sender*
CreateFakeCollectionSender();
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Forward decls should come before this. Add a comment?

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:22: class FakeFontCollection;
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> Nit: Add an empty line below.

Done.

https://codereview.chromium.org/1438603002/diff/140001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:43: 
On 2015/11/24 16:42:36, Alexei Svitkine (slow) wrote:
> DISALLOW_COPY...
> 
> Please put on all classes you define that don't need to be copyable, including
> test classes.

Done.

[Alexei Svitkine (slow), 2015-11-25 20:17:46]

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

https://codereview.chromium.org/1438603002/diff/160001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:64: const
base::string16 GetWindowsFontsPath() {
Nit: No const on return value

https://codereview.chromium.org/1438603002/diff/160001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h
(right):

https://codereview.chromium.org/1438603002/diff/160001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h:38: void
OnFindFamily(const base::string16& family_name, uint32* family_index);
Please eliminate all uses of uint32 in the code. Either switch to uint32_t, if
it doesn't have any dependencies on  external API types, or use the external API
types (i.e. UINT32 capitalized).

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

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:57:
base::WideToUTF16(family_name, wcslen(family_name), &name);
Nit: Does base::string16 name(family_name); work?

I think on Windows string16 and wstring are equivalent and wstring can be
initialized with a C-style string.

But I could be wrong.

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

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:20: namespace mswr =
Microsoft::WRL;
No namespace decls in header files.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:42: //
IDWriteFontCollection:
Add an empty line above.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:123: void
SetName(base::string16 family_name) { family_name_.assign(family_name); }
Trivial functions should be named hacker_style; not trivial functions should not
be inlined. Same comment for line 125 below.

Also, the param should be const&.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:131: unsigned int
family_index_;
Nit: size_t?

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

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:16: for
(unsigned int n = 0; n < strings_.size(); ++n) {
Convention is to use size_t for std::vector indexes in Chromium code.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:38: if (size <=
strings_[index].first.size())
If it's equal, shouldn't it be OK? Or is this to account for the null
terminator? If so, add a comment.

Also, if you're ensuring there's space for the null terminator, then please
ensure to add it. Same comments for GetString() below.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:40:
wcsncpy(locale_name, strings_[index].first.data(), size);
This isn't correct. You're copying |size| bytes, but |size| is the size of the
buffer, not the size of the string!

You should use the size of the string and also ensure its null terminated.

After fixing this, please add a unit test (e.g. that tries to copy a small
string into a 1k buffer) that would have crashed the previous code.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_localized_strings_win.h (right):

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.h:16: namespace
mswr = Microsoft::WRL;
Header files shouldn't have using declarations.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.h:24: public:
Nit: Add an explicit default ctor.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.h:25:
~DWriteLocalizedStrings();
Shouldn't this have override() since it's a subclass?

https://codereview.chromium.org/1438603002/diff/160001/content/common/dwrite_...
File content/common/dwrite_font_proxy_messages.h (right):

https://codereview.chromium.org/1438603002/diff/160001/content/common/dwrite_...
content/common/dwrite_font_proxy_messages.h:17: typedef
std::pair<base::string16, base::string16> StringPair;
Nit: Please name this type more specifically, e.g. DWriteStringPair.

Otherwise, you're polluting the global namespace with this generic named type.

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

https://codereview.chromium.org/1438603002/diff/160001/content/common/font_wa...
content/common/font_warmup_win.cc:444: DCHECK(patched == 0);
Nit: DCHECK_EQ; fix throughout.

[Ilya Kulshin, 2015-12-02 02:23:02]

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

https://codereview.chromium.org/1438603002/diff/160001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:64: const
base::string16 GetWindowsFontsPath() {
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Nit: No const on return value

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/browser/render...
File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h
(right):

https://codereview.chromium.org/1438603002/diff/160001/content/browser/render...
content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h:38: void
OnFindFamily(const base::string16& family_name, uint32* family_index);
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Please eliminate all uses of uint32 in the code. Either switch to uint32_t, if
> it doesn't have any dependencies on  external API types, or use the external
API
> types (i.e. UINT32 capitalized).

Done.

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

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:57:
base::WideToUTF16(family_name, wcslen(family_name), &name);
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Nit: Does base::string16 name(family_name); work?
> 
> I think on Windows string16 and wstring are equivalent and wstring can be
> initialized with a C-style string.
> 
> But I could be wrong.

Done.

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

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:20: namespace mswr =
Microsoft::WRL;
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> No namespace decls in header files.

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:42: //
IDWriteFontCollection:
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Add an empty line above.

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:123: void
SetName(base::string16 family_name) { family_name_.assign(family_name); }
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Trivial functions should be named hacker_style; not trivial functions should
not
> be inlined. Same comment for line 125 below.
> 
> Also, the param should be const&.

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_font_proxy_win.h:131: unsigned int
family_index_;
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Nit: size_t?

Switched to UINT32, since these indexes usually start as or end up needing to be
a UINT32 anyway.

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

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:16: for
(unsigned int n = 0; n < strings_.size(); ++n) {
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Convention is to use size_t for std::vector indexes in Chromium code.

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:38: if (size <=
strings_[index].first.size())
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> If it's equal, shouldn't it be OK? Or is this to account for the null
> terminator? If so, add a comment.
> 
> Also, if you're ensuring there's space for the null terminator, then please
> ensure to add it. Same comments for GetString() below.

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.cc:40:
wcsncpy(locale_name, strings_[index].first.data(), size);
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> This isn't correct. You're copying |size| bytes, but |size| is the size of the
> buffer, not the size of the string!
> 
> You should use the size of the string and also ensure its null terminated.
> 
> After fixing this, please add a unit test (e.g. that tries to copy a small
> string into a 1k buffer) that would have crashed the previous code.

wcsncpy will not overread the source (it will instead pad with null characters),
so |size| is safe to use. I prefer to use |size| as-is, to reduce the risk of
making a mistake while calculating an acceptable number of characters to copy.
Since we ensure that |size| is greater than the number of characters, wcsncpy
will also ensure that the output is null terminated.

I changed data() to c_str(), but that change is only for readability and will
have no effect (the two functions are equivalent).

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
File content/child/dwrite_font_proxy/dwrite_localized_strings_win.h (right):

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.h:16: namespace
mswr = Microsoft::WRL;
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Header files shouldn't have using declarations.

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.h:24: public:
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Nit: Add an explicit default ctor.

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/child/dwrite_f...
content/child/dwrite_font_proxy/dwrite_localized_strings_win.h:25:
~DWriteLocalizedStrings();
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Shouldn't this have override() since it's a subclass?

Done.

https://codereview.chromium.org/1438603002/diff/160001/content/common/dwrite_...
File content/common/dwrite_font_proxy_messages.h (right):

https://codereview.chromium.org/1438603002/diff/160001/content/common/dwrite_...
content/common/dwrite_font_proxy_messages.h:17: typedef
std::pair<base::string16, base::string16> StringPair;
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Nit: Please name this type more specifically, e.g. DWriteStringPair.
> 
> Otherwise, you're polluting the global namespace with this generic named type.

Done.

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

https://codereview.chromium.org/1438603002/diff/160001/content/common/font_wa...
content/common/font_warmup_win.cc:444: DCHECK(patched == 0);
On 2015/11/25 20:17:45, Alexei Svitkine (slow) wrote:
> Nit: DCHECK_EQ; fix throughout.

Will do, in a followup change.

[Alexei Svitkine (slow), 2015-12-03 18:50:23]

lgtm

[Ilya Kulshin, 2015-12-08 19:04:11]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2015-12-08 19:04:12]

The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org,
scottmg@chromium.org, jam@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1438603002/#ps220001
(title: "Fix compile error")

[Ilya Kulshin, 2015-12-08 19:05:19]

The CQ bit was unchecked by kulshin@chromium.org

[Ilya Kulshin, 2015-12-08 19:05:31]

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

[commit-bot: I haz the power, 2015-12-08 19:07:10]

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

[Ilya Kulshin, 2015-12-08 19:13:23]

The CQ bit was unchecked by kulshin@chromium.org

[Ilya Kulshin, 2015-12-08 19:13:58]

Can I get a new set of LGTMs, since I made a change since the last one? Thanks!

[jschuh, 2015-12-08 21:01:45]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2015-12-08 21:04:56]

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

[Alexei Svitkine (slow), 2015-12-08 21:11:13]

lgtm

[commit-bot: I haz the power, 2015-12-08 21:42:07]

Description was changed from

==========
Create direct write font proxy classes and unit tests.

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

to

==========
Create direct write font proxy classes and unit tests.

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

[commit-bot: I haz the power, 2015-12-08 21:42:12]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-12-08 21:43:09]

Description was changed from

==========
Create direct write font proxy classes and unit tests.

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

to

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}
==========

[commit-bot: I haz the power, 2015-12-08 21:43:10]

Patchset 12 (id:??) landed as
https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

[Nico, 2015-12-08 22:28:19]

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

[Nico, 2015-12-08 22:28:20]

clang complains:

In file included from
..\..\content\browser\renderer_host\dwrite_font_proxy_message_filter_win.cc:5:
..\..\content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h(30,3)
:  error: [chromium-style] Classes that are ref-counted should have destructors
that are declared protected or private.
  ~DWriteFontProxyMessageFilter() override;
  ^
..\..\content/browser/renderer_host/dwrite_font_proxy_message_filter_win.h(27,7)
:  note: [chromium-style] 'DWriteFontProxyMessageFilter' inherits from
'content::BrowserMessageFilter' here
    : public BrowserMessageFilter {
      ^
..\..\content/public/browser/browser_message_filter.h(32,7) :  note:
[chromium-style] 'BrowserMessageFilter' inherits from
'base::RefCountedThreadSafe<BrowserMessageFilter, BrowserMessageFilterTraits>'
here
    : public base::RefCountedThreadSafe<
      ^
1 error generated.


Please take a look.

[Ilya Kulshin, 2015-12-09 21:56:34]

Description was changed from

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}
==========

to

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}
==========

[Ilya Kulshin, 2015-12-10 00:41:59]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2015-12-10 00:41:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org,
jam@chromium.org, scottmg@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1438603002/#ps260001
(title: "More compile fixes")

[commit-bot: I haz the power, 2015-12-10 00:43:39]

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

[dcheng, 2015-12-10 01:05:28]

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

[dcheng, 2015-12-10 01:05:29]

A couple more clang warnings that would be nice to fix before this lands

https://codereview.chromium.org/1438603002/diff/260001/content/test/dwrite_fo...
File content/test/dwrite_font_fake_sender_win.h (right):

https://codereview.chromium.org/1438603002/diff/260001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:30: explicit FakeFont(const
base::string16& name) : font_name_(name) {}
..\..\content/test/dwrite_font_fake_sender_win.h(30,3) :  error:
[chromium-style] Complex constructor has an inlined body.
  explicit FakeFont(const base::string16& name) : font_name_(name) {}

https://codereview.chromium.org/1438603002/diff/260001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:76: FakeFontCollection() = default;
..\..\content/test/dwrite_font_fake_sender_win.h(76,3) :  error:
[chromium-style] Complex constructor has an inlined body.
  FakeFontCollection() = default;

https://codereview.chromium.org/1438603002/diff/260001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:96: ~ReplySender();
..\..\content/test/dwrite_font_fake_sender_win.h(96,19) :  error:
[chromium-style] Overriding method must be marked with 'override' or 'final'.
    ~ReplySender();
                  ^
                   override
..\..\content/test/dwrite_font_fake_sender_win.h(123,5) :  error:
[chromium-style] Complex constructor has an inlined body.
    FakeSender(FakeFontCollection* collection, bool track_messages)

https://codereview.chromium.org/1438603002/diff/260001/content/test/dwrite_fo...
content/test/dwrite_font_fake_sender_win.h:124: : collection_(collection),
track_messages_(track_messages) {}
..\..\content/test/dwrite_font_fake_sender_win.h(124,11) :  error: field 'collec
tion_' will be initialized after field 'track_messages_' [-Werror,-Wreorder]
        : collection_(collection), track_messages_(track_messages) {}

[Nico, 2015-12-10 01:06:13]

The CQ bit was unchecked by thakis@chromium.org

[Ilya Kulshin, 2015-12-10 19:34:16]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2015-12-10 19:34:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org,
jam@chromium.org, scottmg@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1438603002/#ps280001
(title: " ")

[commit-bot: I haz the power, 2015-12-10 19:39:29]

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

[commit-bot: I haz the power, 2015-12-10 19:56:05]

Description was changed from

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}
==========

to

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}
==========

[commit-bot: I haz the power, 2015-12-10 19:56:07]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-12-10 19:57:22]

Description was changed from

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}
==========

to

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

Committed: https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}
==========

[commit-bot: I haz the power, 2015-12-10 19:57:24]

Patchset 15 (id:??) landed as
https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}

[Marijn Kruisselbrink, 2015-12-10 21:47:55]

I reverted this (for the second time) in
https://codereview.chromium.org/1514183002. Please make sure you actually fix
the unit tests before landing it again... (first revert was
https://codereview.chromium.org/1507063004)

[Ilya Kulshin, 2015-12-10 23:32:30]

Description was changed from

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

Committed: https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}
==========

to

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

Committed: https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}
==========

[Ilya Kulshin, 2015-12-11 21:11:41]

Merge to head

[Ilya Kulshin, 2015-12-14 22:13:24]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2015-12-14 22:13:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org,
jam@chromium.org, scottmg@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1438603002/#ps340001
(title: "Merge to head")

[commit-bot: I haz the power, 2015-12-14 22:15:20]

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

[commit-bot: I haz the power, 2015-12-14 23:13:02]

Description was changed from

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

Committed: https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}
==========

to

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

Committed: https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}
==========

[commit-bot: I haz the power, 2015-12-14 23:13:03]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-12-14 23:14:29]

Description was changed from

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

Committed: https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}
==========

to

==========
Create direct write font proxy classes and unit tests.

BUG=525142

Committed: https://crrev.com/42290cb91a66566637a1715737b1961a8480b934
Cr-Commit-Position: refs/heads/master@{#363785}

Committed: https://crrev.com/772bb2e55ce932909dc9ac934eabb4c47dd638d4
Cr-Commit-Position: refs/heads/master@{#364445}

Committed: https://crrev.com/4039fd5b32b74b01866ca4c1ce73998f19320de1
Cr-Commit-Position: refs/heads/master@{#365122}
==========

[commit-bot: I haz the power, 2015-12-14 23:14:30]

Patchset 18 (id:??) landed as
https://crrev.com/4039fd5b32b74b01866ca4c1ce73998f19320de1
Cr-Commit-Position: refs/heads/master@{#365122}