PDF: Add UMA to track successful / failed PDF loads

PDF: Add UMA to track successful / failed PDF loads

This CL covers successful loads of the internal PDF plugin, as well as
when we show the plugin placeholder for the PDF (when user is on Android
or the plugin is disabled and there is no fallback content).

This CL does not cover the drive-by download or main-frame download
cases. Those will be in a followup CL.

BUG=714917
Review-Url: https://codereview.chromium.org/2852193002
Cr-Commit-Position: refs/heads/master@{#470130}
Committed: https://chromium.googlesource.com/chromium/src/+/e57c73a2db2ce7fad847d8663748d387fe545390

[tommycli, 2017-05-01 22:08:09]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 22:09:29]

Dry run: CQ is trying da patch.

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

[tommycli, 2017-05-01 22:25:21]

tommycli@chromium.org changed reviewers:
+ thestig@chromium.org

[tommycli, 2017-05-01 22:25:22]

thestig: PTAL, this is the UMA we discussed previously. Both Android frontend
and Media PMs thought this would be a worthwhile thing to track.

I've manually tested all the cases in this CL using
http://chrome-pdf-test.appspot.com/ with the Open PDF externally pref on and
off.

Thanks!

[tommycli, 2017-05-03 15:28:47]

thestig: ping, thanks!

[Lei Zhang, 2017-05-03 23:50:35]

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:25: #include
"chrome/common/chrome_content_client.h"
Remove existing entry from line 158 then?

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:818: // extension that loads
another plugin, make sure we don't double count.
extension that loads a second plugin, avoid double counting by ignoring the
creation of the second plugin.

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:823:
frame->View()->MainFrame()->IsWebLocalFrame() &&
I think the following does the same, without having to involve WebView.

render_frame->IsMainFrame() &&
render_frame->GetWebFrame()->GetDocument().IsPluginDocument()

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_uma.h (right):

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma.h:72: static void
ReportPDFLoadStatus(PDFLoadStatus status);
Keep the static methods together.

[tommycli, 2017-05-04 00:08:31]

thanks!

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:25: #include
"chrome/common/chrome_content_client.h"
On 2017/05/03 23:50:34, Lei Zhang wrote:
> Remove existing entry from line 158 then?

Done.

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:818: // extension that loads
another plugin, make sure we don't double count.
On 2017/05/03 23:50:34, Lei Zhang wrote:
> extension that loads a second plugin, avoid double counting by ignoring the
> creation of the second plugin.

Done.

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:823:
frame->View()->MainFrame()->IsWebLocalFrame() &&
On 2017/05/03 23:50:34, Lei Zhang wrote:
> I think the following does the same, without having to involve WebView.
> 
> render_frame->IsMainFrame() &&
> render_frame->GetWebFrame()->GetDocument().IsPluginDocument()

Done.

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_uma.h (right):

https://codereview.chromium.org/2852193002/diff/20001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma.h:72: static void
ReportPDFLoadStatus(PDFLoadStatus status);
On 2017/05/03 23:50:34, Lei Zhang wrote:
> Keep the static methods together.

Done.

[tommycli, 2017-05-04 00:11:29]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 00:12:08]

Dry run: CQ is trying da patch.

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

[Lei Zhang, 2017-05-04 00:18:17]

Need a histogram OWNER, BTW.

[Lei Zhang, 2017-05-04 00:19:11]

lgtm

https://codereview.chromium.org/2852193002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2852193002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:124: #include
"third_party/WebKit/public/web/WebView.h"
No longer needed?

[commit-bot: I haz the power, 2017-05-04 01:14:09]

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

[commit-bot: I haz the power, 2017-05-04 01:14:10]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-05-08 16:17:52]

thanks lei!

https://codereview.chromium.org/2852193002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2852193002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:124: #include
"third_party/WebKit/public/web/WebView.h"
On 2017/05/04 00:19:11, Lei Zhang wrote:
> No longer needed?

Done.

[tommycli, 2017-05-08 16:18:25]

tommycli@chromium.org changed reviewers:
+ isherman@chromium.org

[tommycli, 2017-05-08 16:18:25]

isherman: histograms PTAL, thanks!

[tommycli, 2017-05-08 17:52:16]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 17:53:20]

Dry run: CQ is trying da patch.

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

[Ilya Sherman, 2017-05-08 17:59:57]

Metrics LGTM % nits:

https://codereview.chromium.org/2852193002/diff/60001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_uma.h (right):

https://codereview.chromium.org/2852193002/diff/60001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma.h:47: // Must be kept in sync with
PDFLoadStatus enum in histograms.xml.
Please also document that this enum should be treated as append-only, since we
want time-invariant semantics for the histogram buckets.

https://codereview.chromium.org/2852193002/diff/60001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma.h:52: PDF_LOAD_STATUS_MAX
Optional nit: Typically, "max" is used as an alias for the last item in the
enum, whereas "count" is used to count the number of valid entries in the enum. 
So, I'd recommend "count" here.

[tommycli, 2017-05-08 18:16:17]

https://codereview.chromium.org/2852193002/diff/60001/chrome/renderer/plugins...
File chrome/renderer/plugins/plugin_uma.h (right):

https://codereview.chromium.org/2852193002/diff/60001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma.h:47: // Must be kept in sync with
PDFLoadStatus enum in histograms.xml.
On 2017/05/08 17:59:57, Ilya Sherman wrote:
> Please also document that this enum should be treated as append-only, since we
> want time-invariant semantics for the histogram buckets.

Done.

https://codereview.chromium.org/2852193002/diff/60001/chrome/renderer/plugins...
chrome/renderer/plugins/plugin_uma.h:52: PDF_LOAD_STATUS_MAX
On 2017/05/08 17:59:57, Ilya Sherman wrote:
> Optional nit: Typically, "max" is used as an alias for the last item in the
> enum, whereas "count" is used to count the number of valid entries in the
enum. 
> So, I'd recommend "count" here.

Done.

[tommycli, 2017-05-08 18:17:03]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-05-08 18:17:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2852193002/#ps80001
(title: "change to count")

[commit-bot: I haz the power, 2017-05-08 18:18:11]

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, 2017-05-08 21:18:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494267423373630,
"parent_rev": "9affbc15342e08bc0162756c9d1ec32fdf492fab", "commit_rev":
"e57c73a2db2ce7fad847d8663748d387fe545390"}

[commit-bot: I haz the power, 2017-05-08 21:18:25]

Description was changed from

==========
PDF: Add UMA to track successful / failed PDF loads

This CL covers successful loads of the internal PDF plugin, as well as
when we show the plugin placeholder for the PDF (when user is on Android
or the plugin is disabled and there is no fallback content).

This CL does not cover the drive-by download or main-frame download
cases. Those will be in a followup CL.

BUG=714917
==========

to

==========
PDF: Add UMA to track successful / failed PDF loads

This CL covers successful loads of the internal PDF plugin, as well as
when we show the plugin placeholder for the PDF (when user is on Android
or the plugin is disabled and there is no fallback content).

This CL does not cover the drive-by download or main-frame download
cases. Those will be in a followup CL.

BUG=714917

Review-Url: https://codereview.chromium.org/2852193002
Cr-Commit-Position: refs/heads/master@{#470130}
Committed:
https://chromium.googlesource.com/chromium/src/+/e57c73a2db2ce7fad847d8663748...
==========

[commit-bot: I haz the power, 2017-05-08 21:18:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e57c73a2db2ce7fad847d8663748...