Add Pepper TrueType font API plumbing.

Add Pepper TrueType font API plumbing.
Adds Font resource and host, font singleton resource and host.
Adds PPAPI IPC messages to communicate with renderer and browser.
Adds Browser process font listing helper class abstraction and impls for Windows, Mac, and Linux. Font
family enumeration is done in the browser process and is a rework of the trusted BrowserFont API.
Adds Renderer process font helper class abstraction and impl stubs. Font operations are done in the
renderer process because of Linux sandbox restrictions.


BUG=79375
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188576

[bbudge, 2013-03-11 22:53:18]

Brett for content plumbing and browser process rework of BrowserFont list stuff.
David for PPAPI plumbing.
Justin for IPC messages (reassign if appropriate.)

[dmichael (off chromium), 2013-03-12 18:07:58]

Looks good overall; mostly nits.

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:67:
}
It looks like this brace is unmatched? Or indentation is off?

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_list.h (right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list.h:17:
std::vector<std::string> GetFontFamilies_SlowBlocking();
Probably not a huge deal, assuming there aren't many of these and the call is
infrequent. But returning a vector of strings by value is kind of inefficient.
Optional: consider scoped_ptr or an out-parameter.

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_android.cc
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list_android.cc:10:
return std::vector<std::string>();
Should there be a TODO? Or a note that this isn't used on Android?

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:22: for
(int i = 0; i < num_families; i++) {
nit: Don't need curly braces here; inconsistent w.r.t. the loop on 29

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:30:
font_families.push_back(*iter);
vector has a begin/end constructor that should work...
std::vector<std::strsing> font_families(sorted_families.begin(),
sorted_families.end());

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_win.cc
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list_win.cc:48:
font_families.push_back(*iter);
ditto, could use vector constructor

(BTW, The Win API is greek to me. I'm assuming somebody else is reviewing or has
reviewed that)

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_singleton_host.h
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_singleton_host.h:15:
class PepperTrueTypeFontSingletonHost : public ppapi::host::ResourceHost {
This name's pretty long, and it looks like we don't use "Singleton" in the name
usually (even when they are instance-singleton resources). I'd drop that part.

https://codereview.chromium.org/12600019/diff/4001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_truetype_font.h (right):

https://codereview.chromium.org/12600019/diff/4001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_truetype_font.h:17: // Createa a font matching
the given descriptor. The exact font that is
Createa -> Create

https://codereview.chromium.org/12600019/diff/4001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/12600019/diff/4001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_truetype_font_linux.cc:12: NOTIMPLEMENTED();
Is this to be implemented in another CL?

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/serialized_str...
File ppapi/proxy/serialized_structs.cc (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/serialized_str...
ppapi/proxy/serialized_structs.cc:106: void
SerializedTrueTypeFontDesc::SetToPPTrueTypeFontDesc(
I assume anything that starts with "Set" is modifying this object. How about
something like "CopyToPPTrueTypeFontDesc"?

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/serialized_str...
File ppapi/proxy/serialized_structs.h (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/serialized_str...
ppapi/proxy/serialized_structs.h:70: // Converts to a PP_FontDescription_Dev.
The face name will have one ref
s/The face name/|desc.family|
?

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
File ppapi/proxy/truetype_font_resource.cc (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
ppapi/proxy/truetype_font_resource.cc:30: const PP_TrueTypeFontDesc_Dev* desc)
Why is this const* and not const&?

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
ppapi/proxy/truetype_font_resource.cc:51: callback, desc));
This is a little bit unusual. I think it will work, but I'd be a bit cautious...
by binding TrackedCallback in to the base::Callback, you're opening up the
possibility that the TrackedCallback outlives its Resource. This should be okay;
if the Resource is destroyed, TrackedCallback will be Aborted and should know
not to fire again.

The usual pattern is to store your callback and the desc inside this object, and
return PP_ERROR_INPROGRESS if the caller tries to Describe while one is pending.

On the bright side, this means you can support multiple "Describe" calls in
flight. I think I like the way you're doing it, but I just wanted you to be
aware that it's different than usual and therefore might hit some latent bugs.

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
File ppapi/proxy/truetype_font_singleton_resource.h (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
ppapi/proxy/truetype_font_singleton_resource.h:25: class
TrueTypeFontSingletonResource
s/Singleton//

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
ppapi/proxy/truetype_font_singleton_resource.h:39: PP_CompletionCallback
callback) OVERRIDE;
Should be scoped_refptr<TrackedCallback> (optionally const&)

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
File ppapi/thunk/ppb_truetype_font_api.h (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
ppapi/thunk/ppb_truetype_font_api.h:18: class PPAPI_THUNK_EXPORT
PPB_TrueTypeFont_API {
Note Justin TerAvest has been trying to make thunks automatically generated.
There's an attribute you can add to your
IDL:https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file_system.idl&q=generate_thunk&sq=package:chromium&type=cs&l=10

If it works for you at all, it will put GetFontFamilies in to 1 API with these.
Is there any chance that's feasible? Or maybe you can talk to Justin about ways
to make the thunk generator work for your stuff?

If it's too hard, I'm okay with it never being generated. But it's worth looking
in to briefly. Possibly for a future CL if you change anything at all.

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
File ppapi/thunk/ppb_truetype_font_thunk.cc (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
ppapi/thunk/ppb_truetype_font_thunk.cc:24:
EnterInstanceAPI<PPB_TrueTypeFont_Singleton_API> enter(instance);
You need to pass the callback here in order to get proper callback behavior.

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
ppapi/thunk/ppb_truetype_font_thunk.cc:27: return
enter.functions()->GetFontFamilies(instance, output, callback);
And this should be enter.callback()

[bbudge, 2013-03-13 01:26:50]

Addressing David's comments. Ignore ppapi/thunk/enter.* files. These changes are
in another CL but my feeble git skills can't get rid of them here.

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:67:
}
Fixed indentation and added braces to 'case' to improve readability.
On 2013/03/12 18:07:58, dmichael wrote:
> It looks like this brace is unmatched? Or indentation is off?

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_list.h (right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list.h:17:
std::vector<std::string> GetFontFamilies_SlowBlocking();
This is the Chromium project. We do things right. Made this an 'out' param.
On 2013/03/12 18:07:58, dmichael wrote:
> Probably not a huge deal, assuming there aren't many of these and the call is
> infrequent. But returning a vector of strings by value is kind of inefficient.
> Optional: consider scoped_ptr or an out-parameter.

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_android.cc
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list_android.cc:10:
return std::vector<std::string>();
On 2013/03/12 18:07:58, dmichael wrote:
> Should there be a TODO? Or a note that this isn't used on Android?

Done.

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:22: for
(int i = 0; i < num_families; i++) {
On 2013/03/12 18:07:58, dmichael wrote:
> nit: Don't need curly braces here; inconsistent w.r.t. the loop on 29

Done.

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:30:
font_families.push_back(*iter);
This is now an 'out' param rather than return value. I'll remember that ctor for
next time.
On 2013/03/12 18:07:58, dmichael wrote:
> vector has a begin/end constructor that should work...
> std::vector<std::strsing> font_families(sorted_families.begin(),
> sorted_families.end());

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_win.cc
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_list_win.cc:48:
font_families.push_back(*iter);
This is copy/paste/tweak from Brett's font_list stuff.
On 2013/03/12 18:07:58, dmichael wrote:
> ditto, could use vector constructor
> 
> (BTW, The Win API is greek to me. I'm assuming somebody else is reviewing or
has
> reviewed that)

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_singleton_host.h
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_singleton_host.h:15:
class PepperTrueTypeFontSingletonHost : public ppapi::host::ResourceHost {
This follows the precedent of browser_font_singleton_host. Also, there's a
PepperTrueTypeFontHost on the renderer side.
On 2013/03/12 18:07:58, dmichael wrote:
> This name's pretty long, and it looks like we don't use "Singleton" in the
name
> usually (even when they are instance-singleton resources). I'd drop that part.

https://codereview.chromium.org/12600019/diff/4001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_truetype_font.h (right):

https://codereview.chromium.org/12600019/diff/4001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_truetype_font.h:17: // Createa a font matching
the given descriptor. The exact font that is
On 2013/03/12 18:07:58, dmichael wrote:
> Createa -> Create

Done.

https://codereview.chromium.org/12600019/diff/4001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/12600019/diff/4001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_truetype_font_linux.cc:12: NOTIMPLEMENTED();
Yep. There is a fair bit of code for each platform, and this makes the CL far
more reviewable.
On 2013/03/12 18:07:58, dmichael wrote:
> Is this to be implemented in another CL?

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/serialized_str...
File ppapi/proxy/serialized_structs.cc (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/serialized_str...
ppapi/proxy/serialized_structs.cc:106: void
SerializedTrueTypeFontDesc::SetToPPTrueTypeFontDesc(
On 2013/03/12 18:07:58, dmichael wrote:
> I assume anything that starts with "Set" is modifying this object. How about
> something like "CopyToPPTrueTypeFontDesc"?

Done.

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/serialized_str...
File ppapi/proxy/serialized_structs.h (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/serialized_str...
ppapi/proxy/serialized_structs.h:70: // Converts to a PP_FontDescription_Dev.
The face name will have one ref
On 2013/03/12 18:07:58, dmichael wrote:
> s/The face name/|desc.family|
> ?

Done. Reworked the comments.

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
File ppapi/proxy/truetype_font_resource.cc (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
ppapi/proxy/truetype_font_resource.cc:30: const PP_TrueTypeFontDesc_Dev* desc)
On 2013/03/12 18:07:58, dmichael wrote:
> Why is this const* and not const&?

Done.

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
ppapi/proxy/truetype_font_resource.cc:51: callback, desc));
Noted. Thanks for the heads up.
On 2013/03/12 18:07:58, dmichael wrote:
> This is a little bit unusual. I think it will work, but I'd be a bit
cautious...
> by binding TrackedCallback in to the base::Callback, you're opening up the
> possibility that the TrackedCallback outlives its Resource. This should be
okay;
> if the Resource is destroyed, TrackedCallback will be Aborted and should know
> not to fire again.
> 
> The usual pattern is to store your callback and the desc inside this object,
and
> return PP_ERROR_INPROGRESS if the caller tries to Describe while one is
pending.
> 
> On the bright side, this means you can support multiple "Describe" calls in
> flight. I think I like the way you're doing it, but I just wanted you to be
> aware that it's different than usual and therefore might hit some latent bugs.

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
File ppapi/proxy/truetype_font_singleton_resource.h (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
ppapi/proxy/truetype_font_singleton_resource.h:25: class
TrueTypeFontSingletonResource
This is the resource for the browser side. The renderer one is
TrueTypeFontResource.
On 2013/03/12 18:07:58, dmichael wrote:
> s/Singleton//

https://codereview.chromium.org/12600019/diff/4001/ppapi/proxy/truetype_font_...
ppapi/proxy/truetype_font_singleton_resource.h:39: PP_CompletionCallback
callback) OVERRIDE;
On 2013/03/12 18:07:58, dmichael wrote:
> Should be scoped_refptr<TrackedCallback> (optionally const&)

Done.

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
File ppapi/thunk/ppb_truetype_font_api.h (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
ppapi/thunk/ppb_truetype_font_api.h:18: class PPAPI_THUNK_EXPORT
PPB_TrueTypeFont_API {
I'll switch this over in a follow up patch, when EnterInstanceAPI is supported.
On 2013/03/12 18:07:58, dmichael wrote:
> Note Justin TerAvest has been trying to make thunks automatically generated.
> There's an attribute you can add to your
>
IDL:https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file_system.idl&q=generate_thunk&sq=package:chromium&type=cs&l=10
> 
> If it works for you at all, it will put GetFontFamilies in to 1 API with
these.
> Is there any chance that's feasible? Or maybe you can talk to Justin about
ways
> to make the thunk generator work for your stuff?
> 
> If it's too hard, I'm okay with it never being generated. But it's worth
looking
> in to briefly. Possibly for a future CL if you change anything at all.

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
File ppapi/thunk/ppb_truetype_font_thunk.cc (right):

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
ppapi/thunk/ppb_truetype_font_thunk.cc:24:
EnterInstanceAPI<PPB_TrueTypeFont_Singleton_API> enter(instance);
On 2013/03/12 18:07:58, dmichael wrote:
> You need to pass the callback here in order to get proper callback behavior.

Done.

https://codereview.chromium.org/12600019/diff/4001/ppapi/thunk/ppb_truetype_f...
ppapi/thunk/ppb_truetype_font_thunk.cc:27: return
enter.functions()->GetFontFamilies(instance, output, callback);
On 2013/03/12 18:07:58, dmichael wrote:
> And this should be enter.callback()

Done.

[bbudge, 2013-03-13 18:28:38]

Addressed David's comments, rebased, and recreated my branch locally. A git
rebase went bad. PTAL

[dmichael (off chromium), 2013-03-13 20:20:25]

lgtm

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc
(right):

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:23:
sorted_families.insert(::pango_font_family_get_name(families[i]));
BTW, why do you do the intermediate set thing? Is it to make sure they are
unique? Ordered? A comment might be good. If it's an ordering thing, you could
instead use only the vector and sort it after the elements are added.

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:25: 
DCHECK(font_families->empty());
?

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:29:
font_families->push_back(*iter);
could also do font_families->insert(font_families.end(),
sorted_families.begin(), sorted_families.end());

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_win.cc
(right):

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_win.cc:48:
font_families->push_back(*iter);
same suggestions as for linux

https://codereview.chromium.org/12600019/diff/38001/ppapi/proxy/truetype_font...
File ppapi/proxy/truetype_font_singleton_resource.h (right):

https://codereview.chromium.org/12600019/diff/38001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.h:40: const
scoped_refptr<TrackedCallback> callback) OVERRIDE;
did you mean const scoped_refptr<TrackedCallback>&?
(note the &)

[bbudge, 2013-03-13 21:20:37]

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc
(right):

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:23:
sorted_families.insert(::pango_font_family_get_name(families[i]));
This code is originally from content/common/font_list_linux.cc. I assumed it was
because there might be duplicates. If it's just to sort, I can just use a vector
and sort that when it's full. Brett?
On 2013/03/13 20:20:25, dmichael wrote:
> BTW, why do you do the intermediate set thing? Is it to make sure they are
> unique? Ordered? A comment might be good. If it's an ordering thing, you could
> instead use only the vector and sort it after the elements are added.

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:25: 
See comment below.
On 2013/03/13 20:20:25, dmichael wrote:
> DCHECK(font_families->empty());
> ?

https://codereview.chromium.org/12600019/diff/38001/ppapi/proxy/truetype_font...
File ppapi/proxy/truetype_font_singleton_resource.h (right):

https://codereview.chromium.org/12600019/diff/38001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.h:40: const
scoped_refptr<TrackedCallback> callback) OVERRIDE;
On 2013/03/13 20:20:25, dmichael wrote:
> did you mean const scoped_refptr<TrackedCallback>&?
> (note the &)

Done.

[dmichael (off chromium), 2013-03-13 21:50:51]

OOps, noticed another nit

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_singleton_host.h
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_singleton_host.h:15:
class PepperTrueTypeFontSingletonHost : public ppapi::host::ResourceHost {
On 2013/03/13 01:26:50, bbudge1 wrote:
> This follows the precedent of browser_font_singleton_host. 
That one actually is the exception to the rule... 
https://code.google.com/p/chromium/codesearch#search/&q=kSingletonResourceId&...

Also, there's a
> PepperTrueTypeFontHost on the renderer side.
Maybe the browser one should have "Family" in it instead of Singleton, then?
(Darn, I was hoping to shorten it more than that)


> On 2013/03/12 18:07:58, dmichael wrote:
> > This name's pretty long, and it looks like we don't use "Singleton" in the
> name
> > usually (even when they are instance-singleton resources). I'd drop that
part.
>

[bbudge, 2013-03-13 22:48:50]

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_truetype_font_singleton_host.h
(right):

https://codereview.chromium.org/12600019/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_truetype_font_singleton_host.h:15:
class PepperTrueTypeFontSingletonHost : public ppapi::host::ResourceHost {
On 2013/03/13 21:50:51, dmichael wrote:
> On 2013/03/13 01:26:50, bbudge1 wrote:
> > This follows the precedent of browser_font_singleton_host. 
> That one actually is the exception to the rule... 
>
https://code.google.com/p/chromium/codesearch#search/&q=kSingletonResourceId&...
> 
> Also, there's a
> > PepperTrueTypeFontHost on the renderer side.
> Maybe the browser one should have "Family" in it instead of Singleton, then?
> (Darn, I was hoping to shorten it more than that)
> 
> 
> > On 2013/03/12 18:07:58, dmichael wrote:
> > > This name's pretty long, and it looks like we don't use "Singleton" in the
> > name
> > > usually (even when they are instance-singleton resources). I'd drop that
> part.
> > 
> 

Done. I renamed it PepperTrueTypeFontListHost.

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc
(right):

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:23:
sorted_families.insert(::pango_font_family_get_name(families[i]));
I removed the use of a set. I sort the vector in the host object so we don't
replicate this code for every platform. Mac may return the fonts already sorted,
but this isolates us if that ever changed and has little perf impact.

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:25: 
This API is pretty foolproof. There is no provision for returning an error.
Also, a platform could in theory be configured to have no true type fonts.
On 2013/03/13 20:20:25, dmichael wrote:
> DCHECK(font_families->empty());
> ?

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_linux.cc:29:
font_families->push_back(*iter);
Eliminated 'set'
On 2013/03/13 20:20:25, dmichael wrote:
> could also do font_families->insert(font_families.end(),
> sorted_families.begin(), sorted_families.end());

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_win.cc
(right):

https://codereview.chromium.org/12600019/diff/38001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_win.cc:48:
font_families->push_back(*iter);
On 2013/03/13 20:20:25, dmichael wrote:
> same suggestions as for linux

Done.

[brettw, 2013-03-14 18:31:21]

lgtm

[bbudge, 2013-03-14 18:57:13]

Added some explanation of the messages for jschuh to help with IPC message
review.

https://codereview.chromium.org/12600019/diff/56001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/12600019/diff/56001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1427: std::vector<std::string> /* font_families */)
jschuh: This message goes to the browser process (the rest to the renderer) The
vector is a list of strings for the font families available on the host system.
The names come from the host OS and are valid utf-8.

https://codereview.chromium.org/12600019/diff/56001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1429: ppapi::proxy::SerializedTrueTypeFontDesc /*
desc */)
jschuh: SerializedTrueTypeFontDesc contains a std::string that holds the family
name of the desired font. This value comes from the plugin and is untrusted. It
will be passed to the host OS when we try to create the font in the renderer
process. The code to actually create the font will have to check that the string
is valid utf-8. That's in the next CL.

https://codereview.chromium.org/12600019/diff/56001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1432: ppapi::proxy::SerializedTrueTypeFontDesc /*
desc */)
jschuh: SerializedTrueTypeFontDesc contains a std::string that holds the family
name of the font. In this message it's an out param. The value is ignored and it
is filled in by renderer code. It should be valid utf-8 from the host OS.

https://codereview.chromium.org/12600019/diff/56001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1435: std::vector<uint32_t> /* tags */)
jschuh: This returns a list of font table tags (32 bit int keys) used to request
specific font tables. It's constructed in the renderer and should be relatively
small (1k max).

https://codereview.chromium.org/12600019/diff/56001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1441: std::string /* data */)
jschuh: This returns actual font table data. It can potentially be very large
(say 30 MB). It's constructed in the renderer process.

[bbudge, 2013-03-14 19:31:22]

https://codereview.chromium.org/12600019/diff/75001/content/renderer/pepper/c...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/12600019/diff/75001/content/renderer/pepper/c...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:88: if
(IsStringUTF8(desc.family)) {
This check has to be done here, or else in each platform implementation. It's a
bit awkward to to it here, but this ensures every platform will be protected.

[jschuh, 2013-03-14 21:56:07]

A few comments. I'll make a last sanity check after resolving these.

https://codereview.chromium.org/12600019/diff/75001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_host.cc
(right):

https://codereview.chromium.org/12600019/diff/75001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_host.cc:75: int
result = static_cast<int32_t>(font_families.size());
Use a checked_numeric_cast here.

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1441: std::string /* data */)
Given the size, should this be shared memory? It's not really a security issue,
but that's quite a bit of data to send over the IPC channel.

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
File ppapi/proxy/truetype_font_resource.cc (right):

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_resource.cc:97: result ==
static_cast<int32_t>(tag_array.size()));
If result is negative, wouldn't you end up passing a large positive value?

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_resource.cc:117: result ==
static_cast<int32_t>(data.size()));
Same question as above.

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
File ppapi/proxy/truetype_font_singleton_resource.cc (right):

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:50: result ==
static_cast<int32_t>(font_families.size()));
What does a negative result mean, and should we just return an error from here?

[bbudge, 2013-03-14 23:20:22]

Addressed first round of jschuh's comments.

https://codereview.chromium.org/12600019/diff/75001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_host.cc
(right):

https://codereview.chromium.org/12600019/diff/75001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_host.cc:75: int
result = static_cast<int32_t>(font_families.size());
On 2013/03/14 21:56:07, Justin Schuh wrote:
> Use a checked_numeric_cast here.

Done.

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1441: std::string /* data */)
We've talked about that. For now this may be good enough so we'll leave that for
a future enhancement if it proves necessary.
On 2013/03/14 21:56:07, Justin Schuh wrote:
> Given the size, should this be shared memory? It's not really a security
issue,
> but that's quite a bit of data to send over the IPC channel.

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
File ppapi/proxy/truetype_font_resource.cc (right):

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_resource.cc:97: result ==
static_cast<int32_t>(tag_array.size()));
Result is negative when there is a Pepper error. Because of the std::max with 0
below we write 0 bytes if the result is negative. The semantics of array output
are to always call the client, even if the size is 0. There's a precedent in
FileIOResource::OnPluginMsgReadComplete which I follow here.
On 2013/03/14 21:56:07, Justin Schuh wrote:
> If result is negative, wouldn't you end up passing a large positive value?

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_resource.cc:117: result ==
static_cast<int32_t>(data.size()));
Same reply too.
On 2013/03/14 21:56:07, Justin Schuh wrote:
> See above.

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
File ppapi/proxy/truetype_font_singleton_resource.cc (right):

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:50: result ==
static_cast<int32_t>(font_families.size()));
Negative means there is an error. On all current platform implementations, there
is no possibility of error, just a 0-length list. If there was an error, the
name list should be empty and we would construct a 0-length array of Vars and do
the array output.

On 2013/03/14 21:56:07, Justin Schuh wrote:
> What does a negative result mean, and should we just return an error from
here?

[jschuh, 2013-03-15 18:28:01]

ipc security lgtm.

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
File ppapi/proxy/truetype_font_resource.cc (right):

https://codereview.chromium.org/12600019/diff/75001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_resource.cc:97: result ==
static_cast<int32_t>(tag_array.size()));
On 2013/03/14 23:20:23, bbudge1 wrote:
> Result is negative when there is a Pepper error. Because of the std::max with
0
> below we write 0 bytes if the result is negative. The semantics of array
output
> are to always call the client, even if the size is 0. There's a precedent in
> FileIOResource::OnPluginMsgReadComplete which I follow here.
> On 2013/03/14 21:56:07, Justin Schuh wrote:
> > If result is negative, wouldn't you end up passing a large positive value?
> 

Gah, sorry. My sleep addled brain translated it to std::min.

[commit-bot: I haz the power, 2013-03-15 18:32:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/12600019/83001

[commit-bot: I haz the power, 2013-03-15 18:50:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-15 19:22:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/12600019/83001

[commit-bot: I haz the power, 2013-03-15 20:15:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-15 21:44:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/12600019/23050

[commit-bot: I haz the power, 2013-03-16 09:57:58]

Change committed as 188576