Add font substitution metric for Linux

Add font map user metrics for Linux.

The method MapFont, defined in pdfium_engine.cc, is used by Chrome on
Linux to find a substitute font whenever a pdf file uses a font which is
not embedded (or could not be loaded). When this method is called, the
method UserMetricsRecordAction will now be called to record that this
document has requested a substitute font.

Adding similar functionality for other OS's will take more effort just
because the MapFont definitions are completely inside pdfium core.

BUG=chromium:448584
Committed: https://crrev.com/d24ca0432921fdf310c781c3a63aac7387306a9f
Cr-Commit-Position: refs/heads/master@{#424877}

[npm, 2016-10-05 19:31:40]

Description was changed from

==========
Add font substitution metric for Linux
==========

to

==========
Add font map user metrics for Linux.

The method MapFont, defined in pdfium_engine.cc, is used by Chrome on
Linux to find a substitute font whenever a pdf file uses a font which is
not embedded (or could not be loaded). When this method is called, the
method UserMetricsRecordAction will now be called to record that this
document has requested a substitute font.

Adding similar functionality for other OS's will take more effort just
because the MapFont definitions are completely inside pdfium core.

BUG=chromium:448584
==========

[npm, 2016-10-05 19:31:40]

npm@chromium.org changed reviewers:
+ dsinclair@chromium.org, rkaplow@chromium.org, thestig@chromium.org

[npm, 2016-10-05 19:32:57]

PTAL Lei for pdf part, Robert for metrics part

[rkaplow, 2016-10-05 19:40:29]

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance.cc
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:1319: font_substitution_reported = true;
UserAction should generally be used when a user has done something explicit to
trigger it. it should be used only when we need the timestamp of the action.

Is the goal here just to count the number of times this has occured?

Also - is this meant to be a beginning step to what we discussed earlier today?

[npm, 2016-10-05 19:53:03]

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance.cc
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:1319: font_substitution_reported = true;
On 2016/10/05 19:40:28, rkaplow wrote:
> UserAction should generally be used when a user has done something explicit to
> trigger it. it should be used only when we need the timestamp of the action.
> 
> Is the goal here just to count the number of times this has occured?
> 
> Also - is this meant to be a beginning step to what we discussed earlier
today?

I see this method being called for LoadFailure, LoadSuccess, and
PreviewDocumentLoadFailure in this same class. I believe those are not
userAction-dependent? What would be ideal here?

The goal is just to count how many times this occurs (but counting it once per
document).

This is much simpler than what we discussed today but still a useful first step.

[Lei Zhang, 2016-10-05 20:48:09]

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance.h
File pdf/out_of_process_instance.h (right):

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.h:315: bool font_substitution_reported;
members need a trailing underscore.

[rkaplow, 2016-10-05 21:03:10]

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance.cc
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.cc:1319: font_substitution_reported = true;
On 2016/10/05 19:53:03, npm wrote:
> On 2016/10/05 19:40:28, rkaplow wrote:
> > UserAction should generally be used when a user has done something explicit
to
> > trigger it. it should be used only when we need the timestamp of the action.
> > 
> > Is the goal here just to count the number of times this has occured?
> > 
> > Also - is this meant to be a beginning step to what we discussed earlier
> today?
> 
> I see this method being called for LoadFailure, LoadSuccess, and
> PreviewDocumentLoadFailure in this same class. I believe those are not
> userAction-dependent? What would be ideal here?
> 
> The goal is just to count how many times this occurs (but counting it once per
> document).
> 
> This is much simpler than what we discussed today but still a useful first
step.

Yes, it's not ideal. It is still better for it to be in a histogram since you
don't need the timing info. You can have it be a boolean histogram and just use
the 'true' bucket.

usually we like to have a baseline comparison - in this case I don't see a
natural baseline but if there is one it would be good (generally some sort of
fail case)

[npm, 2016-10-06 15:20:47]

PTAL

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance.h
File pdf/out_of_process_instance.h (right):

https://codereview.chromium.org/2398763002/diff/1/pdf/out_of_process_instance...
pdf/out_of_process_instance.h:315: bool font_substitution_reported;
On 2016/10/05 20:48:09, Lei Zhang wrote:
> members need a trailing underscore.

Done.

[rkaplow, 2016-10-07 17:40:31]

https://codereview.chromium.org/2398763002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2398763002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:41635: +<histogram
name="PDF.FontSubstituted" enum="PDFFontSubstituted">
I would just use Boolean, and name this something like 
PDF.IsFontSubstituted, so you don't need custom enum here

[npm, 2016-10-07 18:25:57]

Changed to Boolean in histograms.xml

[rkaplow, 2016-10-11 15:30:17]

lgtm

[Lei Zhang, 2016-10-11 22:35:59]

https://codereview.chromium.org/2398763002/diff/40001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2398763002/diff/40001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:1211:
uma_.HistogramEnumeration("PDF.IsFontSubstituted", 0, 2);
We are going to record this on other platforms. Do we care to?

https://codereview.chromium.org/2398763002/diff/40001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:1322:
uma_.HistogramEnumeration("PDF.IsFontSubstituted", 1, 2);
Do we want constants instead of 1 and 2? (and 0 above)

https://codereview.chromium.org/2398763002/diff/40001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2398763002/diff/40001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2455: g_client_for_fontmapper_ = client_;
I'm worried we set this but never unset it. There is only 1 PDF plugin process,
and it contains multiple engines. If engine A reaches here first, and then
engine B reaches here, and then engine B gets destroyed... we can end up with a
dangling pointer if engine A then reaches MapFont() somehow.

[npm, 2016-10-12 18:19:53]

PTAL Lei

https://codereview.chromium.org/2398763002/diff/40001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2398763002/diff/40001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:1211:
uma_.HistogramEnumeration("PDF.IsFontSubstituted", 0, 2);
On 2016/10/11 22:35:59, Lei Zhang wrote:
> We are going to record this on other platforms. Do we care to?

Should only record on Linux for now.

https://codereview.chromium.org/2398763002/diff/40001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:1322:
uma_.HistogramEnumeration("PDF.IsFontSubstituted", 1, 2);
On 2016/10/11 22:35:59, Lei Zhang wrote:
> Do we want constants instead of 1 and 2? (and 0 above)

They represent booleans, so I don't think that constants are necessary here...

https://codereview.chromium.org/2398763002/diff/40001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2398763002/diff/40001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2455: g_client_for_fontmapper_ = client_;
On 2016/10/11 22:35:59, Lei Zhang wrote:
> I'm worried we set this but never unset it. There is only 1 PDF plugin
process,
> and it contains multiple engines. If engine A reaches here first, and then
> engine B reaches here, and then engine B gets destroyed... we can end up with
a
> dangling pointer if engine A then reaches MapFont() somehow.

I see that. But this is how g_last_instance_id works. Maybe the best way is to
set - unset the client on the same method.

[Lei Zhang, 2016-10-12 18:46:58]

https://codereview.chromium.org/2398763002/diff/40001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2398763002/diff/40001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2455: g_client_for_fontmapper_ = client_;
On 2016/10/12 18:19:52, npm wrote:
> On 2016/10/11 22:35:59, Lei Zhang wrote:
> > I'm worried we set this but never unset it. There is only 1 PDF plugin
> process,
> > and it contains multiple engines. If engine A reaches here first, and then
> > engine B reaches here, and then engine B gets destroyed... we can end up
with
> a
> > dangling pointer if engine A then reaches MapFont() somehow.
> 
> I see that. But this is how g_last_instance_id works. Maybe the best way is to
> set - unset the client on the same method.

However, |g_last_instance_id| is an int, and having an invalid value does not
cause potential use-after-frees.

[Lei Zhang, 2016-10-12 18:56:38]

lgtm

https://codereview.chromium.org/2398763002/diff/80001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2398763002/diff/80001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:1321: if (font_substitution_reported_)
Can we have a matching #if defined() here? Also for
|font_substitution_reported_| as well. Maybe if the build tools see an empty
virtual function, they will be smart enough to optimize it away.

https://codereview.chromium.org/2398763002/diff/80001/pdf/pdfium/pdfium_engine.h
File pdf/pdfium/pdfium_engine.h (right):

https://codereview.chromium.org/2398763002/diff/80001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.h:757: };
Both of the scopers can use a DISALLOW_COPY_AND_ASSIGN().

[npm, 2016-10-12 19:36:30]

The CQ bit was checked by npm@chromium.org

[npm, 2016-10-12 19:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2398763002/#ps100001
(title: "Comments")

[commit-bot: I haz the power, 2016-10-12 19:36:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 20:19:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 20:20:00]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[npm, 2016-10-12 20:29:58]

The CQ bit was checked by npm@chromium.org

[npm, 2016-10-12 20:29:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2398763002/#ps120001
(title: "No ScopedSubstFont in PDFiumEngineExports")

[commit-bot: I haz the power, 2016-10-12 20:30:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 22:05:57]

Description was changed from

==========
Add font map user metrics for Linux.

The method MapFont, defined in pdfium_engine.cc, is used by Chrome on
Linux to find a substitute font whenever a pdf file uses a font which is
not embedded (or could not be loaded). When this method is called, the
method UserMetricsRecordAction will now be called to record that this
document has requested a substitute font.

Adding similar functionality for other OS's will take more effort just
because the MapFont definitions are completely inside pdfium core.

BUG=chromium:448584
==========

to

==========
Add font map user metrics for Linux.

The method MapFont, defined in pdfium_engine.cc, is used by Chrome on
Linux to find a substitute font whenever a pdf file uses a font which is
not embedded (or could not be loaded). When this method is called, the
method UserMetricsRecordAction will now be called to record that this
document has requested a substitute font.

Adding similar functionality for other OS's will take more effort just
because the MapFont definitions are completely inside pdfium core.

BUG=chromium:448584
==========

[commit-bot: I haz the power, 2016-10-12 22:05:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-12 22:08:32]

Description was changed from

==========
Add font map user metrics for Linux.

The method MapFont, defined in pdfium_engine.cc, is used by Chrome on
Linux to find a substitute font whenever a pdf file uses a font which is
not embedded (or could not be loaded). When this method is called, the
method UserMetricsRecordAction will now be called to record that this
document has requested a substitute font.

Adding similar functionality for other OS's will take more effort just
because the MapFont definitions are completely inside pdfium core.

BUG=chromium:448584
==========

to

==========
Add font map user metrics for Linux.

The method MapFont, defined in pdfium_engine.cc, is used by Chrome on
Linux to find a substitute font whenever a pdf file uses a font which is
not embedded (or could not be loaded). When this method is called, the
method UserMetricsRecordAction will now be called to record that this
document has requested a substitute font.

Adding similar functionality for other OS's will take more effort just
because the MapFont definitions are completely inside pdfium core.

BUG=chromium:448584

Committed: https://crrev.com/d24ca0432921fdf310c781c3a63aac7387306a9f
Cr-Commit-Position: refs/heads/master@{#424877}
==========

[commit-bot: I haz the power, 2016-10-12 22:08:34]

Patchset 7 (id:??) landed as
https://crrev.com/d24ca0432921fdf310c781c3a63aac7387306a9f
Cr-Commit-Position: refs/heads/master@{#424877}