CdmAdapter: Report size of the file read by CDM via FileIO.

CdmAdapter: Report size of the file read by CDM via FileIO.

We report when:
1, The first time a file is fully read.
2, After the CDM throws an error with a special system code.

BUG=413812
TEST=Tested and both new UMA show in about://histograms.

Committed: https://crrev.com/fe40560f991d5334aa5bae4f1d527662b76a99e8
Cr-Commit-Position: refs/heads/master@{#294843}

[xhwang, 2014-09-12 00:17:31]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org

[xhwang, 2014-09-12 00:17:32]

PTAL

[ddorwin, 2014-09-12 02:26:28]

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:24: const uint32_t kSizeKBMin = 1;
What about 0? There can be empty files. Or maybe we should exclude those because
they are non-interesting. Well, we can always do that manually in the viewer.

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:1108: last_read_file_size_kb_ = file_size_bytes /
1000;
Why 1000 instead of 1024?  Your max above would be 512 * 1024 * 1000 bytes.

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_file_io_...
File media/cdm/ppapi/cdm_file_io_impl.h (right):

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_file_io_...
media/cdm/ppapi/cdm_file_io_impl.h:40: const pp::CompletionCallback&
last_read_file_size_kb_cb);
s/kb/bytes/ - the value passed is bytes - this code doesn't care how it's being
reported.
Actually, it doesn't really matter that it's last read either.
read_complete_cb? With a comment that that the value passed is the number of
bytes read.

[xhwang, 2014-09-12 04:30:12]

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

[xhwang, 2014-09-12 04:30:13]

ddorwin: PTAL again.

isherman: Please OWNERS review histograms.xml.

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:24: const uint32_t kSizeKBMin = 1;
On 2014/09/12 02:26:28, ddorwin wrote:
> What about 0? There can be empty files. Or maybe we should exclude those
because
> they are non-interesting. Well, we can always do that manually in the viewer.

https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...

The minimum allowed "min" is 1. Zero will be implicitly added as the first
range.

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:1108: last_read_file_size_kb_ = file_size_bytes /
1000;
On 2014/09/12 02:26:28, ddorwin wrote:
> Why 1000 instead of 1024?  Your max above would be 512 * 1024 * 1000 bytes.

lol, good catch! Fixed.

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_file_io_...
File media/cdm/ppapi/cdm_file_io_impl.h (right):

https://codereview.chromium.org/568623003/diff/1/media/cdm/ppapi/cdm_file_io_...
media/cdm/ppapi/cdm_file_io_impl.h:40: const pp::CompletionCallback&
last_read_file_size_kb_cb);
On 2014/09/12 02:26:28, ddorwin wrote:
> s/kb/bytes/ - the value passed is bytes - this code doesn't care how it's
being
> reported.
> Actually, it doesn't really matter that it's last read either.
> read_complete_cb? With a comment that that the value passed is the number of
> bytes read.

I renamed all other places but forgot this one. Renamed to file_read_cb.

[ddorwin, 2014-09-12 05:44:30]

LGTM with suggestion

https://codereview.chromium.org/568623003/diff/20001/media/cdm/ppapi/cdm_file...
File media/cdm/ppapi/cdm_file_io_impl.h (right):

https://codereview.chromium.org/568623003/diff/20001/media/cdm/ppapi/cdm_file...
media/cdm/ppapi/cdm_file_io_impl.h:40: const pp::CompletionCallback&
file_read_cb);
Comment that the CB takes a parameter that is the number of bytes?

[xhwang, 2014-09-12 17:47:32]

comments addressed

[xhwang, 2014-09-12 17:48:45]

https://codereview.chromium.org/568623003/diff/20001/media/cdm/ppapi/cdm_file...
File media/cdm/ppapi/cdm_file_io_impl.h (right):

https://codereview.chromium.org/568623003/diff/20001/media/cdm/ppapi/cdm_file...
media/cdm/ppapi/cdm_file_io_impl.h:40: const pp::CompletionCallback&
file_read_cb);
On 2014/09/12 05:44:29, ddorwin wrote:
> Comment that the CB takes a parameter that is the number of bytes?

Done.

[Ilya Sherman, 2014-09-12 19:40:44]

histograms LGTM

[xhwang, 2014-09-12 23:03:43]

rebase only

[xhwang, 2014-09-12 23:03:55]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-09-12 23:12:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568623003/60001

[commit-bot: I haz the power, 2014-09-13 01:55:40]

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

[commit-bot: I haz the power, 2014-09-13 01:55:41]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[xhwang, 2014-09-14 03:59:21]

The callback created by CompletionCallbackFactory is for single-use only:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/utility/comp...

So I updated this CL so that CdmFileIOImpl only reports the file size of the
first read. This should be fine for our use cases.

ddorwin: PTAL again.

[ddorwin, 2014-09-14 21:48:00]

LGTM % comment

https://codereview.chromium.org/568623003/diff/80001/media/cdm/ppapi/cdm_file...
File media/cdm/ppapi/cdm_file_io_impl.h (right):

https://codereview.chromium.org/568623003/diff/80001/media/cdm/ppapi/cdm_file...
media/cdm/ppapi/cdm_file_io_impl.h:42: const pp::CompletionCallback&
file_read_cb);
rename the cb here too?

[xhwang, 2014-09-15 03:44:10]

comments addressed

[xhwang, 2014-09-15 03:44:31]

https://codereview.chromium.org/568623003/diff/80001/media/cdm/ppapi/cdm_file...
File media/cdm/ppapi/cdm_file_io_impl.h (right):

https://codereview.chromium.org/568623003/diff/80001/media/cdm/ppapi/cdm_file...
media/cdm/ppapi/cdm_file_io_impl.h:42: const pp::CompletionCallback&
file_read_cb);
On 2014/09/14 21:47:59, ddorwin wrote:
> rename the cb here too?

Done.

[xhwang, 2014-09-15 03:44:38]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-09-15 03:44:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568623003/100001

[commit-bot: I haz the power, 2014-09-15 05:01:07]

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

[commit-bot: I haz the power, 2014-09-15 05:01:09]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[xhwang, 2014-09-15 16:09:06]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-09-15 16:09:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568623003/100001

[commit-bot: I haz the power, 2014-09-15 17:24:05]

Committed patchset #6 (id:100001) as 64e253e1d88d88ceb73f1e44a138685bcaffa69b

[commit-bot: I haz the power, 2014-09-15 17:28:54]

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