Call Skia's version of SkFontHost_fontconfig

Call version of SkFontHost_fontconfig in Skia (cloned from here).
Retool the direct/remove plumbing to use SkFontConfigInterface.

This change allows Skia to interate on the details of SkFontHost w/o having
to synchromize with chrome on each change. It also means Skia's internal testing
can exercise exactly the same config that Chrome does.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187283

[reed1, 2013-03-04 20:10:32]

cevans or thakis, I need an owner reviewer as well. interested?

[Nico, 2013-03-04 20:15:09]

skia/ owners-lgtm

cevans probably wants to glance over the ipc bits

[reed1, 2013-03-04 20:19:37]

This requires a small skia change, which should land in DEPS tomorrow morning. I
will run trybots then.

[cevans, 2013-03-05 01:27:54]

I'm OOO, so is jln@.
Perhaps jorgelo@ or palmer@ had a few mins, since it looks Linux-y ?

On Mon, Mar 4, 2013 at 12:15 PM, <thakis@chromium.org> wrote:

> skia/ owners-lgtm
>
> cevans probably wants to glance over the ipc bits
>
>
https://codereview.chromium.**org/12391070/<https://codereview.chromium.org/1...
>

[Chris Palmer, 2013-03-05 01:42:51]

Looking at it now.


On Mon, Mar 4, 2013 at 5:27 PM, Chris Evans <cevans@google.com> wrote:
> I'm OOO, so is jln@.
> Perhaps jorgelo@ or palmer@ had a few mins, since it looks Linux-y ?
>
> On Mon, Mar 4, 2013 at 12:15 PM, <thakis@chromium.org> wrote:
>>
>> skia/ owners-lgtm
>>
>> cevans probably wants to glance over the ipc bits
>>
>> https://codereview.chromium.org/12391070/
>
>

[Chris Palmer, 2013-03-05 01:49:21]

Oops, I am not going to be able to finish it today (gotta leave at 6
to teach guitar lesson). If you need it done ASAP, hopefully Jorge can
tackle it. Otherwise I'll do it in the morning.

On Mon, Mar 4, 2013 at 5:42 PM, Chris Palmer <palmer@google.com> wrote:
> Looking at it now.
>
>
> On Mon, Mar 4, 2013 at 5:27 PM, Chris Evans <cevans@google.com> wrote:
>> I'm OOO, so is jln@.
>> Perhaps jorgelo@ or palmer@ had a few mins, since it looks Linux-y ?
>>
>> On Mon, Mar 4, 2013 at 12:15 PM, <thakis@chromium.org> wrote:
>>>
>>> skia/ owners-lgtm
>>>
>>> cevans probably wants to glance over the ipc bits
>>>
>>> https://codereview.chromium.org/12391070/
>>
>>

[reed1, 2013-03-05 15:22:43]

The IPC change should reflect the following:

The old code kept 2 different private maps, to associate path-names and 32bit
ids. This ID was passed back and forth by the IPC (along with other data).

The new code now stores the path-name w/ the SkTypeface, eliminating the need
for both private maps (yea!). So now we (for the openStream api) just pass the
path-name directly to be opened.

[palmer, 2013-03-05 21:38:53]

https://codereview.chromium.org/12391070/diff/26002/content/browser/renderer_...
File content/browser/renderer_host/render_sandbox_host_linux.cc (right):

https://codereview.chromium.org/12391070/diff/26002/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:179: family.c_str(),
(SkTypeface::Style)requested_style,
C-style cast; use C++ style instead.

https://codereview.chromium.org/12391070/diff/26002/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:198: if
(!pickle.ReadString(&iter, &path))
Who sends these pickles? Untrustworthy renderers? The browser must not open
arbitrary pathnames on behalf of an untrustworthy requester.

https://codereview.chromium.org/12391070/diff/26002/content/common/font_confi...
File content/common/font_config_ipc_linux.cc (right):

https://codereview.chromium.org/12391070/diff/26002/content/common/font_confi...
content/common/font_config_ipc_linux.cc:74: *outStyle =
(SkTypeface::Style)reply_style;
Use C++-style casts.

[palmer, 2013-03-05 22:32:08]

Talking with Mike, it sounds like we are going to have to go with some kind of
map to allow the browser to resolve font names/IDs to pathnames. The browser
will determine the pathnames unilaterally, rather than opening whatever the
other side requests.

However, that is the "only" change — the browser will consult the map in the
call site to open(2) in renderer_sandbox_host_linux.cc. Otherwise this CL is
fine from a security point of view.

[reed1, 2013-03-06 14:44:08]

Now passing id (index) instead of arbitrary path, so only the secure process
gets to choose which file to open.

[palmer, 2013-03-06 21:26:48]

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
File content/browser/renderer_host/render_sandbox_host_linux.cc (right):

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:166: int count =
paths_.count();
|count| and |i| should be size_t, right? And then this function should return
size_t.

Or is SkTDArray::count defined to return int? Yeah, I see that SkTDArray
declares .fCount as size_t, and then the ::count accessor casts it to int.
Ultimately, you'll want to declare ::count to return size_t and get rid of the
cast, but you could also pull in checked_numeric_cast from Chromium base.

Multiple people in my cube found the type confusion confusing, so I really think
you should fix this at some point. :)

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:198:
result_identity.fID = FindOrAddPath(result_identity.fString);
And thus .fID will have to be declared as size_t (preferred), or checked and
static_cast'ed.

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:201:
reply.WriteString(std::string(result_family.c_str()));
Is it necessary to convert result_family to a C string and then create a new
std::string here? Can you just pass result_family and be done?

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:202:
reply.WriteUInt32(result_identity.fID);
Ahh, if we are imposing an explicit width on the protocol (which is fine), then
we should declare .fID as UInt32 and static_cast it above (line 198).

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:204:
reply.WriteString(std::string(result_identity.fString.c_str()));
Same comment here — no need to laboriously copy string? The previous code did
not, and just passed result_family (previous line 211).

https://codereview.chromium.org/12391070/diff/32001/content/common/font_confi...
File content/common/font_config_ipc_linux.cc (right):

https://codereview.chromium.org/12391070/diff/32001/content/common/font_confi...
content/common/font_config_ipc_linux.cc:83: std::string
path(identity.fString.c_str());
Never used?

[reed1, 2013-03-06 22:03:22]

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
File content/browser/renderer_host/render_sandbox_host_linux.cc (right):

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:166: int count =
paths_.count();
On 2013/03/06 21:26:48, Chris P. wrote:
> |count| and |i| should be size_t, right? And then this function should return
> size_t.
> 
> Or is SkTDArray::count defined to return int? Yeah, I see that SkTDArray
> declares .fCount as size_t, and then the ::count accessor casts it to int.
> Ultimately, you'll want to declare ::count to return size_t and get rid of the
> cast, but you could also pull in checked_numeric_cast from Chromium base.
> 
> Multiple people in my cube found the type confusion confusing, so I really
think
> you should fix this at some point. :)

Skia uses int for all of its indices and counts, so our array class' interface
is consistent with that. Not sure it matters how the private field is defined
for this review.

I can change this helper function to return uint32_t, which is how the caller is
going to use the result, but I don't know that that helps readability for the
function itself. It returns the index of the entry where path was
found-or-added, and that is an int in this case.

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:198:
result_identity.fID = FindOrAddPath(result_identity.fString);
On 2013/03/06 21:26:48, Chris P. wrote:
> And thus .fID will have to be declared as size_t (preferred), or checked and
> static_cast'ed.

fID is declared to be uint32_t, and its intended to be an abstract 32bit field
for the subclass of SkFontConfigInterface to use as it sees fit. This subclass
is using it to store an index.

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:201:
reply.WriteString(std::string(result_family.c_str()));
On 2013/03/06 21:26:48, Chris P. wrote:
> Is it necessary to convert result_family to a C string and then create a new
> std::string here? Can you just pass result_family and be done?

result_string is SkString, not std::string, and there is no pickle reader/writer
for that type. The "conversion" to a C string is free (just returns a ptr). If
there is a pickle API for C strings, I should definitely change to call that
instead (to avoid the copy to std::string).

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:202:
reply.WriteUInt32(result_identity.fID);
On 2013/03/06 21:26:48, Chris P. wrote:
> Ahh, if we are imposing an explicit width on the protocol (which is fine),
then
> we should declare .fID as UInt32 and static_cast it above (line 198).

Done.

https://codereview.chromium.org/12391070/diff/32001/content/browser/renderer_...
content/browser/renderer_host/render_sandbox_host_linux.cc:204:
reply.WriteString(std::string(result_identity.fString.c_str()));
On 2013/03/06 21:26:48, Chris P. wrote:
> Same comment here — no need to laboriously copy string? The previous code did
> not, and just passed result_family (previous line 211).

I am of two minds about whether or not to return the string. At the moment, we
happen to not set the string to anything, but the entire point of FontIdentity
is that its contents (other than TTCIndex) are meant to be opaqueish. My intent
was to pickle the entire FontIdentity, so the the receiver need not be aware of
what aspects are being used at the moment. Since the string in this case is
empty, there should not be a measurable perf hit for the extra pickle entry.

https://codereview.chromium.org/12391070/diff/32001/content/common/font_confi...
File content/common/font_config_ipc_linux.cc (right):

https://codereview.chromium.org/12391070/diff/32001/content/common/font_confi...
content/common/font_config_ipc_linux.cc:83: std::string
path(identity.fString.c_str());
On 2013/03/06 21:26:48, Chris P. wrote:
> Never used?

Done.

[reed1, 2013-03-06 22:04:34]

I see that pickle has ReadData and WriteData, which handle char arrays. This
seems like a good way to avoid the string copy. I will look at doing that next.

[reed1, 2013-03-07 14:42:40]

moved pickle helpers for skia types into their own utilities (in skia/ext). This
both simplifies reading the call sites, and also allows the read and write paths
to appear in the same .cc, making it much easier to see that they are mutually
in sync.

PTAL

[reed1, 2013-03-08 16:20:57]

patch #8 just adds SK_API to new header for sharelib build.

ptal

[Jorge Lucangeli Obes, 2013-03-08 21:12:32]

On 2013/03/08 16:20:57, reed1 wrote:
> patch #8 just adds SK_API to new header for sharelib build.
> 
> ptal

security lgtm.

I would argue that the fact that you need to do static_cast's should be argument
enough that having int's for sizes in the API wrong, but it's not a discussion
for this CL.

[reed1, 2013-03-11 12:46:58]

Committed patchset #9 manually as r187283.