|
|
Created:
4 years, 2 months ago by pastarmovj Modified:
4 years, 2 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, jam Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd migration code for the EnabledPlugins and DisabledPlugins policies.
These policies are deprecated in favor of:
DefaultPluginSettings (aka. Flash BLOCK/ALLOW)
AlwaysOpenPdfExternally (aka. PDF plugin disabled)
BUG=654731
TEST=browser_tests:PolicyPrefIndicator
Committed: https://crrev.com/c6c75ce329c9b4f9c925056d583dd9f1ae24a9d7
Cr-Commit-Position: refs/heads/master@{#425743}
Patch Set 1 #Patch Set 2 : Reduce code duplication. #Patch Set 3 : Remove the direct policy handling for the deprecated policies. #Patch Set 4 : Add handling for the Exceptions policy. #Patch Set 5 : Make "*" entries work as expected. #Patch Set 6 : Rebased on ToT. #Patch Set 7 : Fix copy/paste thingy that VC doesn't catch. #Patch Set 8 : Fix clang stuff. #Patch Set 9 : Fix platforms that has no plugins support. #Patch Set 10 : Adjust policy tests. #
Total comments: 7
Patch Set 11 : Address comments. #Patch Set 12 : Invert the matching. #
Total comments: 15
Patch Set 13 : Address comments. #Patch Set 14 : Remove case insensitivity. #
Messages
Total messages: 35 (22 generated)
The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #7 (id:120001) has been deleted
Description was changed from ========== Add migration code for the EnabledPlugins and DisabledPlugins policies. These policies are deprecated in favor of: DefaultPluginSettings (aka. Flash BLOCK/ALLOW) AlwaysOpenPdfExternally (aka. PDF plugin disabled) BUG=654731 ========== to ========== Add migration code for the EnabledPlugins and DisabledPlugins policies. These policies are deprecated in favor of: DefaultPluginSettings (aka. Flash BLOCK/ALLOW) AlwaysOpenPdfExternally (aka. PDF plugin disabled) BUG=654731 TEST=browser_tests:PolicyPrefIndicator ==========
The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pastarmovj@chromium.org changed reviewers: + bauerb@chromium.org
Hi Bernhard, Please take a look at this CL. Thanks! :) Cheers, Julian
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.cc (right): https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:26: return NULL; nullptr https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:94: // Finally check if any of the two is in the excpetions list and remove the Nit: "exceptions" https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.h (right): https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.h:25: // the new policies. Hm... wouldn't it actually be _easier_ to just match the pattern defined by policy against, say, "Shockwave Flash Player" and "PDF Viewer"? I get the feeling that would miss fewer edge cases, and be slightly less code as well.
Thanks for the review! PTAL. https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.cc (right): https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:26: return NULL; On 2016/10/12 15:28:49, Bernhard Bauer wrote: > nullptr Done. https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:94: // Finally check if any of the two is in the excpetions list and remove the On 2016/10/12 15:28:49, Bernhard Bauer wrote: > Nit: "exceptions" Done. https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.h (right): https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.h:25: // the new policies. On 2016/10/12 15:28:49, Bernhard Bauer wrote: > Hm... wouldn't it actually be _easier_ to just match the pattern defined by > policy against, say, "Shockwave Flash Player" and "PDF Viewer"? I get the > feeling that would miss fewer edge cases, and be slightly less code as well. We have been advising admins on various bugs to be broader in the old NPAPI days and use the wildcard specs to catch all version of particular plugins so I think it is expected that there is a fair number of wildcard policies out in the wild.
https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.h (right): https://codereview.chromium.org/2410113002/diff/200001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.h:25: // the new policies. On 2016/10/13 13:29:00, pastarmovj wrote: > On 2016/10/12 15:28:49, Bernhard Bauer wrote: > > Hm... wouldn't it actually be _easier_ to just match the pattern defined by > > policy against, say, "Shockwave Flash Player" and "PDF Viewer"? I get the > > feeling that would miss fewer edge cases, and be slightly less code as well. > > We have been advising admins on various bugs to be broader in the old NPAPI days > and use the wildcard specs to catch all version of particular plugins so I think > it is expected that there is a fair number of wildcard policies out in the wild. I don't doubt there are wildcard policies out in the wild (heh), but it appears to me the easiest way to check whether a given pattern is meant to match e.g. Flash would be... to match it against Flash, no? (Where "match" here means using the pattern semantics that we have defined in the policy.)
Inverted the logic as discussed yesterday. PTAL.
https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.cc (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:56: plugin = base::ToUpperASCII(plugin); I think this is also not necessary -- if MatchPattern() is case-insensitive, it wouldn't matter, and if it is case-sensitive, it would have never matched in the first place :) https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:57: if ((plugin == "*" || This is also not necessary -- * will match anything :) https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:67: base::MatchPattern(base::ToUpperASCII(kAdobeFlashPlayerName), For extra resilience, you could attempt to match content::kFlashPluginName as well -- that way you would catch both patterns matching the actual plugin name as well as the plugin group name. https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.h (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.h:22: // ignored. The string matching rules are not trying to catch edge cases like This comments needs updating. https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.h:29: ~PluginPolicyHandler() override; If you have a protected section anyway, move the destructor to it? This handler will be owned by ConfigurationPolicyHandlerList, which will own (and therefore destroy it) as the base class. https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/policy/... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/policy/... chrome/browser/policy/configuration_policy_handler_list_factory.cc:86: #if defined(ENABLE_PLUGINS) Move this before ENABLE_SPELLCHECK so that the #ifdef blocks are in alphabetical order?
https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.cc (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:56: plugin = base::ToUpperASCII(plugin); On 2016/10/14 11:49:45, Bernhard Bauer wrote: > I think this is also not necessary -- if MatchPattern() is case-insensitive, it > wouldn't matter, and if it is case-sensitive, it would have never matched in the > first place :) I am upper-casing the name below as well so it is upper case against upper case (MatchPattern is case sensitive). And our policy matching wasn't. https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:57: if ((plugin == "*" || On 2016/10/14 11:49:45, Bernhard Bauer wrote: > This is also not necessary -- * will match anything :) Right now that we rotated it around :) https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:67: base::MatchPattern(base::ToUpperASCII(kAdobeFlashPlayerName), On 2016/10/14 11:49:45, Bernhard Bauer wrote: > For extra resilience, you could attempt to match content::kFlashPluginName as > well -- that way you would catch both patterns matching the actual plugin name > as well as the plugin group name. Done. https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.h (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.h:22: // ignored. The string matching rules are not trying to catch edge cases like On 2016/10/14 11:49:46, Bernhard Bauer wrote: > This comments needs updating. Yes. No need for the explanation anymore. https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.h:29: ~PluginPolicyHandler() override; On 2016/10/14 11:49:46, Bernhard Bauer wrote: > If you have a protected section anyway, move the destructor to it? This handler > will be owned by ConfigurationPolicyHandlerList, which will own (and therefore > destroy it) as the base class. Nope. VC doesn't like it ninja -t msvc -e environment.x64 -- "C:\src\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\VC\bin\amd64/cl.exe" /nologo /showIncludes /FC @obj/chrome/browser/browser/configuration_policy_handler_list_factory.obj.rsp /c ../../chrome/browser/policy/configuration_policy_handler_list_factory.cc /Foobj/chrome/browser/browser/configuration_policy_handler_list_factory.obj /Fd"obj/chrome/browser/browser_cc.pdb" c:\src\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\vc\include\memory(1195): error C2248: 'PluginPolicyHandler::~PluginPolicyHandler': cannot access protected member declared in class 'PluginPolicyHandler' https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/policy/... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/policy/... chrome/browser/policy/configuration_policy_handler_list_factory.cc:86: #if defined(ENABLE_PLUGINS) On 2016/10/14 11:49:46, Bernhard Bauer wrote: > Move this before ENABLE_SPELLCHECK so that the #ifdef blocks are in alphabetical > order? Done.
https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.cc (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:56: plugin = base::ToUpperASCII(plugin); On 2016/10/14 12:38:59, pastarmovj wrote: > On 2016/10/14 11:49:45, Bernhard Bauer wrote: > > I think this is also not necessary -- if MatchPattern() is case-insensitive, > it > > wouldn't matter, and if it is case-sensitive, it would have never matched in > the > > first place :) > > I am upper-casing the name below as well so it is upper case against upper case > (MatchPattern is case sensitive). And our policy matching wasn't. Wasn't case sensitive? PluginPrefs::IsStringMatchedInSet() is. https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.h (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.h:29: ~PluginPolicyHandler() override; On 2016/10/14 12:38:59, pastarmovj wrote: > On 2016/10/14 11:49:46, Bernhard Bauer wrote: > > If you have a protected section anyway, move the destructor to it? This > handler > > will be owned by ConfigurationPolicyHandlerList, which will own (and therefore > > destroy it) as the base class. > > Nope. VC doesn't like it > > ninja -t msvc -e environment.x64 -- > "C:\src\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\VC\bin\amd64/cl.exe" > /nologo /showIncludes /FC > @obj/chrome/browser/browser/configuration_policy_handler_list_factory.obj.rsp /c > ../../chrome/browser/policy/configuration_policy_handler_list_factory.cc > /Foobj/chrome/browser/browser/configuration_policy_handler_list_factory.obj > /Fd"obj/chrome/browser/browser_cc.pdb" > c:\src\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\vc\include\memory(1195): > error C2248: 'PluginPolicyHandler::~PluginPolicyHandler': cannot access > protected member declared in class 'PluginPolicyHandler' Oh right. For posteriority, I think this is because the compiler will generate code that would destroy the unique_ptr, even though that code will never run because we will have passed the unique_ptr to somewhere else.
https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... File chrome/browser/plugins/plugin_policy_handler.cc (right): https://codereview.chromium.org/2410113002/diff/240001/chrome/browser/plugins... chrome/browser/plugins/plugin_policy_handler.cc:56: plugin = base::ToUpperASCII(plugin); On 2016/10/14 15:27:05, Bernhard Bauer wrote: > On 2016/10/14 12:38:59, pastarmovj wrote: > > On 2016/10/14 11:49:45, Bernhard Bauer wrote: > > > I think this is also not necessary -- if MatchPattern() is case-insensitive, > > it > > > wouldn't matter, and if it is case-sensitive, it would have never matched in > > the > > > first place :) > > > > I am upper-casing the name below as well so it is upper case against upper > case > > (MatchPattern is case sensitive). And our policy matching wasn't. > > Wasn't case sensitive? PluginPrefs::IsStringMatchedInSet() is. Seems like I might be remembering wrong. Removed case insenstivity from my checks too.
LGTM, thanks!
The CQ bit was checked by pastarmovj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add migration code for the EnabledPlugins and DisabledPlugins policies. These policies are deprecated in favor of: DefaultPluginSettings (aka. Flash BLOCK/ALLOW) AlwaysOpenPdfExternally (aka. PDF plugin disabled) BUG=654731 TEST=browser_tests:PolicyPrefIndicator ========== to ========== Add migration code for the EnabledPlugins and DisabledPlugins policies. These policies are deprecated in favor of: DefaultPluginSettings (aka. Flash BLOCK/ALLOW) AlwaysOpenPdfExternally (aka. PDF plugin disabled) BUG=654731 TEST=browser_tests:PolicyPrefIndicator ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add migration code for the EnabledPlugins and DisabledPlugins policies. These policies are deprecated in favor of: DefaultPluginSettings (aka. Flash BLOCK/ALLOW) AlwaysOpenPdfExternally (aka. PDF plugin disabled) BUG=654731 TEST=browser_tests:PolicyPrefIndicator ========== to ========== Add migration code for the EnabledPlugins and DisabledPlugins policies. These policies are deprecated in favor of: DefaultPluginSettings (aka. Flash BLOCK/ALLOW) AlwaysOpenPdfExternally (aka. PDF plugin disabled) BUG=654731 TEST=browser_tests:PolicyPrefIndicator Committed: https://crrev.com/c6c75ce329c9b4f9c925056d583dd9f1ae24a9d7 Cr-Commit-Position: refs/heads/master@{#425743} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c6c75ce329c9b4f9c925056d583dd9f1ae24a9d7 Cr-Commit-Position: refs/heads/master@{#425743} |