Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208463002:
[HBD] Implement site-origin exceptions (Closed)
Description[HBD] Implement site-origin exceptions
If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not.
Here is more information about HTML5 by Default:
go/hbd-design-doc
BUG=626728
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c56e1750274cbd7efb5cba947421ec7c36379f3a
Cr-Commit-Position: refs/heads/master@{#413842}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase #Patch Set 3 : Refactor GetPluginContentSetting out to a new file #
Total comments: 6
Patch Set 4 : Changes per tommycli@'s comments #
Total comments: 8
Patch Set 5 : Changes per tommycli@'s comments #
Total comments: 2
Patch Set 6 : Change per tommycli@'s comment #
Total comments: 2
Patch Set 7 : DCHECK to DCHECK_EQ and include fix #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : Changes per bauerb@'s and tommycli@'s comments #Patch Set 10 : Remove changes from chrome_plugin_service_filter.h and render_frame_message_filter.cc #Patch Set 11 : Rebase #
Dependent Patchsets: Messages
Total messages: 75 (46 generated)
Description was changed from ========== [HBD] Implement site-origin exceptions BUG=626728 ========== to ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 ==========
trizzofo@google.com changed reviewers: + tommycli@chromium.org
tommycli, PTAL
https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:127: std::unique_ptr<base::Value> specific_setting = I think you need exactly the logic within PluginInfoMessageFilter::GetPluginContentSetting. It would probably be good to factor that method out into somewhere where this class can access. https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:129: policy_url, policy_url, CONTENT_SETTINGS_TYPE_PLUGINS, Are both these parameters supposed to be policy_url? https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:132: content_settings::ValueToContentSetting(specific_setting.get()); Before using the content_setting, also filter it through PluginsFieldTrial::EffectiveContentSetting https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_info_messa... We should refactor the above method also (probably consolidate it into GetPluginContentSetting and output legacy_ask_user as a bool* parameter).
https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:127: std::unique_ptr<base::Value> specific_setting = On 2016/08/02 20:30:17, tommycli wrote: > I think you need exactly the logic within > PluginInfoMessageFilter::GetPluginContentSetting. It would probably be good to > factor that method out into somewhere where this class can access. I'm thinking about moving it to HostContentSettingsMap. Does it sound like a good idea? https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:129: policy_url, policy_url, CONTENT_SETTINGS_TYPE_PLUGINS, On 2016/08/02 20:30:17, tommycli wrote: > Are both these parameters supposed to be policy_url? Since the plugin url is not relevant for now, and we don't have access to it, I decided to use policy_url twice. This is also done here: https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_info_messa... I'll remove this and use the GetPluginContentSetting method instead. https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:132: content_settings::ValueToContentSetting(specific_setting.get()); On 2016/08/02 20:30:17, tommycli wrote: > Before using the content_setting, also filter it through > PluginsFieldTrial::EffectiveContentSetting > > https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_info_messa... > > We should refactor the above method also (probably consolidate it into > GetPluginContentSetting and output legacy_ask_user as a bool* parameter). You mean moving PluginsFieldTrial::EffectiveContentSettings() code into GetPluginContentSetting() and adding a legacy_ask_user bool* parameter to GetPluginContentSetting()?
https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:127: std::unique_ptr<base::Value> specific_setting = On 2016/08/03 00:23:06, trizzofo wrote: > On 2016/08/02 20:30:17, tommycli wrote: > > I think you need exactly the logic within > > PluginInfoMessageFilter::GetPluginContentSetting. It would probably be good to > > factor that method out into somewhere where this class can access. > > I'm thinking about moving it to HostContentSettingsMap. Does it sound like a > good idea? If it fits, sure! https://codereview.chromium.org/2208463002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:132: content_settings::ValueToContentSetting(specific_setting.get()); On 2016/08/03 00:23:06, trizzofo wrote: > On 2016/08/02 20:30:17, tommycli wrote: > > Before using the content_setting, also filter it through > > PluginsFieldTrial::EffectiveContentSetting > > > > > https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_info_messa... > > > > We should refactor the above method also (probably consolidate it into > > GetPluginContentSetting and output legacy_ask_user as a bool* parameter). > > You mean moving PluginsFieldTrial::EffectiveContentSettings() code into > GetPluginContentSetting() and adding a legacy_ask_user bool* parameter to > GetPluginContentSetting()? Yeah, if it fits the usage cases, try it.
The CQ bit was checked by trizzofo@google.com 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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by trizzofo@google.com 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...
tommycli, ptal. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Overall very close here. https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:131: std::unique_ptr<base::Value> specific_setting = This call still needs changing to the new util call right? https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_filter_utils.cc (right): https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/plugin_filter_utils.cc:45: // TODO(trizzofo): Add param. What param is the TODO for? The legacy_ask_user? https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_info_message_filter_unittest.cc (right): https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/plugin_info_message_filter_unittest.cc:147: ::GetPluginContentSetting(host_content_settings_map_, Does this need a :: prefix? Or can it work with just GetPluginContentSetting?
The CQ bit was checked by trizzofo@google.com 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...
https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:131: std::unique_ptr<base::Value> specific_setting = On 2016/08/08 16:52:20, tommycli wrote: > This call still needs changing to the new util call right? Yes. Done. https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_filter_utils.cc (right): https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/plugin_filter_utils.cc:45: // TODO(trizzofo): Add param. On 2016/08/08 16:52:20, tommycli wrote: > What param is the TODO for? The legacy_ask_user? Yes, it was. I forgot to remove the TODO comment. https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_info_message_filter_unittest.cc (right): https://codereview.chromium.org/2208463002/diff/60001/chrome/browser/plugins/... chrome/browser/plugins/plugin_info_message_filter_unittest.cc:147: ::GetPluginContentSetting(host_content_settings_map_, On 2016/08/08 16:52:20, tommycli wrote: > Does this need a :: prefix? Or can it work with just GetPluginContentSetting? I thought it wouldn't work without the :: prefix, but I was wrong. Removed it.
https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:133: ContentSetting plugin_setting; initialize with ContentSetting plugin_setting = CONTENT_SETTING_DEFAULT; https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:135: bool is_managed; Since these two variables aren't used here, I would recommend making them optional (allow nullptr in the function). You can just use an if statement to not set them if they are nullptr. Then you can just delete these two vars from this function. https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:146: plugin_setting == CONTENT_SETTING_DETECT_IMPORTANT_CONTENT) Nit: Two line predicates in if statements need braces: if (foo || bar) { return false } https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_filter_utils.h (right): https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/plugin_filter_utils.h:17: void GetPluginContentSetting( This could use a short comment. Even if it just says that |is_default| and |is_managed| may be nullptr.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm still unsure about why and how the plugin_url is used in GetPluginContentSetting. We should probably ask bauerb. For now, I'll pass the policy_url as the plugin_url. https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:133: ContentSetting plugin_setting; On 2016/08/08 20:38:06, tommycli wrote: > initialize with ContentSetting plugin_setting = CONTENT_SETTING_DEFAULT; Done. https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:135: bool is_managed; On 2016/08/08 20:38:06, tommycli wrote: > Since these two variables aren't used here, I would recommend making them > optional (allow nullptr in the function). You can just use an if statement to > not set them if they are nullptr. > > Then you can just delete these two vars from this function. Done. https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:146: plugin_setting == CONTENT_SETTING_DETECT_IMPORTANT_CONTENT) On 2016/08/08 20:38:06, tommycli wrote: > Nit: Two line predicates in if statements need braces: > > if (foo || > bar) { > return false > } Done. https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... File chrome/browser/plugins/plugin_filter_utils.h (right): https://codereview.chromium.org/2208463002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/plugin_filter_utils.h:17: void GetPluginContentSetting( On 2016/08/08 20:38:06, tommycli wrote: > This could use a short comment. Even if it just says that |is_default| and > |is_managed| may be nullptr. Done.
The CQ bit was checked by trizzofo@google.com 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...
LGTM except below. Thanks https://codereview.chromium.org/2208463002/diff/100001/chrome/browser/plugins... File chrome/browser/plugins/plugin_filter_utils.cc (right): https://codereview.chromium.org/2208463002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/plugin_filter_utils.cc:86: if (uses_default_content_setting) nit: this needs braces also
The CQ bit was checked by trizzofo@google.com 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...
trizzofo@google.com changed reviewers: + bauerb@chromium.org
bauerb, ptal. We are now fetching the plugin content setting given the main frame origin (policy_url). Since GetPluginContentSetting is now being used by two different classes, we refactored it out to a new file called plugin_filter_utils.h. Regarding the GetPluginContentSetting function, I was wondering why would the plugin_url be relevant since it's the policy_url that's used for plugin content settings exceptions. Is it really being used? In the current patch set, the caller in chrome_plugin_service_filter.cc doesn't have access to the plugin url so, for now, I'm passing the policy_url as the plugin_url. Thanks! https://codereview.chromium.org/2208463002/diff/100001/chrome/browser/plugins... File chrome/browser/plugins/plugin_filter_utils.cc (right): https://codereview.chromium.org/2208463002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/plugin_filter_utils.cc:86: if (uses_default_content_setting) On 2016/08/08 21:45:54, tommycli wrote: > nit: this needs braces also Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/08 22:21:14, trizzofo wrote: > bauerb, ptal. > > We are now fetching the plugin content setting given the main frame origin > (policy_url). Since GetPluginContentSetting is now being used by two different > classes, we refactored it out to a new file called plugin_filter_utils.h. > > Regarding the GetPluginContentSetting function, I was wondering why would the > plugin_url be relevant since it's the policy_url that's used for plugin content > settings exceptions. Is it really being used? Yes, in principle content settings can be defined for two origins -- the primary one for plugins is the origin of the top-level frame, and the secondary one is the URL of the plugin content. This could allow someone for example to block all plugins on a given website except for the ones hosted on the trusted CDN. The functionality to configure this is only available via the content settings extension API though. > In the current patch set, the caller in chrome_plugin_service_filter.cc doesn't > have access to the plugin url so, for now, I'm passing the policy_url as the > plugin_url. Yes, you do. It's the |url| parameter. https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... File chrome/browser/plugins/plugin_filter_utils.cc (right): https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... chrome/browser/plugins/plugin_filter_utils.cc:35: DCHECK(plugin.type == WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS); DCHECK_EQ (and put the expected value first) for nicer error messages.
On 2016/08/09 10:36:47, Bernhard Bauer wrote: > On 2016/08/08 22:21:14, trizzofo wrote: > > bauerb, ptal. > > > > We are now fetching the plugin content setting given the main frame origin > > (policy_url). Since GetPluginContentSetting is now being used by two different > > classes, we refactored it out to a new file called plugin_filter_utils.h. > > > > Regarding the GetPluginContentSetting function, I was wondering why would the > > plugin_url be relevant since it's the policy_url that's used for plugin > content > > settings exceptions. Is it really being used? > > Yes, in principle content settings can be defined for two origins -- the primary > one for plugins is the origin of the top-level frame, and the secondary one is > the URL of the plugin content. This could allow someone for example to block all > plugins on a given website except for the ones hosted on the trusted CDN. The > functionality to configure this is only available via the content settings > extension API though. > > > In the current patch set, the caller in chrome_plugin_service_filter.cc > doesn't > > have access to the plugin url so, for now, I'm passing the policy_url as the > > plugin_url. > > Yes, you do. It's the |url| parameter. > > https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... > File chrome/browser/plugins/plugin_filter_utils.cc (right): > > https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... > chrome/browser/plugins/plugin_filter_utils.cc:35: DCHECK(plugin.type == > WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS); > DCHECK_EQ (and put the expected value first) for nicer error messages. Hey. There's definitely some circumstances where there's no plugin content URL. For instance, if there's just JavaScript that queries navigator.plugins(). What should we pass for the secondary_url then? The policy_url again? Thanks.
On 2016/08/09 17:15:46, tommycli wrote: > On 2016/08/09 10:36:47, Bernhard Bauer wrote: > > On 2016/08/08 22:21:14, trizzofo wrote: > > > bauerb, ptal. > > > > > > We are now fetching the plugin content setting given the main frame origin > > > (policy_url). Since GetPluginContentSetting is now being used by two > different > > > classes, we refactored it out to a new file called plugin_filter_utils.h. > > > > > > Regarding the GetPluginContentSetting function, I was wondering why would > the > > > plugin_url be relevant since it's the policy_url that's used for plugin > > content > > > settings exceptions. Is it really being used? > > > > Yes, in principle content settings can be defined for two origins -- the > primary > > one for plugins is the origin of the top-level frame, and the secondary one is > > the URL of the plugin content. This could allow someone for example to block > all > > plugins on a given website except for the ones hosted on the trusted CDN. The > > functionality to configure this is only available via the content settings > > extension API though. > > > > > In the current patch set, the caller in chrome_plugin_service_filter.cc > > doesn't > > > have access to the plugin url so, for now, I'm passing the policy_url as the > > > plugin_url. > > > > Yes, you do. It's the |url| parameter. > > > > > https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... > > File chrome/browser/plugins/plugin_filter_utils.cc (right): > > > > > https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... > > chrome/browser/plugins/plugin_filter_utils.cc:35: DCHECK(plugin.type == > > WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS); > > DCHECK_EQ (and put the expected value first) for nicer error messages. > > Hey. There's definitely some circumstances where there's no plugin content URL. > > For instance, if there's just JavaScript that queries navigator.plugins(). What > should we pass for the secondary_url then? The policy_url again? > > Thanks. bauerb, ptal at tommycli comment. For more context, this is the ChromePluginServiceFilter::IsAvailable() caller that we're interested in: https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... The CL on which this CL depends is changing it to pass the policy_url, but we're still passing GURL() as the plugin_url: https://crrev.com/2156803002/diff/380001/content/browser/frame_host/render_fr... Thanks!
On 2016/08/11 18:49:25, trizzofo wrote: > On 2016/08/09 17:15:46, tommycli wrote: > > On 2016/08/09 10:36:47, Bernhard Bauer wrote: > > > On 2016/08/08 22:21:14, trizzofo wrote: > > > > bauerb, ptal. > > > > > > > > We are now fetching the plugin content setting given the main frame origin > > > > (policy_url). Since GetPluginContentSetting is now being used by two > > different > > > > classes, we refactored it out to a new file called plugin_filter_utils.h. > > > > > > > > Regarding the GetPluginContentSetting function, I was wondering why would > > the > > > > plugin_url be relevant since it's the policy_url that's used for plugin > > > content > > > > settings exceptions. Is it really being used? > > > > > > Yes, in principle content settings can be defined for two origins -- the > > primary > > > one for plugins is the origin of the top-level frame, and the secondary one > is > > > the URL of the plugin content. This could allow someone for example to block > > all > > > plugins on a given website except for the ones hosted on the trusted CDN. > The > > > functionality to configure this is only available via the content settings > > > extension API though. > > > > > > > In the current patch set, the caller in chrome_plugin_service_filter.cc > > > doesn't > > > > have access to the plugin url so, for now, I'm passing the policy_url as > the > > > > plugin_url. > > > > > > Yes, you do. It's the |url| parameter. > > > > > > > > > https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... > > > File chrome/browser/plugins/plugin_filter_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... > > > chrome/browser/plugins/plugin_filter_utils.cc:35: DCHECK(plugin.type == > > > WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS); > > > DCHECK_EQ (and put the expected value first) for nicer error messages. > > > > Hey. There's definitely some circumstances where there's no plugin content > URL. > > > > For instance, if there's just JavaScript that queries navigator.plugins(). > What > > should we pass for the secondary_url then? The policy_url again? > > > > Thanks. > > bauerb, ptal at tommycli comment. For more context, this is the > ChromePluginServiceFilter::IsAvailable() caller that we're interested in: > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > > The CL on which this CL depends is changing it to pass the policy_url, but we're > still passing GURL() as the plugin_url: > https://crrev.com/2156803002/diff/380001/content/browser/frame_host/render_fr... > > Thanks! Ok, in that case I would pass the |policy_url| for both, which effectively means that Flash is advertised for a given site if a Flash embed hosted on the same site would be allowed there. Other Flash embeds might still be blocked in theory, but that wouldn't affect whether it's advertised. Does that sound okay?
One issue I can think of is that, if the user allows an origin, but blocks plugins hosted there, Flash won't be advertised. So, even if all the Flash embeds in the page are hosted on a different origin, they won't be displayed. Maybe that behavior is a little bit confusing. Do you think that it would still make sense to keep per plugin_url exceptions after HBD? I'm not sure how both will behave together since the decision of advertising or not Flash is global for the whole page. https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... File chrome/browser/plugins/plugin_filter_utils.cc (right): https://codereview.chromium.org/2208463002/diff/120001/chrome/browser/plugins... chrome/browser/plugins/plugin_filter_utils.cc:35: DCHECK(plugin.type == WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS); On 2016/08/09 10:36:47, Bernhard Bauer wrote: > DCHECK_EQ (and put the expected value first) for nicer error messages. Done.
On 2016/08/15 22:32:01, trizzofo wrote: > One issue I can think of is that, if the user allows an origin, but blocks > plugins hosted there, Flash won't be advertised. So, even if all the Flash > embeds in the page are hosted on a different origin, they won't be displayed. > Maybe that behavior is a little bit confusing. What do you mean by that? What would the content setting rules look like for that situation? > Do you think that it would still make sense to keep per plugin_url exceptions > after HBD? I'm not sure how both will behave together since the decision of > advertising or not Flash is global for the whole page. I think in general it still makes sense, because there is more to plugin loading than advertising the plugin in navigator.plugins – if a plugin is advertised, we don't guarantee that it will actually be loaded (in fact, that's how plugin blocking has worked since we implemented click-to-play), and a plugin doesn't need to be advertised in order to be instantiated,
On 2016/08/15 23:27:15, Bernhard Bauer wrote: > On 2016/08/15 22:32:01, trizzofo wrote: > > One issue I can think of is that, if the user allows an origin, but blocks > > plugins hosted there, Flash won't be advertised. So, even if all the Flash > > embeds in the page are hosted on a different origin, they won't be displayed. > > Maybe that behavior is a little bit confusing. > > What do you mean by that? What would the content setting rules look like for > that situation? Here is my understanding about how plugin content settings exceptions works (correct me if I'm wrong): There are two sets of rules that override the default setting. One is matched by the |policy_url| and the other one, only accessible via API, is matched by the |plugin_url|. More specifically, |policy_url| rules override the default settings and |plugin_url| rules override |policy_url| rules. So, imagine if you have an ALLOW |policy_url| rule for [.*]example.com and a BLOCK |plugin_url| rule for [.*]example.com. Passing |policy_url| for both GetPluginContentSetting() url params implies that Flash won't be advertised for [.*]example.com pages, even if none of the Flash embeds in it are hosted in [.*]example.com. I'm not sure if that is what we want. > > Do you think that it would still make sense to keep per plugin_url exceptions > > after HBD? I'm not sure how both will behave together since the decision of > > advertising or not Flash is global for the whole page. > > I think in general it still makes sense, because there is more to plugin loading > than advertising the plugin in navigator.plugins – if a plugin is advertised, we > don't guarantee that it will actually be loaded (in fact, that's how plugin > blocking has worked since we implemented click-to-play), and a plugin doesn't > need to be advertised in order to be instantiated, Hmmm yes, that makes sense. I believe that the expected behavior is that IsPluginAvailable() ignores |plugin_url| rules when it's called to check whether a plugin should be advertised, but consider them when it's being called to check whether a Flash embed should run. Correct me if I'm wrong tommycli. What happens if I pass an empty GURL as the |plugin_url| parameter to GetPluginSettings()? Will the |plugin_url| rules be ignored?
On 2016/08/16 00:38:25, trizzofo wrote: > On 2016/08/15 23:27:15, Bernhard Bauer wrote: > > On 2016/08/15 22:32:01, trizzofo wrote: > > > One issue I can think of is that, if the user allows an origin, but blocks > > > plugins hosted there, Flash won't be advertised. So, even if all the Flash > > > embeds in the page are hosted on a different origin, they won't be > displayed. > > > Maybe that behavior is a little bit confusing. > > > > What do you mean by that? What would the content setting rules look like for > > that situation? > > Here is my understanding about how plugin content settings exceptions works > (correct me if I'm wrong): > > There are two sets of rules that override the default setting. One is matched by > the |policy_url| and the other one, only accessible via API, is matched by the > |plugin_url|. More specifically, |policy_url| rules override the default > settings and |plugin_url| rules override |policy_url| rules. > > So, imagine if you have an ALLOW |policy_url| rule for [.*]example.com and a > BLOCK |plugin_url| rule for [.*]example.com. Passing |policy_url| for both > GetPluginContentSetting() url params implies that Flash won't be advertised for > [.*]example.com pages, even if none of the Flash embeds in it are hosted in > [.*]example.com. I'm not sure if that is what we want. > > > > Do you think that it would still make sense to keep per plugin_url > exceptions > > > after HBD? I'm not sure how both will behave together since the decision of > > > advertising or not Flash is global for the whole page. > > > > I think in general it still makes sense, because there is more to plugin > loading > > than advertising the plugin in navigator.plugins – if a plugin is advertised, > we > > don't guarantee that it will actually be loaded (in fact, that's how plugin > > blocking has worked since we implemented click-to-play), and a plugin doesn't > > need to be advertised in order to be instantiated, > > Hmmm yes, that makes sense. I believe that the expected behavior is that > IsPluginAvailable() ignores |plugin_url| rules when it's called to check whether > a plugin should be advertised, but consider them when it's being called to check > whether a Flash embed should run. Correct me if I'm wrong tommycli. > > What happens if I pass an empty GURL as the |plugin_url| parameter to > GetPluginSettings()? Will the |plugin_url| rules be ignored? Above reasoning seems solid if secondary_url rules do indeed override primary_url rules. In that case, it seems like we should pass in an empty URL for generating the list of plugins to be advertised.
On 2016/08/16 00:38:25, trizzofo wrote: > On 2016/08/15 23:27:15, Bernhard Bauer wrote: > > On 2016/08/15 22:32:01, trizzofo wrote: > > > One issue I can think of is that, if the user allows an origin, but blocks > > > plugins hosted there, Flash won't be advertised. So, even if all the Flash > > > embeds in the page are hosted on a different origin, they won't be > displayed. > > > Maybe that behavior is a little bit confusing. > > > > What do you mean by that? What would the content setting rules look like for > > that situation? > > Here is my understanding about how plugin content settings exceptions works > (correct me if I'm wrong): > > There are two sets of rules that override the default setting. One is matched by > the |policy_url| and the other one, only accessible via API, is matched by the > |plugin_url|. More specifically, |policy_url| rules override the default > settings and |plugin_url| rules override |policy_url| rules. > > So, imagine if you have an ALLOW |policy_url| rule for [.*]example.com and a > BLOCK |plugin_url| rule for [.*]example.com. Passing |policy_url| for both > GetPluginContentSetting() url params implies that Flash won't be advertised for > [.*]example.com pages, even if none of the Flash embeds in it are hosted in > [.*]example.com. I'm not sure if that is what we want. Not quite :) There is only one set of rules, but each rule specifies both a primary pattern (matching the |policy_url| here) and a secondary pattern (matching the |plugin_url|), although each pattern can be a wildcard. The difference between primary and secondary comes into play when deciding which one of multiple matching rules gets applied: The rule with the more specific primary pattern takes precedence, and for equal primary patterns the rule with the more specific secondary pattern takes precedence. Also, content setting rules set by the UI always use a wildcard for the secondary pattern (which has the effect of ignoring the secondary URL, because the pattern will match anything, and lets extensions set rules with more specific behavior). So, you are right that it could happen that we would not advertise Flash for a site, even though due to specific content settings Flash embeds would actually be allowed. That could happen regardless of what we pass for the secondary URL though. If we passed an empty GURL, we could still have the following case: [*.]example.com, *: BLOCK (set by UI) [*.]example.com, examplecdn.com: ALLOW (set by extension) An empty URL would only match a wildcard pattern, so the content setting returned would be BLOCK and we would not advertise Flash on example.com. If the site embeds Flash from examplecdn.com though, it would actually be ALLOWed though. So, I think we could always construct pathological cases where we'd do the wrong thing; the reason why I was suggesting to use the same primary and secondary URL was just because that's what we used to do e.g. for content settings that don't use a secondary URL at all. But I guess I can also see passing an empty URL, to ignore any rules that have something more specific than a wildcard for the secondary pattern. > > > Do you think that it would still make sense to keep per plugin_url > exceptions > > > after HBD? I'm not sure how both will behave together since the decision of > > > advertising or not Flash is global for the whole page. > > > > I think in general it still makes sense, because there is more to plugin > loading > > than advertising the plugin in navigator.plugins – if a plugin is advertised, > we > > don't guarantee that it will actually be loaded (in fact, that's how plugin > > blocking has worked since we implemented click-to-play), and a plugin doesn't > > need to be advertised in order to be instantiated, > > Hmmm yes, that makes sense. I believe that the expected behavior is that > IsPluginAvailable() ignores |plugin_url| rules when it's called to check whether > a plugin should be advertised, but consider them when it's being called to check > whether a Flash embed should run. Correct me if I'm wrong tommycli. > > What happens if I pass an empty GURL as the |plugin_url| parameter to > GetPluginSettings()? Will the |plugin_url| rules be ignored? Like I've mentioned above, an empty GURL will only match wildcard rules, so we would ignore any rule where the secondary pattern is more specific than that. https://codereview.chromium.org/2208463002/diff/140001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/140001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter.cc:137: policy_url, policy_url, Pass the |url| as the second parameter here? Then we can pass in the empty GURL() to this at the call site when we want to.
On 2016/08/18 18:06:07, Bernhard Bauer wrote: > On 2016/08/16 00:38:25, trizzofo wrote: > > On 2016/08/15 23:27:15, Bernhard Bauer wrote: > > > On 2016/08/15 22:32:01, trizzofo wrote: > > > > One issue I can think of is that, if the user allows an origin, but blocks > > > > plugins hosted there, Flash won't be advertised. So, even if all the Flash > > > > embeds in the page are hosted on a different origin, they won't be > > displayed. > > > > Maybe that behavior is a little bit confusing. > > > > > > What do you mean by that? What would the content setting rules look like for > > > that situation? > > > > Here is my understanding about how plugin content settings exceptions works > > (correct me if I'm wrong): > > > > There are two sets of rules that override the default setting. One is matched > by > > the |policy_url| and the other one, only accessible via API, is matched by the > > |plugin_url|. More specifically, |policy_url| rules override the default > > settings and |plugin_url| rules override |policy_url| rules. > > > > So, imagine if you have an ALLOW |policy_url| rule for [.*]example.com and a > > BLOCK |plugin_url| rule for [.*]example.com. Passing |policy_url| for both > > GetPluginContentSetting() url params implies that Flash won't be advertised > for > > [.*]example.com pages, even if none of the Flash embeds in it are hosted in > > [.*]example.com. I'm not sure if that is what we want. > > Not quite :) There is only one set of rules, but each rule specifies both a > primary pattern (matching the |policy_url| here) and a secondary pattern > (matching the |plugin_url|), although each pattern can be a wildcard. The > difference between primary and secondary comes into play when deciding which one > of multiple matching rules gets applied: The rule with the more specific primary > pattern takes precedence, and for equal primary patterns the rule with the more > specific secondary pattern takes precedence. Also, content setting rules set by > the UI always use a wildcard for the secondary pattern (which has the effect of > ignoring the secondary URL, because the pattern will match anything, and lets > extensions set rules with more specific behavior). > > So, you are right that it could happen that we would not advertise Flash for a > site, even though due to specific content settings Flash embeds would actually > be allowed. That could happen regardless of what we pass for the secondary URL > though. If we passed an empty GURL, we could still have the following case: > > [*.]example.com, *: BLOCK (set by UI) > [*.]example.com, http://examplecdn.com: ALLOW (set by extension) > > An empty URL would only match a wildcard pattern, so the content setting > returned would be BLOCK and we would not advertise Flash on http://example.com. If the > site embeds Flash from http://examplecdn.com though, it would actually be ALLOWed > though. > > So, I think we could always construct pathological cases where we'd do the wrong > thing; the reason why I was suggesting to use the same primary and secondary URL > was just because that's what we used to do e.g. for content settings that don't > use a secondary URL at all. But I guess I can also see passing an empty URL, to > ignore any rules that have something more specific than a wildcard for the > secondary pattern. Wow that's more complicated that I imagined. In that case, since there is already a precedent of passing the primary URL twice in the case where we don't have a secondary URL, I would agree with sticking to that as well. I think we should add a short comment explaining that other places lacking a secondary URL pass the top level URL twice. > > > > > Do you think that it would still make sense to keep per plugin_url > > exceptions > > > > after HBD? I'm not sure how both will behave together since the decision > of > > > > advertising or not Flash is global for the whole page. > > > > > > I think in general it still makes sense, because there is more to plugin > > loading > > > than advertising the plugin in navigator.plugins – if a plugin is > advertised, > > we > > > don't guarantee that it will actually be loaded (in fact, that's how plugin > > > blocking has worked since we implemented click-to-play), and a plugin > doesn't > > > need to be advertised in order to be instantiated, > > > > Hmmm yes, that makes sense. I believe that the expected behavior is that > > IsPluginAvailable() ignores |plugin_url| rules when it's called to check > whether > > a plugin should be advertised, but consider them when it's being called to > check > > whether a Flash embed should run. Correct me if I'm wrong tommycli. > > > > What happens if I pass an empty GURL as the |plugin_url| parameter to > > GetPluginSettings()? Will the |plugin_url| rules be ignored? > > Like I've mentioned above, an empty GURL will only match wildcard rules, so we > would ignore any rule where the secondary pattern is more specific than that. > > https://codereview.chromium.org/2208463002/diff/140001/chrome/browser/plugins... > File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): > > https://codereview.chromium.org/2208463002/diff/140001/chrome/browser/plugins... > chrome/browser/plugins/chrome_plugin_service_filter.cc:137: policy_url, > policy_url, > Pass the |url| as the second parameter here? Then we can pass in the empty > GURL() to this at the call site when we want to.
Description was changed from ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 ========== to ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by trizzofo@google.com 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...
Patchset #8 (id:160001) has been deleted
Patchset #8 (id:180001) has been deleted
The CQ bit was checked by trizzofo@google.com 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...
Patchset #9 (id:220001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by trizzofo@google.com 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...
trizzofo@google.com changed reviewers: + clamy@chromium.org
Thanks! I'll pass the |policy_url| twice then. clamy, ptal at render_frame_message_filter.cc. https://codereview.chromium.org/2208463002/diff/140001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2208463002/diff/140001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter.cc:137: policy_url, policy_url, On 2016/08/18 18:06:07, Bernhard Bauer wrote: > Pass the |url| as the second parameter here? Then we can pass in the empty > GURL() to this at the call site when we want to. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
clamy, ptal. Thanks!
clamy, ptal. Thanks!
The CQ bit was checked by trizzofo@google.com 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: This issue passed the CQ dry run.
The CQ bit was checked by trizzofo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2208463002/#ps280001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
clamy, I moved content/browser/frame_host/render_frame_message_filter.cc changes to another issue, so we don't need your review anymore. Thanks!
Description was changed from ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
trizzofo@google.com changed reviewers: - clamy@chromium.org
Message was sent while issue was closed.
Description was changed from ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [HBD] Implement site-origin exceptions If PreferHtmlOverPlugins feature is enabled, use "Plugins" Content Settings exceptions to decide if Flash should be advertised or not. Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c56e1750274cbd7efb5cba947421ec7c36379f3a Cr-Commit-Position: refs/heads/master@{#413842} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c56e1750274cbd7efb5cba947421ec7c36379f3a Cr-Commit-Position: refs/heads/master@{#413842} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
