Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
281853003:
Whitelist libwidevinecdmadapter.so for pepper UMA reporting. (Closed)
|
Created:
6 years, 7 months ago by xhwang Modified:
5 years, 6 months ago Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWhitelist libwidevinecdmadapter.so for pepper UMA reporting.
BUG=368743
TEST=Media.EME.OutputProtection appears in about://histogram
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270616
Patch Set 1 #
Total comments: 11
Patch Set 2 : comments addressed #Patch Set 3 : rebase only #
Total comments: 2
Patch Set 4 : comments addressed #Messages
Total messages: 17 (0 generated)
PTAL
Does it make more sense to use a prefix and reuse the existing infrastructure rather than adding more? LGTM, but I defer to the owners. https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_uma_host.cc (right): https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.cc:36: "libwidevinecdmadapter.so", // see http://crbug.com/368743 Could/should we just add a histogram prefix?
LGTM, but please get isherman to agree to this too. You will also need a Pepper owner, maybe bbudge would be a good choice (he reviewed this interface originally for me a few months ago) https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_uma_host.cc (right): https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.cc:36: "libwidevinecdmadapter.so", // see http://crbug.com/368743 I'm assuming this is chromeos only or something? https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_uma_host.h (right): https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.h:67: base::FilePath plugin_file_name_; nit: it's a basename, not a filename https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.h:74: std::set<std::string> allowed_plugin_file_names_; same comment about basename
comment only https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_uma_host.cc (right): https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.cc:36: "libwidevinecdmadapter.so", // see http://crbug.com/368743 On 2014/05/14 17:17:21, elijahtaylor1 wrote: > I'm assuming this is chromeos only or something? This is the name on linux and cros, and I only care about cros actually.
bbudge@: Please OWNERS review this CL.
OWNERS lgtm w/nit https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_uma_host.h (right): https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.h:74: std::set<std::string> allowed_plugin_file_names_; nit: base::hash_set
https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_uma_host.cc (right): https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.cc:36: "libwidevinecdmadapter.so", // see http://crbug.com/368743 On 2014/05/14 17:41:28, xhwang wrote: > On 2014/05/14 17:17:21, elijahtaylor1 wrote: > > I'm assuming this is chromeos only or something? > > This is the name on linux and cros, and I only care about cros actually. Should we ifdef it to make this clear? Other platforms would just have an empty array (that might cause problems for arraysize, though).
comments addressed
https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_uma_host.cc (right): https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.cc:36: "libwidevinecdmadapter.so", // see http://crbug.com/368743 On 2014/05/14 19:41:19, ddorwin wrote: > On 2014/05/14 17:41:28, xhwang wrote: > > On 2014/05/14 17:17:21, elijahtaylor1 wrote: > > > I'm assuming this is chromeos only or something? > > > > This is the name on linux and cros, and I only care about cros actually. > > Should we ifdef it to make this clear? Other platforms would just have an empty > array (that might cause problems for arraysize, though). Yeah, arraysize doesn't work with empty array. I'll just keep this as is. https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_uma_host.h (right): https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.h:67: base::FilePath plugin_file_name_; On 2014/05/14 17:17:21, elijahtaylor1 wrote: > nit: it's a basename, not a filename Done. https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.h:74: std::set<std::string> allowed_plugin_file_names_; On 2014/05/14 17:17:21, elijahtaylor1 wrote: > same comment about basename Done. https://codereview.chromium.org/281853003/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_uma_host.h:74: std::set<std::string> allowed_plugin_file_names_; On 2014/05/14 19:31:00, bbudge wrote: > nit: base::hash_set Done. I keep the above two as std::set because (1) I want my change to be small so it's easier to merge (2) ChromeContentRendererClient::IsExtensionOrSharedModuleWhitelisted() takes a std::set<string>
isherman: Do you also want to take a look?
rebase only
LGTM https://codereview.chromium.org/281853003/diff/40001/chrome/renderer/pepper/p... File chrome/renderer/pepper/pepper_uma_host.h (right): https://codereview.chromium.org/281853003/diff/40001/chrome/renderer/pepper/p... chrome/renderer/pepper/pepper_uma_host.h:75: base::hash_set<std::string> allowed_plugin_base_names_; nit: bbudge, why did you recommend base::hash_set over std::set? Do the performance characteristics really matter?
comments addressed
https://codereview.chromium.org/281853003/diff/40001/chrome/renderer/pepper/p... File chrome/renderer/pepper/pepper_uma_host.h (right): https://codereview.chromium.org/281853003/diff/40001/chrome/renderer/pepper/p... chrome/renderer/pepper/pepper_uma_host.h:75: base::hash_set<std::string> allowed_plugin_base_names_; On 2014/05/14 21:52:51, Ilya Sherman wrote: > nit: bbudge, why did you recommend base::hash_set over std::set? Do the > performance characteristics really matter? Reverted back to std::set for consistency with the rest of the code. Since this map is super small, I don't expect any real performance difference.
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/281853003/60001
Message was sent while issue was closed.
Change committed as 270616 |
