Material PDF: Add metrics to count usage of PDF title and bookmarks

Material PDF: Add metrics to count usage of PDF title and bookmarks

Support for these metadata fields was added recently, this will allow us to
determine how commonly these features are used.

BUG=439114
Committed: https://crrev.com/f3f7317c276814ad1774776edfcfbc9e94a184bd
Cr-Commit-Position: refs/heads/master@{#354413}

[tsergeant, 2015-10-13 00:46:13]

tsergeant@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2015-10-13 05:27:54]

lgtm

https://codereview.chromium.org/1403833002/diff/20001/tools/metrics/actions/e...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1403833002/diff/20001/tools/metrics/actions/e...
tools/metrics/actions/extract_actions.py:224: actions.add('PDF_Unsupported_XFA')
Hmm can we remove the ones that are now unused?

[Alexei Svitkine (slow), 2015-10-13 16:44:25]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-10-13 16:44:25]

What's the reason for logging these as actions as opposed to a histogram?

Actions have timestamp information, which is useful for sequence analysis, but
is less efficient. If you just want to see counts/user counts, an enumerated
histogram is preferable.

[tsergeant, 2015-10-14 02:28:48]

Done, the use of actions was mostly just because the surrounding code uses
actions. Thanks for the advice!

Raymes, do you mind taking another quick look?

https://codereview.chromium.org/1403833002/diff/20001/tools/metrics/actions/e...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1403833002/diff/20001/tools/metrics/actions/e...
tools/metrics/actions/extract_actions.py:224: actions.add('PDF_Unsupported_XFA')
On 2015/10/13 at 05:27:54, raymes wrote:
> Hmm can we remove the ones that are now unused?

isherman@, what is the procedure for removing actions from this file? If I mark
an action as obsolete in the xml file, does it need to stay in this file as
well?

[tsergeant, 2015-10-14 02:36:50]

https://codereview.chromium.org/1403833002/diff/20001/tools/metrics/actions/e...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1403833002/diff/20001/tools/metrics/actions/e...
tools/metrics/actions/extract_actions.py:224: actions.add('PDF_Unsupported_XFA')
On 2015/10/14 at 02:28:48, tsergeant wrote:
> On 2015/10/13 at 05:27:54, raymes wrote:
> > Hmm can we remove the ones that are now unused?
> 
> isherman@, what is the procedure for removing actions from this file? If I
mark an action as obsolete in the xml file, does it need to stay in this file as
well?

Ah, whoops, that was a draft from yesterday. Same question goes to asvitkine@, I
guess =)

[raymes, 2015-10-14 02:39:08]

pdf/out_of_process_instance.cc lgtm

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

https://codereview.chromium.org/1403833002/diff/40001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:1117: if (has_title) {
nit: don't need {

[Alexei Svitkine (slow), 2015-10-14 15:01:50]

https://codereview.chromium.org/1403833002/diff/20001/tools/metrics/actions/e...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1403833002/diff/20001/tools/metrics/actions/e...
tools/metrics/actions/extract_actions.py:224: actions.add('PDF_Unsupported_XFA')
On 2015/10/14 02:36:50, tsergeant wrote:
> On 2015/10/14 at 02:28:48, tsergeant wrote:
> > On 2015/10/13 at 05:27:54, raymes wrote:
> > > Hmm can we remove the ones that are now unused?
> > 
> > isherman@, what is the procedure for removing actions from this file? If I
> mark an action as obsolete in the xml file, does it need to stay in this file
as
> well?
> 
> Ah, whoops, that was a draft from yesterday. Same question goes to asvitkine@,
I
> guess =)

Feel free to remove obsolete entries from this list as long as the <obsolete>
entries are still in the xml file.

[Alexei Svitkine (slow), 2015-10-14 15:06:10]

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

https://codereview.chromium.org/1403833002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30831: +    Tracks which features are
used by documents opened in the PDF viewer.
Nit: Mention when it's logged.

https://codereview.chromium.org/1403833002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:78410: +  <suffix name="Bookmarks"
label="PDF document has a table of contents."/>
Hmm, while this way of doing it is fine, I wonder if using a single enumerated
histogram with an enum value for each feature would be sufficient as well? If it
would, I think that would be a better approach since it would be simpler and
more efficient.

You can have an enum entry in that histogram that is recorded unconditionally in
the same place, which can be used as a baseline.

[tsergeant, 2015-10-15 02:56:40]

Patchset #5 (id:80001) has been deleted

[tsergeant, 2015-10-15 03:00:37]

https://codereview.chromium.org/1403833002/diff/110004/pdf/out_of_process_ins...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/1403833002/diff/110004/pdf/out_of_process_ins...
pdf/out_of_process_instance.cc:1111: UserMetricsRecordAction("PDF.LoadSuccess");
This action is now obsolete, but I'll follow up with another CL to clean up the
actions.

[Alexei Svitkine (slow), 2015-10-15 19:12:30]

lgtm

[tsergeant, 2015-10-15 23:14:44]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2015-10-15 23:14:44]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1403833002/#ps110004
(title: "Use a single histogram")

[commit-bot: I haz the power, 2015-10-15 23:15:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403833002/110004
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403833002/110004

[commit-bot: I haz the power, 2015-10-16 00:27:15]

Committed patchset #6 (id:110004)

[commit-bot: I haz the power, 2015-10-16 00:27:56]

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