Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
399063004:
Check whether the plugin is disabled in OnIsInternalPluginRegisteredForMimeType(). (Closed)
|
Created:
6 years, 5 months ago by xhwang Modified:
6 years, 5 months ago CC:
chromium-reviews, stuartmorgan+watch_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionCheck whether the plugin is disabled in OnIsInternalPluginRegisteredForMimeType().
BUG=395294
TEST=Manually tested disabling the plugin and canPlayType() returns "".
R=ddorwin@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284753
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase only #Patch Set 3 : add UMA #
Total comments: 8
Patch Set 4 : comments addressed #
Total comments: 7
Patch Set 5 : comments addressed #
Total comments: 14
Patch Set 6 : comments addressed #
Messages
Total messages: 27 (0 generated)
This Context class has some similar but more complex functions defined which I don't know how to use. So I just defined a simple one that I need. PTAL
LGTM. Question/suggestion. https://codereview.chromium.org/399063004/diff/1/chrome/browser/plugins/plugi... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/399063004/diff/1/chrome/browser/plugins/plugi... chrome/browser/plugins/plugin_info_message_filter.cc:182: continue; Should we UMA this? Really, we'd want to put this at 187, though. WDYT?
rebase only
bauerb: Please OWNERS review!
https://codereview.chromium.org/399063004/diff/1/chrome/browser/plugins/plugi... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/399063004/diff/1/chrome/browser/plugins/plugi... chrome/browser/plugins/plugin_info_message_filter.cc:182: continue; On 2014/07/18 00:15:22, ddorwin wrote: > Should we UMA this? Really, we'd want to put this at 187, though. WDYT? Do you mean to put UMA after 187? That sgtm. But with the other CL, this could be called multiple times, so we have to think through about what we are measuring...
comments addressed. PTAL again!
isherman@chromium.org: Please review changes in tools/metrics/histograms/histograms.xml
LGTM % comments in 2 files. https://codereview.chromium.org/399063004/diff/40001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/399063004/diff/40001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:183: // Only report results for Widevine CDM. #if defined(WIDEVINE_CDM_AVAILABLE) && defined(ENABLE_PEPPER_CDMS) https://codereview.chromium.org/399063004/diff/40001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:205: if (!context_.IsPluginEnabled(plugins[i])) { nit: With all the nesting and complexity, maybe we should have a local var for "plugin". https://codereview.chromium.org/399063004/diff/40001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:221: mime_type, is_plugin_disabled ? PLUGIN_DISABLED : PLUGIN_NOT_REGISTERED); For the record, PLUGIN_DISABLED will not be sent if there happened to be another plugin registered for this mime type that was not disabled. (No action required.) https://codereview.chromium.org/399063004/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/399063004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:22045: + enum="PluginRegistratinoStatus"> Registration
comments addressed
https://codereview.chromium.org/399063004/diff/40001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/399063004/diff/40001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:183: // Only report results for Widevine CDM. On 2014/07/18 23:50:18, ddorwin wrote: > #if defined(WIDEVINE_CDM_AVAILABLE) && defined(ENABLE_PEPPER_CDMS) Done. https://codereview.chromium.org/399063004/diff/40001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:205: if (!context_.IsPluginEnabled(plugins[i])) { On 2014/07/18 23:50:18, ddorwin wrote: > nit: With all the nesting and complexity, maybe we should have a local var for > "plugin". Done. https://codereview.chromium.org/399063004/diff/40001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:221: mime_type, is_plugin_disabled ? PLUGIN_DISABLED : PLUGIN_NOT_REGISTERED); On 2014/07/18 23:50:18, ddorwin wrote: > For the record, PLUGIN_DISABLED will not be sent if there happened to be another > plugin registered for this mime type that was not disabled. (No action > required.) Acknowledged. https://codereview.chromium.org/399063004/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/399063004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:22045: + enum="PluginRegistratinoStatus"> On 2014/07/18 23:50:18, ddorwin wrote: > Registration Done.
LGTM. Thanks.
https://codereview.chromium.org/399063004/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/399063004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:22047: + <summary>The registration status of Widevine CDM.</summary> Please document when this metric is recorded.
https://codereview.chromium.org/399063004/diff/60001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/399063004/diff/60001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:181: static void SendPluginRegistrationUMA(const std::string& mime_type, Nit: Move this into an anonymous namespace. https://codereview.chromium.org/399063004/diff/60001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:183: #if defined(WIDEVINE_CDM_AVAILABLE) && defined(ENABLE_PEPPER_CDMS) The second #defined test is unnecessary if this is already inside of a #if defined(ENABLE_PEPPER_CDMS) block. https://codereview.chromium.org/399063004/diff/60001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:193: void PluginInfoMessageFilter::OnIsInternalPluginRegisteredForMimeType( Do we want to rename this method now that it checks not only for the plugin being registered, but also enabled?
comments addressed
I believe I address all comments. Please take a look again and let me know what you think. https://codereview.chromium.org/399063004/diff/60001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/399063004/diff/60001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:181: static void SendPluginRegistrationUMA(const std::string& mime_type, On 2014/07/21 08:29:58, Bernhard Bauer wrote: > Nit: Move this into an anonymous namespace. Done. https://codereview.chromium.org/399063004/diff/60001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:183: #if defined(WIDEVINE_CDM_AVAILABLE) && defined(ENABLE_PEPPER_CDMS) On 2014/07/21 08:29:58, Bernhard Bauer wrote: > The second #defined test is unnecessary if this is already inside of a #if > defined(ENABLE_PEPPER_CDMS) block. Done. https://codereview.chromium.org/399063004/diff/60001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:193: void PluginInfoMessageFilter::OnIsInternalPluginRegisteredForMimeType( On 2014/07/21 08:29:58, Bernhard Bauer wrote: > Do we want to rename this method now that it checks not only for the plugin > being registered, but also enabled? Renamed "Registered" to "Available". "Available" means "registered" and "not-disabled".
Minor comments. https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:68: PLUGIN_AVAILABLE, nit: Should these and the histogram values be in a more logical order? DISABLED seems to come in between, though it might be the least likely. NOT_REGISTERED is probably the initial state, so it might make sense as first. Then AVAILABLE then DISABLED? https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_info_message_filter.h (right): https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.h:104: // Returns whether any internal plugin supporting |mime_type| is registered. ... and enabled. https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.h:107: // When the returned *|is_registered| is true, |additional_param_names| and ditto from 112 https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.h:109: // for the *first* plugin found that is registered for |mime_type|. ditto from 104 https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.h:112: bool* is_registered, is_available https://codereview.chromium.org/399063004/diff/80001/chrome/common/render_mes... File chrome/common/render_messages.h (right): https://codereview.chromium.org/399063004/diff/80001/chrome/common/render_mes... chrome/common/render_messages.h:433: // Returns whether any internal plugin supporting |mime_type| is available. Should we update these comments per my suggestions in the previous header? https://codereview.chromium.org/399063004/diff/80001/chrome/common/render_mes... chrome/common/render_messages.h:442: bool /* available */, nit: is_available?
comments addressed
Thanks for the comments. PTAL again. https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.cc:68: PLUGIN_AVAILABLE, On 2014/07/21 17:54:57, ddorwin wrote: > nit: Should these and the histogram values be in a more logical order? DISABLED > seems to come in between, though it might be the least likely. > > NOT_REGISTERED is probably the initial state, so it might make sense as first. > Then AVAILABLE then DISABLED? Done. https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_info_message_filter.h (right): https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.h:104: // Returns whether any internal plugin supporting |mime_type| is registered. On 2014/07/21 17:54:57, ddorwin wrote: > ... and enabled. Done. https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.h:107: // When the returned *|is_registered| is true, |additional_param_names| and On 2014/07/21 17:54:58, ddorwin wrote: > ditto from 112 Done. https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.h:109: // for the *first* plugin found that is registered for |mime_type|. On 2014/07/21 17:54:57, ddorwin wrote: > ditto from 104 Done. https://codereview.chromium.org/399063004/diff/80001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_info_message_filter.h:112: bool* is_registered, On 2014/07/21 17:54:57, ddorwin wrote: > is_available Done. https://codereview.chromium.org/399063004/diff/80001/chrome/common/render_mes... File chrome/common/render_messages.h (right): https://codereview.chromium.org/399063004/diff/80001/chrome/common/render_mes... chrome/common/render_messages.h:433: // Returns whether any internal plugin supporting |mime_type| is available. On 2014/07/21 17:54:58, ddorwin wrote: > Should we update these comments per my suggestions in the previous header? Done. https://codereview.chromium.org/399063004/diff/80001/chrome/common/render_mes... chrome/common/render_messages.h:442: bool /* available */, On 2014/07/21 17:54:58, ddorwin wrote: > nit: is_available? Done.
Histograms LGTM, thanks.
LGTM!
dcheng@chromium.org: Please review changes in chrome/common/render_messages.h It's a renaming change and should be very easy to review.
-dcheng since he's OOO +cevans: Please OWNERS review changes in chrome/common/render_messages.h It's a renaming change and should be very easy to review.
On 2014/07/22 16:33:15, xhwang wrote: > -dcheng since he's OOO > > +cevans: Please OWNERS review changes in > > chrome/common/render_messages.h > > It's a renaming change and should be very easy to review. chrome/common/render_messages.h lgtm
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/399063004/100001
Message was sent while issue was closed.
Change committed as 284753 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
