Add metrics to determine what font types are being used with print preview.

Add metrics to determine what font types are being used with print preview.

This depends on Skia r1444.

BUG=80918
TEST=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87242
Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=87248
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87257

[jar (doing other things), 2011-05-29 01:49:15]

I'm not expert on the instrumented code, but tried to guess what you were doing.

The histogram code LGTM, but there is a bias you may want to think about
removing (which would use less memory).

It is your choice...  I just wanted to be sure you thought about it.

http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc
File printing/pdf_metafile_skia.cc (right):

http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc#n...
printing/pdf_metafile_skia.cc:110:
SkAdvancedTypefaceMetrics::kNotEmbeddable_Font + 1);
There are some interesting biases you have in this, and you should be sure
you're doing this intentionally.  IMO, you may be adding complexity to the code
and size to data (using a hashmap), to further enhance the bias.

There are only about 7 distinct font types.  If a page has a dozen distinct
fonts, and they are all the same "FontType," do you really want to count that 12
times?  I'm guessing this code already counts pages that reload themselves more
than ones that don't (a harder bias to avoid).  IMO, it might be interesting to
know if a page has *any* fonts of a given type, but it is less significant that
it had dozens of a given FontType.... but then again, I'm not sure what you're
intending to conclude from the gathered stats.

Perhaps the nicest thing would be (some day) to have (for this histogram) a list
of how many users participated in use of each FontType.  Perhaps that will be
the most interesting metric, and that will de-dup the counting completely.

For now, I *suspect* you really only want to answer as to whether a given page
has a FontType.  Hence I'd *guess* you'd be better off maintaining a small
vector of bools, which you aggregate into the histogram here, than maintaining a
hash_map.

[Chris Guillory, 2011-05-29 01:50:48]

http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc
File printing/pdf_metafile_skia.cc (right):

http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc#n...
printing/pdf_metafile_skia.cc:84: const SkTDArray<SkPDFFont*>& font_resources =
Can we not put this logic into FinishDocument() somehow?

[vandebo (ex-Chrome), 2011-05-29 06:12:15]

On 2011/05/29 01:49:15, jar wrote:
> I'm not expert on the instrumented code, but tried to guess what you were
doing.
> 
> The histogram code LGTM, but there is a bias you may want to think about
> removing (which would use less memory).
> 
> It is your choice...  I just wanted to be sure you thought about it.
> 
> http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc
> File printing/pdf_metafile_skia.cc (right):
> 
>
http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc#n...
> printing/pdf_metafile_skia.cc:110:
> SkAdvancedTypefaceMetrics::kNotEmbeddable_Font + 1);
> There are some interesting biases you have in this, and you should be sure
> you're doing this intentionally.  IMO, you may be adding complexity to the
code
> and size to data (using a hashmap), to further enhance the bias.
> 
> There are only about 7 distinct font types.  If a page has a dozen distinct
> fonts, and they are all the same "FontType," do you really want to count that
12
> times?  I'm guessing this code already counts pages that reload themselves
more
> than ones that don't (a harder bias to avoid).  IMO, it might be interesting
to

Yes, we might prefer to count a page once, and not recount it each time it is
rerendered, but that is harder to accomplish and I don't expect a correlation
between font type and rerendering count, so on average this measurement should
be close enough to the other one.

> know if a page has *any* fonts of a given type, but it is less significant
that
> it had dozens of a given FontType.... but then again, I'm not sure what you're
> intending to conclude from the gathered stats.
> 
> Perhaps the nicest thing would be (some day) to have (for this histogram) a
list
> of how many users participated in use of each FontType.  Perhaps that will be
> the most interesting metric, and that will de-dup the counting completely.
> 
> For now, I *suspect* you really only want to answer as to whether a given page
> has a FontType.  Hence I'd *guess* you'd be better off maintaining a small
> vector of bools, which you aggregate into the histogram here, than maintaining
a
> hash_map.

I appreciate your feedback, but I think in this instance, this is what we want,
but I'll explain a bit more to see if you agree.  Simply based on default
installed fonts on major platforms, we know that TrueType is by far the most
popular format.  As such, we are doing things to improve our TrueType support
(namely supporting font subsetting to reduce file size) for PDF generation.  But
we are interested in knowing how popular other format are.  So, I think knowing
that a page has X TrueType fonts, and Y Type1 fonts will help us answer the
question of format popularity better than simply knowing a page contains
TrueType and Type1 fonts.

I'm going to commit once I get the dependencies in (M13 and all), but my change
things based on your feedback.

[vandebo (ex-Chrome), 2011-05-29 06:14:54]

http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc
File printing/pdf_metafile_skia.cc (right):

http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc#n...
printing/pdf_metafile_skia.cc:84: const SkTDArray<SkPDFFont*>& font_resources =
On 2011/05/29 01:50:48, Chris Guillory wrote:
> Can we not put this logic into FinishDocument() somehow?

We could keep a list of pages instead of just the current page... or we could
add an accessor to get the list of pages out of the Document, but I'm not sure
what that will gain us.  Can you say more?

[Chris Guillory, 2011-05-30 16:46:31]

LGTM.

http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc
File printing/pdf_metafile_skia.cc (right):

http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc#n...
printing/pdf_metafile_skia.cc:84: const SkTDArray<SkPDFFont*>& font_resources =
On 2011/05/29 06:14:54, vandebo wrote:
> On 2011/05/29 01:50:48, Chris Guillory wrote:
> > Can we not put this logic into FinishDocument() somehow?
> 
> We could keep a list of pages instead of just the current page... or we could
> add an accessor to get the list of pages out of the Document, but I'm not sure
> what that will gain us.  Can you say more?
For a second I was still thinking that the document had a way to access the
fonts but it looks it doesn't.
http://codereview.appspot.com/4547069/patch/1/7

Optional: I think an accessor for the pages makes sense.
* All this logic could be together inside of FinishDocuemnt.
* We wouldn't need to add the font_type_stats_ member.

[vandebo (ex-Chrome), 2011-05-30 17:34:41]

On 2011/05/30 16:46:31, Chris Guillory wrote:
> LGTM.
> 
> http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc
> File printing/pdf_metafile_skia.cc (right):
> 
>
http://codereview.chromium.org/7084021/diff/1/printing/pdf_metafile_skia.cc#n...
> printing/pdf_metafile_skia.cc:84: const SkTDArray<SkPDFFont*>& font_resources
=
> On 2011/05/29 06:14:54, vandebo wrote:
> > On 2011/05/29 01:50:48, Chris Guillory wrote:
> > > Can we not put this logic into FinishDocument() somehow?
> > 
> > We could keep a list of pages instead of just the current page... or we
could
> > add an accessor to get the list of pages out of the Document, but I'm not
sure
> > what that will gain us.  Can you say more?
> For a second I was still thinking that the document had a way to access the
> fonts but it looks it doesn't.
> http://codereview.appspot.com/4547069/patch/1/7
> 
> Optional: I think an accessor for the pages makes sense.
> * All this logic could be together inside of FinishDocuemnt.
> * We wouldn't need to add the font_type_stats_ member.

Ok, sounds good.  But I'll do that in a follow up CL, as it requires a Skia
change and a Skia roll.