Add Pepper TrueType font API call to enumerate fonts in a given family.

Add Pepper TrueType font API call to enumerate fonts in a given family.
Adds a new function, GetFontsInFamily, to the PPB_TrueTypeFont_Dev interface.
This method returns an array of descriptors for every font in the given family
on the host platform.

Tests are currently disabled for Windows and Mac, since I got failures on XP and OSX 10.6 when landing them originally. I will re-enable them in follow on patches, which will be easier to land / revert if necessary. The tests pass locally for me on all platforms.

BUG=79375, 230130
TEST=browser_tests, gtest_filter="PPAPIOutOfProcessTest.TrueTypeFont"

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

[bbudge, 2013-04-12 17:34:46]

Brett for content, and Windows, Linux impls.
Ben for Mac impl.
Mike for overall API
David and/or Yuzhu for ppapi

[bbudge, 2013-04-12 17:38:56]

On versioning: Justin Teravest is working on thunk auto generation features so
we can auto generate the thunk for this API. We realized the IsFont function
should be IsTrueTypeFont, so we're going to redo this interface. Since it's
unlikely anyone is using this yet, I'd like to just redo it at the initial
version 0.1. If there's a good reason, I can version the interface instead.

[bbudge, 2013-04-12 17:51:30]

+Justin for ppapi_messages.h (IPC)

[bungeman-skia, 2013-04-12 19:05:59]

The mac impl lgtm.

[dmichael (off chromium), 2013-04-12 23:34:18]

https://codereview.chromium.org/13913006/diff/28001/ppapi/api/dev/ppb_truetyp...
File ppapi/api/dev/ppb_truetype_font_dev.idl (right):

https://codereview.chromium.org/13913006/diff/28001/ppapi/api/dev/ppb_truetyp...
ppapi/api/dev/ppb_truetype_font_dev.idl:169: * desc contains a PP_Var for the
family name which must be released.
This is inconsistent with what you say in your Resource implementation (but I
think the behavior in this ^^^^ documentation is better)

https://codereview.chromium.org/13913006/diff/28001/ppapi/cpp/dev/truetype_fo...
File ppapi/cpp/dev/truetype_font_dev.h (right):

https://codereview.chromium.org/13913006/diff/28001/ppapi/cpp/dev/truetype_fo...
ppapi/cpp/dev/truetype_font_dev.h:143: /// completion of GetFontsInFamily.
nit: Your params need to be updated for the C++ interface.

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

https://codereview.chromium.org/13913006/diff/28001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:89: // The result code should
contain the data size if it's positive.
It's possible that you can get here in a situation where the callback has
already been aborted. I.e., the plugin requested the Fonts in the family but
then the resource got released. In that case, the callback will own the only
ref-count for this resource and for the TrackedCallback.

Maybe in this situation it would be better to not do the output allocation and
stuff? I think it gets leaked, since the callback won't be run a second time.

https://codereview.chromium.org/13913006/diff/28001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:96: // Convvert the message data
to an array of PP_TrueTypeFontDesc_Dev structs.
nit: Convvert->Convert

https://codereview.chromium.org/13913006/diff/28001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:97: // The embedded family
PP_Var is null so we don't worry about refs.
The family is NULL? That seems like it might be confusing (even though it seems
like apps won't often use it in this case)

https://codereview.chromium.org/13913006/diff/28001/ppapi/tests/test_truetype...
File ppapi/tests/test_truetype_font.cc (right):

https://codereview.chromium.org/13913006/diff/28001/ppapi/tests/test_truetype...
ppapi/tests/test_truetype_font.cc:9: #include <stdio.h>
nit: I'm guessing you don't want this anymore

[bbudge, 2013-04-13 00:44:44]

https://codereview.chromium.org/13913006/diff/28001/ppapi/api/dev/ppb_truetyp...
File ppapi/api/dev/ppb_truetype_font_dev.idl (right):

https://codereview.chromium.org/13913006/diff/28001/ppapi/api/dev/ppb_truetyp...
ppapi/api/dev/ppb_truetype_font_dev.idl:169: * desc contains a PP_Var for the
family name which must be released.
This is the behavior now. I'll update the comment on the resource.
On 2013/04/12 23:34:18, dmichael wrote:
> This is inconsistent with what you say in your Resource implementation (but I
> think the behavior in this ^^^^ documentation is better)

https://codereview.chromium.org/13913006/diff/28001/ppapi/cpp/dev/truetype_fo...
File ppapi/cpp/dev/truetype_font_dev.h (right):

https://codereview.chromium.org/13913006/diff/28001/ppapi/cpp/dev/truetype_fo...
ppapi/cpp/dev/truetype_font_dev.h:143: /// completion of GetFontsInFamily.
On 2013/04/12 23:34:18, dmichael wrote:
> nit: Your params need to be updated for the C++ interface.

Done.

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

https://codereview.chromium.org/13913006/diff/28001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:89: // The result code should
contain the data size if it's positive.
Is that possible on a singleton resource?
On 2013/04/12 23:34:18, dmichael wrote:
> It's possible that you can get here in a situation where the callback has
> already been aborted. I.e., the plugin requested the Fonts in the family but
> then the resource got released. In that case, the callback will own the only
> ref-count for this resource and for the TrackedCallback.
> 
> Maybe in this situation it would be better to not do the output allocation and
> stuff? I think it gets leaked, since the callback won't be run a second time.

https://codereview.chromium.org/13913006/diff/28001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:96: // Convvert the message data
to an array of PP_TrueTypeFontDesc_Dev structs.
On 2013/04/12 23:34:18, dmichael wrote:
> nit: Convvert->Convert

Done.

https://codereview.chromium.org/13913006/diff/28001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:97: // The embedded family
PP_Var is null so we don't worry about refs.
I initially thought I could sidestep problems with the embedded PP_Var, but I
think it's likely plugins will want to use these descriptors to create fonts, so
I now set the name. Do I need to do anything special here? There's no
StoreVectorOfStructWithEmbeddedVar call.

On 2013/04/12 23:34:18, dmichael wrote:
> The family is NULL? That seems like it might be confusing (even though it
seems
> like apps won't often use it in this case)

https://codereview.chromium.org/13913006/diff/28001/ppapi/tests/test_truetype...
File ppapi/tests/test_truetype_font.cc (right):

https://codereview.chromium.org/13913006/diff/28001/ppapi/tests/test_truetype...
ppapi/tests/test_truetype_font.cc:9: #include <stdio.h>
On 2013/04/12 23:34:18, dmichael wrote:
> nit: I'm guessing you don't want this anymore

Done.

[dmichael (off chromium), 2013-04-15 18:11:43]

Parts I reviewed look mostly good to me. I think theer's still a corner-case
leak.

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

https://codereview.chromium.org/13913006/diff/28001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:89: // The result code should
contain the data size if it's positive.
On 2013/04/13 00:44:44, bbudge1 wrote:
> Is that possible on a singleton resource?
> On 2013/04/12 23:34:18, dmichael wrote:
> > It's possible that you can get here in a situation where the callback has
> > already been aborted. I.e., the plugin requested the Fonts in the family but
> > then the resource got released. In that case, the callback will own the only
> > ref-count for this resource and for the TrackedCallback.
> > 
> > Maybe in this situation it would be better to not do the output allocation
and
> > stuff? I think it gets leaked, since the callback won't be run a second
time.
> 
Oh, forgot about that. Yes, I think that would still happen on instance
shutdown.

I'm thinking you can avoid it by just checking if the callback is pending:
if (!TrackedCallback::IsPending(callback)) {
  // (Clean up Vars)
  return;
}

[bbudge, 2013-04-17 23:49:19]

Don't create Vars at all if callback isn't pending.
Rebase to pick up Justin's and other changes.
Fix GetFontsInFamily invalid instance test to be more tolerant.

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

https://codereview.chromium.org/13913006/diff/28001/ppapi/proxy/truetype_font...
ppapi/proxy/truetype_font_singleton_resource.cc:89: // The result code should
contain the data size if it's positive.
I added the check at the beginning of this method and the one above too.
On 2013/04/15 18:11:44, dmichael wrote:
> On 2013/04/13 00:44:44, bbudge1 wrote:
> > Is that possible on a singleton resource?
> > On 2013/04/12 23:34:18, dmichael wrote:
> > > It's possible that you can get here in a situation where the callback has
> > > already been aborted. I.e., the plugin requested the Fonts in the family
but
> > > then the resource got released. In that case, the callback will own the
only
> > > ref-count for this resource and for the TrackedCallback.
> > > 
> > > Maybe in this situation it would be better to not do the output allocation
> and
> > > stuff? I think it gets leaked, since the callback won't be run a second
> time.
> > 
> Oh, forgot about that. Yes, I think that would still happen on instance
> shutdown.
> 
> I'm thinking you can avoid it by just checking if the callback is pending:
> if (!TrackedCallback::IsPending(callback)) {
>   // (Clean up Vars)
>   return;
> }

[dmichael (off chromium), 2013-04-18 16:55:33]

lgtm for my parts

https://codereview.chromium.org/13913006/diff/46003/ppapi/tests/test_truetype...
File ppapi/tests/test_truetype_font.cc (right):

https://codereview.chromium.org/13913006/diff/46003/ppapi/tests/test_truetype...
ppapi/tests/test_truetype_font.cc:9: #include <stdio.h>
Oops, it's still there

[bbudge, 2013-04-18 17:21:06]

https://codereview.chromium.org/13913006/diff/46003/ppapi/tests/test_truetype...
File ppapi/tests/test_truetype_font.cc (right):

https://codereview.chromium.org/13913006/diff/46003/ppapi/tests/test_truetype...
ppapi/tests/test_truetype_font.cc:9: #include <stdio.h>
git troubles. Done.
On 2013/04/18 16:55:33, dmichael wrote:
> Oops, it's still there

[brettw, 2013-04-18 17:45:05]

Owners lgtm, I didn't check the details. I noted two style nits:

https://codereview.chromium.org/13913006/diff/71001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_truetype_font_mac.mm (right):

https://codereview.chromium.org/13913006/diff/71001/content/renderer/pepper/p...
content/renderer/pepper/pepper_truetype_font_mac.mm:26: CFNumberRef num;
Check indenting.

https://codereview.chromium.org/13913006/diff/71001/content/renderer/pepper/p...
content/renderer/pepper/pepper_truetype_font_mac.mm:28:
CFDictionaryGetValueIfPresent(dict, name, (const void**)&num) &&
Can you use C++ casts instead?

[bbudge, 2013-04-18 19:40:40]

https://codereview.chromium.org/13913006/diff/71001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_truetype_font_mac.mm (right):

https://codereview.chromium.org/13913006/diff/71001/content/renderer/pepper/p...
content/renderer/pepper/pepper_truetype_font_mac.mm:26: CFNumberRef num;
On 2013/04/18 17:45:05, brettw wrote:
> Check indenting.

Done.

https://codereview.chromium.org/13913006/diff/71001/content/renderer/pepper/p...
content/renderer/pepper/pepper_truetype_font_mac.mm:28:
CFDictionaryGetValueIfPresent(dict, name, (const void**)&num) &&
On 2013/04/18 17:45:05, brettw wrote:
> Can you use C++ casts instead?

Done.

[bbudge, 2013-04-18 19:53:10]

-Justin
+Chris for IPC message review, specifically ppapi_messages.h. Feel free to
delegate.

[bbudge, 2013-04-18 20:46:34]

Removing Chris,
Adding tsepez for IPC review (ppapi_messages.h)

[Tom Sepez, 2013-04-18 20:50:47]

Messages LGTM.

[commit-bot: I haz the power, 2013-04-18 21:24:08]

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

[commit-bot: I haz the power, 2013-04-18 21:27:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-18 21:32:56]

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

[commit-bot: I haz the power, 2013-04-18 21:33:04]

Presubmit check for 13913006-75002 failed and returned exit status 1.

INFO:root:Found 22 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Traceback (most recent call last):
  File "/b/commit-queue/verification/presubmit_shim.py", line 33, in <module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1305, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1129, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1046, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 865, in CheckChangeOnCommit
  File "<string>", line 700, in _CommonChecks
  File "<string>", line 258, in _CheckNoUNIT_TESTInSourceFiles
  File "/b/depot_tools/presubmit_support.py", line 549, in ChangedContents
    for line in self.GenerateScmDiff().splitlines():
  File "/b/depot_tools/presubmit_support.py", line 619, in GenerateScmDiff
    [self.LocalPath()], self._local_root, False, None)
  File "/b/depot_tools/scm.py", line 913, in GenerateDiff
    full_move, revision))
  File "/b/depot_tools/scm.py", line 837, in _DiffItemInternal
    data = SVN.Capture(command, cwd)
  File "/b/depot_tools/scm.py", line 479, in Capture
    ['svn'] + args, stderr=subprocess2.PIPE, cwd=cwd, **kwargs)
  File "/b/depot_tools/subprocess2.py", line 449, in check_output
    return check_call_out(args, stdout=PIPE, **kwargs)[0]
  File "/b/depot_tools/subprocess2.py", line 412, in check_call_out
    returncode, args, kwargs.get('cwd'), out[0], out[1])
subprocess2.CalledProcessError: Command svn diff --internal-diff
content/browser/renderer_host/pepper/pepper_truetype_font_list_android.cc
returned non-zero exit status 1 in /b/commit-queue/workdir/chromium
svn: invalid option: --internal-diff
Type 'svn help' for usage.

[commit-bot: I haz the power, 2013-04-18 22:17:55]

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

[commit-bot: I haz the power, 2013-04-19 03:50:39]

Change committed as 195082

[kylesch, 2013-04-19 05:10:58]

fix build for x86_64 where NSInteger is typedef'd as long and parameters passed
to std::max need to be same type.  

IMO it looks nicer to use a 0L for the integer literal, but then on 32bit
NSinteger would be a normal int so the build failure would just reverse ...

https://chromiumcodereview.appspot.com/13913006/diff/75002/content/browser/re...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_mac.mm
(right):

https://chromiumcodereview.appspot.com/13913006/diff/75002/content/browser/re...
content/browser/renderer_host/pepper/pepper_truetype_font_list_mac.mm:59:
font_weight = std::max(0, font_weight);
font_weight = std::max(static_cast<NSInteger>(0), font_weight);

[bbudge, 2013-04-19 10:30:02]

https://codereview.chromium.org/13913006/diff/75002/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_truetype_font_list_mac.mm
(right):

https://codereview.chromium.org/13913006/diff/75002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_truetype_font_list_mac.mm:59:
font_weight = std::max(0, font_weight);
Thanks, I can try to land this when I re-enable the test on Mac.
On 2013/04/19 05:10:58, kylesch wrote:
> font_weight = std::max(static_cast<NSInteger>(0), font_weight);