Implement API for accessing fonts installed locally on the system.

Implement API for accessing fonts installed locally on the system.

* Allows for the querying of a list of installed fonts on the system and
  returning it using existing CSS Font Face types.
* Allows developers to load the binary data from local font faces for usage with
  JS font rendering libraries, such as OpenType.js or using Emscripten compiled
  Harfbuzz.

The API is guarded under the Experimental Framework, which is currently not
enabled. For more information about this, see
https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTtmg/edit.
This API will be inaccessible until the Experimental Framework launches.

A future patch that relies on these changes will add OS specific implementations
for OS X, Windows, and Linux.

Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/G-hC66MRTso

BUG=535764

[Daniel Nishi, 2016-01-21 20:24:53]

Description was changed from

==========
Implement API for accessing fonts installed locally on the system.

* Allows for the querying of a list of installed fonts on the system and
returning it using existing CSS Font Face types.
* Allows developers to load the binary data from local font faces for usage with
JS font rendering libraries, such as OpenType.js or using Emscripten compiled
Harfbuzz.

The API is guarded under the Experimental Framework, which is currently not
enabled as a Runtime Feature. For more information about this, see
https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt....

A future patch that relies on these changes will add OS specific implementations
for OS X, Windows, and Linux.

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

to

==========
Implement API for accessing fonts installed locally on the system.

* Allows for the querying of a list of installed fonts on the system and
returning it using existing CSS Font Face types.
* Allows developers to load the binary data from local font faces for usage with
JS font rendering libraries, such as OpenType.js or using Emscripten compiled
Harfbuzz.

The API is guarded under the Experimental Framework, which is currently not
enabled. For more information about this, see
https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt....
This API will be inaccessible until the Experimental Framework launches.

A future patch that relies on these changes will add OS specific implementations
for OS X, Windows, and Linux.

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/G-hC66MRTso

BUG=535764
==========

[Daniel Nishi, 2016-01-21 20:41:50]

Patchset #1 (id:1) has been deleted

[Daniel Nishi, 2016-01-21 20:43:12]

dhnishi@chromium.org changed reviewers:
+ esprehn@chromium.org, mlamouri@chromium.org, nick@chromium.org

[Daniel Nishi, 2016-01-21 20:43:12]

nick@chromium.org: Please review changes in content/

mlamouri@chromium.org: Please review changes in  
third_party/WebKit/Source/modules/

esprehn@chromium.org: Please review changes in third_party/WebKit/Source/core/,
third_party/WebKit/public/platform/, and 
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

Note: This API is inaccessible and, once the Experimental Framework launches,
will eventually expire.

[mlamouri (slow - plz ping), 2016-01-26 17:17:12]

I had a quick look and left some comments but I think your CL is way too large
and touches too many different places. You might want to split it in different
parts to help reviewers.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.cc:97: const std::string
FontAccessServiceImpl::FindPathInCache(
should this method be const?

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_access_service_impl.h (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:44: };
nit: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:58: const std::string
style);
const std::string&?

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:62: const std::string
path,
ditto

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:83: };
DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:84: }
nit: leave a blank line before and add `  // namespace content`

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_getter.cc (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_getter.cc:9: }
style: `  // namespace content`

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_getter.h:14: struct FontInformation {
nit: empty line

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_getter.h:20: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_getter_default.cc (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_getter_default.cc:12: }
ditto

https://codereview.chromium.org/1615133002/diff/20001/content/renderer/font_a...
File content/renderer/font_access/web_font_access_impl.h (right):

https://codereview.chromium.org/1615133002/diff/20001/content/renderer/font_a...
content/renderer/font_access/web_font_access_impl.h:20: explicit
WebFontAccessImpl();
no need for `explicit`

https://codereview.chromium.org/1615133002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/FontFaceSet.idl (right):

https://codereview.chromium.org/1615133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFaceSet.idl:64: [CallWith=ScriptState,
RuntimeEnabled=FontAccess, APIExperimentEnabled=FontAccess] static readonly
attribute Promise systemFonts;
Is there some doc to link to with the proposal?

[Daniel Nishi, 2016-01-26 18:36:10]

Thanks, Mounir!

I can split this along the Blink/Chrome division, if that makes sense.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.cc:97: const std::string
FontAccessServiceImpl::FindPathInCache(
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> should this method be const?

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_access_service_impl.h (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:44: };
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> nit: DISALLOW_COPY_AND_ASSIGN?

I think I use the implicit move, so the macro will cause a compile error.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:58: const std::string
style);
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> const std::string&?

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:62: const std::string
path,
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:83: };
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.h:84: }
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> nit: leave a blank line before and add `  // namespace content`

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_getter.cc (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_getter.cc:9: }
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> style: `  // namespace content`

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_getter.h:14: struct FontInformation {
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> nit: empty line

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_getter.h:20: };
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
File content/browser/font_access/font_getter_default.cc (right):

https://codereview.chromium.org/1615133002/diff/20001/content/browser/font_ac...
content/browser/font_access/font_getter_default.cc:12: }
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1615133002/diff/20001/content/renderer/font_a...
File content/renderer/font_access/web_font_access_impl.h (right):

https://codereview.chromium.org/1615133002/diff/20001/content/renderer/font_a...
content/renderer/font_access/web_font_access_impl.h:20: explicit
WebFontAccessImpl();
On 2016/01/26 17:17:11, Mounir Lamouri wrote:
> no need for `explicit`

Done.

https://codereview.chromium.org/1615133002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/FontFaceSet.idl (right):

https://codereview.chromium.org/1615133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFaceSet.idl:64: [CallWith=ScriptState,
RuntimeEnabled=FontAccess, APIExperimentEnabled=FontAccess] static readonly
attribute Promise systemFonts;
On 2016/01/26 17:17:12, Mounir Lamouri wrote:
> Is there some doc to link to with the proposal?

High level design doc:
https://docs.google.com/document/d/19Om0e3TBXk6i3Hb-Q-c--RT2y8VpcPzgyIbOyNNgC...

Decision and discussion about API format:
https://docs.google.com/document/d/1N3gr5UwPxOr7u9vXHh7R0UNW0rsa6QAkcmMMAodHj...

[esprehn, 2016-01-26 20:08:17]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org

[esprehn, 2016-01-26 20:08:18]

I think you want much more of this code in blink's platform, and very little
code out in content/. You want to issue the IPCs from inside platform, and avoid
doing so much string and data type conversion.

https://codereview.chromium.org/1615133002/diff/40001/content/renderer/font_a...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/40001/content/renderer/font_a...
content/renderer/font_access/web_font_access_impl.cc:48:
base::ASCIIToUTF16(fonts[i]->family.get()),
All this code belongs in blink, please stop doing so many string conversions.

[esprehn, 2016-01-26 20:26:45]

I think you need some tests that load a hundred local fonts, right now this
looks like a massive footgun that makes the browser process use tons of memory.

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.cc:85: mojo::SharedBuffer
buffer(1);
this seems like a mojo bug, it should support empty buffers.

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.cc:105: if (iter !=
font_cache_map_->end()) {
this cache grows unbounded which is bad, if you list all the fonts, then touch
binaryData for each one you permanently increase the memory of the browser
process, and this doesn't appear hooked up to any memory pressure signals. Also
fonts can be quite large, so doesn't that mean on a system with a lot of fonts I
can make the browser process consume hundreds of megabytes of fonts?

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
content/browser/font_access/font_getter.h:28: typedef std::map<const FontFamily,
FontStyleMap> FontCacheMap;
hash_map, why do you need the sorting? Also doesn't this increase the browser
process memory footprint by the full filename path of every font on your system?

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:550: void
onSuccess(WebPassOwnPtr<WebVector<WebFontAccessDescription*>> description)
override
who own's the description objects? Why isn't this a Vector of OwnPtr?

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:554:
blink::FontFaceDescriptors faceDesc;
don't abbreviate

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/FontFaceSet.idl (right):

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFaceSet.idl:40: Exposed=(Window,Worker),
This is a web exposed change adding this interface to the window, it also
exposes the FontFaceSet object to workers which doesn't seem safe, especially
with SetWrapperReferenceFrom=document, how does that work?

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fontaccess/FontAccessDescription.h
(right):

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fontaccess/FontAccessDescription.h:14: class
FontAccessDescription
Where is this class used?

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fontaccess/FontAccessDescription.h:38:
OwnPtr<WebFontAccessDescription> m_descriptor;
This should just be two String family and style, no reason to store the whole
descriptor. This also doesn't need to be garbage collected right?

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fontaccess/FontAccessDescription.idl
(right):

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fontaccess/FontAccessDescription.idl:29: ]
interface FontAccessDescription {
what creates these?

[esprehn, 2016-01-26 20:30:52]

Oh I see you're just storing names and paths, that still means I can touch the
binaryData property of a hundred fonts in order and cause the IPCs to queue a
hundred font blobs to read in the renderer? Does it throttle them?

At least the persistent memory is just a map of string -> path, that means on a
system with Photoshop's 400 extra fonts it might be a bunch of memory, but maybe
150kb so it's not awful, but still seems not optimal.

[Daniel Nishi, 2016-01-26 21:01:46]

On 2016/01/26 20:30:52, esprehn wrote:
> Oh I see you're just storing names and paths, that still means I can touch the
> binaryData property of a hundred fonts in order and cause the IPCs to queue a
> hundred font blobs to read in the renderer? Does it throttle them?
> 
> At least the persistent memory is just a map of string -> path, that means on
a
> system with Photoshop's 400 extra fonts it might be a bunch of memory, but
maybe
> 150kb so it's not awful, but still seems not optimal.

Just touching the property would return a Promise to you. Iterating over every
font in my system and resolving the promises appears to spike memory to 450MB
for the tab and use all of a CPU core for about a second (for the amount of
fonts on my workstation). The memory quickly drops once the ArrayBuffers holding
the font binaries are garbage collected.

I was able to cause an issue when I tried doing this in an infinite loop. Memory
spiked to 5.8GB for the tab and, after a few minutes, the MessagePipeDispatcher
received a bad message and crashed.

Currently, there are no throttling mechanisms. I could look into throttling the
Mojo IPC requests to stop a malicious use of the API.

[Daniel Nishi, 2016-01-26 23:49:27]

Thanks again, Elliot!

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.cc:85: mojo::SharedBuffer
buffer(1);
On 2016/01/26 20:26:44, esprehn wrote:
> this seems like a mojo bug, it should support empty buffers.

After rebasing, it appears this bug is fixed.

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
content/browser/font_access/font_getter.h:28: typedef std::map<const FontFamily,
FontStyleMap> FontCacheMap;
On 2016/01/26 20:26:45, esprehn wrote:
> hash_map, why do you need the sorting? Also doesn't this increase the browser
> process memory footprint by the full filename path of every font on your
system?

The map is arranged so that we can quickly lookup the correct path for a given
font.

It does increase the memory footprint by the path of every font, once the cache
has been populated. 

I suppose we could store the font path on the LocalFontFace object itself to
avoid the caching. As long as we're okay with potential duplication of paths
there, we can avoid persisting the information, I think. Would that be an
acceptable solution?

https://codereview.chromium.org/1615133002/diff/40001/content/renderer/font_a...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/40001/content/renderer/font_a...
content/renderer/font_access/web_font_access_impl.cc:48:
base::ASCIIToUTF16(fonts[i]->family.get()),
On 2016/01/26 20:08:18, esprehn wrote:
> All this code belongs in blink, please stop doing so many string conversions.

In terms of moving the code from content to Blink, would this be an
implementation like the Geolocation Mojo-to-blink prototype patch?
(https://codereview.chromium.org/1367853002)

If so, I think there are a lot of open questions regarding Mojo-to-Blink interop
that haven't been addressed yet.

If I'm misunderstanding, please let me know.

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:550: void
onSuccess(WebPassOwnPtr<WebVector<WebFontAccessDescription*>> description)
override
On 2016/01/26 20:26:45, esprehn wrote:
> who own's the description objects? Why isn't this a Vector of OwnPtr?

I think the WebVector owns the Description objects. Looking over it, I'm not
100% sure that is true, so I've changed it to be a vector of Descriptions,
rather than pointers to descriptions.

It isn't a Vector of OwnPtr because this occurs over the Content/Blink boundary.

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:554:
blink::FontFaceDescriptors faceDesc;
On 2016/01/26 20:26:45, esprehn wrote:
> don't abbreviate

Done.

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/FontFaceSet.idl (right):

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFaceSet.idl:40: Exposed=(Window,Worker),
On 2016/01/26 20:26:45, esprehn wrote:
> This is a web exposed change adding this interface to the window, it also
> exposes the FontFaceSet object to workers which doesn't seem safe, especially
> with SetWrapperReferenceFrom=document, how does that work?

Good point. It doesn't make sense from a Worker.

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fontaccess/FontAccessDescription.idl
(right):

https://codereview.chromium.org/1615133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fontaccess/FontAccessDescription.idl:29: ]
interface FontAccessDescription {
On 2016/01/26 20:26:45, esprehn wrote:
> what creates these?

This is a relic from a previous implementation that I forgot to remove. I'm
removing this module.

[dcheng, 2016-01-27 02:46:26]

Drive-by.

https://codereview.chromium.org/1615133002/diff/60001/content/common/font_acc...
File content/common/font_access/font_access_service.mojom (right):

https://codereview.chromium.org/1615133002/diff/60001/content/common/font_acc...
content/common/font_access/font_access_service.mojom:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Please add an OWNERS file with rules like the ones here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/OWN...

[Daniel Nishi, 2016-01-28 23:33:00]

https://codereview.chromium.org/1615133002/diff/60001/content/common/font_acc...
File content/common/font_access/font_access_service.mojom (right):

https://codereview.chromium.org/1615133002/diff/60001/content/common/font_acc...
content/common/font_access/font_access_service.mojom:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/01/27 02:46:26, dcheng wrote:
> Please add an OWNERS file with rules like the ones here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/OWN...

Done.

[esprehn, 2016-01-31 00:54:46]

Still looking this over...

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.cc:155:
mojo::MapBuffer(buffer.handle.get(), 0, size, &data,
what happens if the file doesn't exist here?

https://codereview.chromium.org/1615133002/diff/80001/content/renderer/font_a...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/80001/content/renderer/font_a...
content/renderer/font_access/web_font_access_impl.cc:48:
base::ASCIIToUTF16(fonts[i]->family.get()),
you want to use WebString::fromLatin1 to avoid the string copies here.

[Daniel Nishi, 2016-02-02 17:54:18]

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/40001/content/browser/font_ac...
content/browser/font_access/font_access_service_impl.cc:155:
mojo::MapBuffer(buffer.handle.get(), 0, size, &data,
On 2016/01/31 00:54:46, esprehn wrote:
> what happens if the file doesn't exist here?

It failed silently and returned a big empty buffer before.

I've changed it to fail normally, like if the font specified wasn't listed.

https://codereview.chromium.org/1615133002/diff/80001/content/renderer/font_a...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/80001/content/renderer/font_a...
content/renderer/font_access/web_font_access_impl.cc:48:
base::ASCIIToUTF16(fonts[i]->family.get()),
On 2016/01/31 00:54:46, esprehn wrote:
> you want to use WebString::fromLatin1 to avoid the string copies here.

Done.

[Daniel Nishi, 2016-02-05 17:26:18]

Ping.

[ncarter (slow), 2016-02-09 18:59:44]

nick@chromium.org changed reviewers:
+ nasko@chromium.org
- nick@chromium.org

[ncarter (slow), 2016-02-09 18:59:45]

nasko@ should be a better reviewer for this kind of change. I'm leaving a couple
comments.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:59:
FontCacheMap::iterator iter;
Why not use a range based for loop?

for (auto& entry : font_cache_map_) {

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:60:
mojo::Array<FontDescriptionPtr> fontInfos(0);
font_infos

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:63:
FontStyleMap::iterator faceIter;
face_iter (or just use a range loop)

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:66: FontDescriptionPtr
fontPtr =
font_ptr

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

https://codereview.chromium.org/1615133002/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1074:
base::Bind(&FontAccessServiceImpl::Create));
I'd like to see some browser tests (or layout tests) that exercise the
content/browser part of this change.

[esprehn, 2016-02-09 19:29:57]

Please also wrap your change description sensibly around 80 or 100 cols.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:130: const std::string
path) {
path& ?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:149: const std::string
path,
path& ?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:18: : path(path), fullName(fullName)
{}
full_name ?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:21: std::string fullName;
full_name ?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:27: typedef std::map<const FontFamily,
FontStyleMap> FontCacheMap;
why not hash_map for both?

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:16: using blink::WebVector;
using blink::WebString and cut some verbosity below. :)

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.h (right):

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:17: class CONTENT_EXPORT
WebFontAccessImpl
final ?

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:25: const blink::WebString&,
add argument names, what are the two strings?

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFace.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFace.cpp:648: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
we really should support this for web fonts...

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:542: class
FontAccessDescriptionCallback : public FontAccessCallbacks {
final

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/LocalFontFace.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.cpp:29:
RefPtrWillBeRawPtr<CSSValue> src =
CSSParser::parseFontFaceDescriptor(CSSPropertySrc, sourceString, parserContext);
This is copy pasta from FontFace, we probably want to figure out how to share it
someday.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.cpp:31:
fontFace->setError(DOMException::create(SyntaxError, "The source provided ('" +
sourceString + "') could not be parsed as a value list."));
do you have a test for this case? How does it happen?

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/LocalFontFace.h (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:21: static
PassRefPtrWillBeRawPtr<LocalFontFace> createLocalFontFace(ExecutionContext*,
const AtomicString& family, const AtomicString& styleName, const AtomicString&
fullName, StringOrArrayBufferOrArrayBufferView&, const FontFaceDescriptors&);
create(), no need for the type name in the method name.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:29: };
remove ;

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:34: };
ditto

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/font_access/WebFontAccess.h
(right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/font_access/WebFontAccess.h:26:
virtual void getFontWebData(const WebString&, const WebString&,
FontDataCallbacks*) = 0;
add argument names for strings

[esprehn, 2016-02-09 19:29:57]

Please also wrap your change description sensibly around 80 or 100 cols.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:130: const std::string
path) {
path& ?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:149: const std::string
path,
path& ?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:18: : path(path), fullName(fullName)
{}
full_name ?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:21: std::string fullName;
full_name ?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:27: typedef std::map<const FontFamily,
FontStyleMap> FontCacheMap;
why not hash_map for both?

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:16: using blink::WebVector;
using blink::WebString and cut some verbosity below. :)

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.h (right):

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:17: class CONTENT_EXPORT
WebFontAccessImpl
final ?

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:25: const blink::WebString&,
add argument names, what are the two strings?

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFace.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFace.cpp:648: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
we really should support this for web fonts...

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:542: class
FontAccessDescriptionCallback : public FontAccessCallbacks {
final

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/LocalFontFace.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.cpp:29:
RefPtrWillBeRawPtr<CSSValue> src =
CSSParser::parseFontFaceDescriptor(CSSPropertySrc, sourceString, parserContext);
This is copy pasta from FontFace, we probably want to figure out how to share it
someday.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.cpp:31:
fontFace->setError(DOMException::create(SyntaxError, "The source provided ('" +
sourceString + "') could not be parsed as a value list."));
do you have a test for this case? How does it happen?

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/LocalFontFace.h (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:21: static
PassRefPtrWillBeRawPtr<LocalFontFace> createLocalFontFace(ExecutionContext*,
const AtomicString& family, const AtomicString& styleName, const AtomicString&
fullName, StringOrArrayBufferOrArrayBufferView&, const FontFaceDescriptors&);
create(), no need for the type name in the method name.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:29: };
remove ;

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:34: };
ditto

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/font_access/WebFontAccess.h
(right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/font_access/WebFontAccess.h:26:
virtual void getFontWebData(const WebString&, const WebString&,
FontDataCallbacks*) = 0;
add argument names for strings

[nasko, 2016-02-10 23:43:26]

I have just started poking at this and only looked at the mojom files to get a
sense of what the CL nature is.

What worries me in general is this statement:

"Iterating over every font in my system and resolving the promises appears to
spike memory to 450MB for the tab and use all of a CPU core for about a second
(for the amount of fonts on my workstation)."

This sounds awefully expensive in terms of performance. Can this be optimized
such that it is more like an iterator or stream, such that it doesn't consume a
huge blob of memory?

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_access_service_impl.h (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.h:25: // Returns a list of
FontAccessDescriptions on the callback.
nit: Technically, it returns void. It sends that list to the
GetFontListCallback, right?
nit: s/on the callback/to the callback/

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.h:51: // Pulls the font
binary path from the cache and returns the font binary in a
nit: When talking about paths, one usually thinks of filesystem ones. I don't
see such a type here, only family and style. Maybe define somewhere in comment
what is meant by "font binary path", so it is clear to the reader.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.h:57: // Returns the font
binary path if it exists in the cache.
nit: Empty line before the comment.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.h:70: static BlobInfo
GetFileInfo(const std::string path);
sytle: static methods go first in ordering of the class methods.

https://codereview.chromium.org/1615133002/diff/100001/content/common/font_ac...
File content/common/font_access/font_access_service.mojom (right):

https://codereview.chromium.org/1615133002/diff/100001/content/common/font_ac...
content/common/font_access/font_access_service.mojom:11: GetFontData(string
family, string style) => (handle<shared_buffer> shared_state, int64 size);
style: line exceeds 80 characters.

[Daniel Nishi, 2016-02-10 23:54:55]

Description was changed from

==========
Implement API for accessing fonts installed locally on the system.

* Allows for the querying of a list of installed fonts on the system and
returning it using existing CSS Font Face types.
* Allows developers to load the binary data from local font faces for usage with
JS font rendering libraries, such as OpenType.js or using Emscripten compiled
Harfbuzz.

The API is guarded under the Experimental Framework, which is currently not
enabled. For more information about this, see
https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt....
This API will be inaccessible until the Experimental Framework launches.

A future patch that relies on these changes will add OS specific implementations
for OS X, Windows, and Linux.

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/G-hC66MRTso

BUG=535764
==========

to

==========
Implement API for accessing fonts installed locally on the system.

* Allows for the querying of a list of installed fonts on the system and
  returning it using existing CSS Font Face types.
* Allows developers to load the binary data from local font faces for usage with
  JS font rendering libraries, such as OpenType.js or using Emscripten compiled
  Harfbuzz.

The API is guarded under the Experimental Framework, which is currently not
enabled. For more information about this, see
https://docs.google.com/document/d/1qVP2CK1lbfmtIJRIm6nwuEFFhGhYbtThLQPo3CSTt....
This API will be inaccessible until the Experimental Framework launches.

A future patch that relies on these changes will add OS specific implementations
for OS X, Windows, and Linux.

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/G-hC66MRTso

BUG=535764
==========

[Daniel Nishi, 2016-02-11 00:23:51]

nasko@: For reference, that worst case scenario is unlikely to occur in
practice.

That said, my previous implementation had the Mojo service owning a Blob
handler, which kept references to the file Blobs in memory. The blobs allowed
deferral of loading the font into memory when the font blob was read in a
FileReader. We ended up ditching this because it seemed like the user of this
API would need to do this immediately afterwards in all practical use cases, so
it added needless complexity.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:59:
FontCacheMap::iterator iter;
On 2016/02/09 18:59:45, ncarter wrote:
> Why not use a range based for loop?
> 
> for (auto& entry : font_cache_map_) {

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:60:
mojo::Array<FontDescriptionPtr> fontInfos(0);
On 2016/02/09 18:59:45, ncarter wrote:
> font_infos

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:63:
FontStyleMap::iterator faceIter;
On 2016/02/09 18:59:45, ncarter wrote:
> face_iter (or just use a range loop)

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:66: FontDescriptionPtr
fontPtr =
On 2016/02/09 18:59:45, ncarter wrote:
> font_ptr

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:130: const std::string
path) {
On 2016/02/09 19:29:56, esprehn wrote:
> path& ?

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:149: const std::string
path,
On 2016/02/09 19:29:56, esprehn wrote:
> path& ?

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_access_service_impl.h (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.h:25: // Returns a list of
FontAccessDescriptions on the callback.
On 2016/02/10 23:43:26, nasko wrote:
> nit: Technically, it returns void. It sends that list to the
> GetFontListCallback, right?
> nit: s/on the callback/to the callback/

That is correct.

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.h:51: // Pulls the font
binary path from the cache and returns the font binary in a
On 2016/02/10 23:43:26, nasko wrote:
> nit: When talking about paths, one usually thinks of filesystem ones. I don't
> see such a type here, only family and style. Maybe define somewhere in comment
> what is meant by "font binary path", so it is clear to the reader.

It is referring to a file system one. We're caching a mapping of these font
specifiers to the font file system path.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.h:57: // Returns the font
binary path if it exists in the cache.
On 2016/02/10 23:43:26, nasko wrote:
> nit: Empty line before the comment.

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.h:70: static BlobInfo
GetFileInfo(const std::string path);
On 2016/02/10 23:43:26, nasko wrote:
> sytle: static methods go first in ordering of the class methods.

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:18: : path(path), fullName(fullName)
{}
On 2016/02/09 19:29:56, esprehn wrote:
> full_name ?

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:21: std::string fullName;
On 2016/02/09 19:29:56, esprehn wrote:
> full_name ?

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/browser/font_a...
content/browser/font_access/font_getter.h:27: typedef std::map<const FontFamily,
FontStyleMap> FontCacheMap;
On 2016/02/09 19:29:56, esprehn wrote:
> why not hash_map for both?

Given the lookup speed, hash_map does seem more appropriate.

Done.

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

https://codereview.chromium.org/1615133002/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1074:
base::Bind(&FontAccessServiceImpl::Create));
On 2016/02/09 18:59:45, ncarter wrote:
> I'd like to see some browser tests (or layout tests) that exercise the
> content/browser part of this change.

Which part are we looking to get under the test harness with a browser/layout
test? The content/browser portion is unit-tested, so are we trying to test the
service registry layer?

https://codereview.chromium.org/1615133002/diff/100001/content/common/font_ac...
File content/common/font_access/font_access_service.mojom (right):

https://codereview.chromium.org/1615133002/diff/100001/content/common/font_ac...
content/common/font_access/font_access_service.mojom:11: GetFontData(string
family, string style) => (handle<shared_buffer> shared_state, int64 size);
On 2016/02/10 23:43:26, nasko wrote:
> style: line exceeds 80 characters.

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:16: using blink::WebVector;
On 2016/02/09 19:29:56, esprehn wrote:
> using blink::WebString and cut some verbosity below. :)

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.h (right):

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:17: class CONTENT_EXPORT
WebFontAccessImpl
On 2016/02/09 19:29:56, esprehn wrote:
> final ?

Done.

https://codereview.chromium.org/1615133002/diff/100001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:25: const blink::WebString&,
On 2016/02/09 19:29:56, esprehn wrote:
> add argument names, what are the two strings?

Done.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFace.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFace.cpp:648: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
On 2016/02/09 19:29:56, esprehn wrote:
> we really should support this for web fonts...

I'll investigate this. For a first pass, it should be okay to limit to system
fonts, but I think there is a way to get the binary data out of Skia.

For now, I'll add a TODO.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:542: class
FontAccessDescriptionCallback : public FontAccessCallbacks {
On 2016/02/09 19:29:56, esprehn wrote:
> final

Done.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/LocalFontFace.cpp (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.cpp:29:
RefPtrWillBeRawPtr<CSSValue> src =
CSSParser::parseFontFaceDescriptor(CSSPropertySrc, sourceString, parserContext);
On 2016/02/09 19:29:56, esprehn wrote:
> This is copy pasta from FontFace, we probably want to figure out how to share
it
> someday.

Added a TODO to figure out how to share it.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.cpp:31:
fontFace->setError(DOMException::create(SyntaxError, "The source provided ('" +
sourceString + "') could not be parsed as a value list."));
On 2016/02/09 19:29:56, esprehn wrote:
> do you have a test for this case? How does it happen?

After looking it over, it shouldn't happen. This should always create a list of
a single local font source.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/LocalFontFace.h (right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:21: static
PassRefPtrWillBeRawPtr<LocalFontFace> createLocalFontFace(ExecutionContext*,
const AtomicString& family, const AtomicString& styleName, const AtomicString&
fullName, StringOrArrayBufferOrArrayBufferView&, const FontFaceDescriptors&);
On 2016/02/09 19:29:57, esprehn wrote:
> create(), no need for the type name in the method name.

Done.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:29: };
On 2016/02/09 19:29:57, esprehn wrote:
> remove ;

Done.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/LocalFontFace.h:34: };
On 2016/02/09 19:29:57, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/font_access/WebFontAccess.h
(right):

https://codereview.chromium.org/1615133002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/font_access/WebFontAccess.h:26:
virtual void getFontWebData(const WebString&, const WebString&,
FontDataCallbacks*) = 0;
On 2016/02/09 19:29:57, esprehn wrote:
> add argument names for strings

Done.

[nasko, 2016-02-12 22:05:16]

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

[nasko, 2016-02-12 22:05:20]

I have an overall question of why we are doing this at all? The proposal seemed
controversial when discussed on blink-dev and there wasn't a clear agreement
that we should be proceeding with this work. 
Why can't the fonts be loaded over the network by the provider of the software?
Allowing enumeration of locally installed fonts is a privacy problem.

That said, the CL can use a "git cl format" run as there are a bunch of
formatting issues. I didn't point them all out.

Passing the torch to jam@ for reviewing, as I will be out for a bit.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:4: #include
"content/browser/font_access/font_access_service_impl.h"
Empty line between the comment and the first include.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:15:
FontAccessServiceImpl::FontAccessServiceImpl(
Empty line after namespace and before the constructor.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:25:
ClobberCacheAndReply(base::Bind(&FontAccessServiceImpl::GetFontListFromCache,
What's the point of having a cache if every call to GetFontList will clobber it?

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:55:
FontAccessServiceImpl::BlobInfo::~BlobInfo() {}
Empty line between the two methods.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:61: for (auto& entry :
*font_cache_map_) {
font_cache_map_.get()

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:91: std::string
FontAccessServiceImpl::FindPathInCache(
Why is a method looking for a path returning a std::string? We should be using
FilePath if it is truly a filesystem path and only converting it to string if
needed.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:131: BlobInfo realInfo;
real_info

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:134: return realInfo;
style: Indent only two spaces. One line if statements don't need {}.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:156: return
mojo::ScopedHandleBase<mojo::SharedBufferHandle>();
style: Indent only two spaces. One line if statements don't need {}.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:159: 
No need for two empty lines, one is fine.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:168:
callback.Run(mojo::ScopedHandleBase<mojo::SharedBufferHandle>(), 0);
style: Indent only two spaces.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
File content/browser/font_access/font_getter.cc (right):

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter.cc:8: const char kFontAccessToken[] =
"_font_access_service_getter_";
Empty lines around this line.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
style: No "(c)".

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
File content/browser/font_access/font_getter_default.cc (right):

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter_default.cc:7: namespace content {
Empty line after namespace.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter_default.cc:10: return empty_map;
Why do you need a _default.cc file instead of folding this into the main
font_getter.cc?

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter_default.cc:12: }  // namespace content
Empty line above.

https://codereview.chromium.org/1615133002/diff/120001/content/common/font_ac...
File content/common/font_access/font_description.mojom (right):

https://codereview.chromium.org/1615133002/diff/120001/content/common/font_ac...
content/common/font_access/font_description.mojom:10: string fullName;
full_name

https://codereview.chromium.org/1615133002/diff/120001/content/content_browse...
File content/content_browser.gypi (right):

https://codereview.chromium.org/1615133002/diff/120001/content/content_browse...
content/content_browser.gypi:697:
'browser/font_access/font_access_service_impl.h',
Aphabetic ordering please.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:28: base::Unretained(this),
Why is it safe to pass base::Unretained(this)? Please explain in a comment.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:69: if (success !=
MOJO_RESULT_OK) {
style: Single line if statements don't need {}.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:72: blink::WebData* webData
= new blink::WebData(static_cast<char*>(data), size);
web_data

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.h (right):

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:5: #ifndef
CONTENT_FONT_ACCESS_WEB_FONT_ACCESS_IMPL_H_
Include guard is missing _RENDERER component from the path.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:13: 
No need for two empty lines, one is enough.

[Daniel Nishi, 2016-02-12 22:49:41]

Thanks again, everyone, for taking the time to review my patch! :)

Why the change is here: The fonts can't be loaded over network by the
application provider because these fonts are user supplied, and often the user
has specific usages of the font that they can use. The provider themselves may
provide a few fonts, but this allows for use of all of the fonts on the system.

By providing this system, we enable a new class of web apps for designers.
Although it would be great if we could provide the fonts purely through web
fonts, many font foundries have not gone down this path.

We have clients in mind to test out this functionality under the Experimental
Framework and will make a more educated assessment of if we want to proceed with
this once we have more feedback.

On the privacy issue: This feature will be guarded under a permission before
going out officially under EF.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
File content/browser/font_access/font_access_service_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:4: #include
"content/browser/font_access/font_access_service_impl.h"
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> Empty line between the comment and the first include.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:15:
FontAccessServiceImpl::FontAccessServiceImpl(
On 2016/02/12 22:05:17, nasko (away for February) wrote:
> Empty line after namespace and before the constructor.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:25:
ClobberCacheAndReply(base::Bind(&FontAccessServiceImpl::GetFontListFromCache,
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> What's the point of having a cache if every call to GetFontList will clobber
it?

The list itself may grow stale, but the cache allows us to quickly look up font
paths after the first call.

This saves 2-3 seconds per GetFontData call on certain OSes.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:55:
FontAccessServiceImpl::BlobInfo::~BlobInfo() {}
On 2016/02/12 22:05:17, nasko (away for February) wrote:
> Empty line between the two methods.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:61: for (auto& entry :
*font_cache_map_) {
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> font_cache_map_.get()

The operator* does a .get() and dereferences it. I need the object to do the
range based loop.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:91: std::string
FontAccessServiceImpl::FindPathInCache(
On 2016/02/12 22:05:17, nasko (away for February) wrote:
> Why is a method looking for a path returning a std::string? We should be using
> FilePath if it is truly a filesystem path and only converting it to string if
> needed.

base::FilePath is now used.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:131: BlobInfo realInfo;
On 2016/02/12 22:05:17, nasko (away for February) wrote:
> real_info

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:134: return realInfo;
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> style: Indent only two spaces. One line if statements don't need {}.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:156: return
mojo::ScopedHandleBase<mojo::SharedBufferHandle>();
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> style: Indent only two spaces. One line if statements don't need {}.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:159: 
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> No need for two empty lines, one is fine.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_access_service_impl.cc:168:
callback.Run(mojo::ScopedHandleBase<mojo::SharedBufferHandle>(), 0);
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> style: Indent only two spaces.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
File content/browser/font_access/font_getter.cc (right):

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter.cc:8: const char kFontAccessToken[] =
"_font_access_service_getter_";
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> Empty lines around this line.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
File content/browser/font_access/font_getter.h (right):

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> style: No "(c)".

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
File content/browser/font_access/font_getter_default.cc (right):

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter_default.cc:7: namespace content {
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> Empty line after namespace.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter_default.cc:10: return empty_map;
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> Why do you need a _default.cc file instead of folding this into the main
> font_getter.cc?


Other OS implementations will implement the GetFontCacheMap method and only the
correct implementation will be compiled for the OS (i.e. the default will not
be). The default implementation will only be compiled in for OSes which do not
have an implementation of the GetFontCacheMap method yet.

https://codereview.chromium.org/1615133002/diff/120001/content/browser/font_a...
content/browser/font_access/font_getter_default.cc:12: }  // namespace content
On 2016/02/12 22:05:18, nasko (away for February) wrote:
> Empty line above.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/common/font_ac...
File content/common/font_access/font_description.mojom (right):

https://codereview.chromium.org/1615133002/diff/120001/content/common/font_ac...
content/common/font_access/font_description.mojom:10: string fullName;
On 2016/02/12 22:05:19, nasko (away for February) wrote:
> full_name

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/content_browse...
File content/content_browser.gypi (right):

https://codereview.chromium.org/1615133002/diff/120001/content/content_browse...
content/content_browser.gypi:697:
'browser/font_access/font_access_service_impl.h',
On 2016/02/12 22:05:19, nasko (away for February) wrote:
> Aphabetic ordering please.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.cc (right):

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:28: base::Unretained(this),
On 2016/02/12 22:05:19, nasko (away for February) wrote:
> Why is it safe to pass base::Unretained(this)? Please explain in a comment.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:69: if (success !=
MOJO_RESULT_OK) {
On 2016/02/12 22:05:19, nasko (away for February) wrote:
> style: Single line if statements don't need {}.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.cc:72: blink::WebData* webData
= new blink::WebData(static_cast<char*>(data), size);
On 2016/02/12 22:05:19, nasko (away for February) wrote:
> web_data

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
File content/renderer/font_access/web_font_access_impl.h (right):

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:5: #ifndef
CONTENT_FONT_ACCESS_WEB_FONT_ACCESS_IMPL_H_
On 2016/02/12 22:05:19, nasko (away for February) wrote:
> Include guard is missing _RENDERER component from the path.

Done.

https://codereview.chromium.org/1615133002/diff/120001/content/renderer/font_...
content/renderer/font_access/web_font_access_impl.h:13: 
On 2016/02/12 22:05:19, nasko (away for February) wrote:
> No need for two empty lines, one is enough.

Done.

[Daniel Nishi, 2016-02-17 00:48:50]

dhnishi@chromium.org changed reviewers:
+ jamartin@google.com, palmer@chromium.org
- jam@chromium.org

[Daniel Nishi, 2016-02-17 00:48:51]

palmer@: PTAL at  content/common/font_access/*.mojom, when you get a chance.
Thanks!

[jam, 2016-02-17 16:54:28]

can this cl patch the code from https://codereview.chromium.org/1615133002/ to
call mojo from blink, and then remove the content/renderer part?

[eae, 2016-02-17 17:25:18]

I'm surprised we're going ahead with this given that none of the concerns bought
up in the intent to implement have been addressed. This adds a lot of complexity
for a feature that is very controversial and that we are unlikely ship in its
current form. 

If this is purely for an experimental client why not do the work on a branch?
That's the way we usually do proofi of concepts and prototype implementations.

NOT LGTM until these questions, and nasko's similar ones, have been answered.

[palmer, 2016-04-04 20:28:01]

Can we close this CL, or move forward with it?