Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168453002:
[HBD] Don't advertise Flash if Content Setting is on CONTENT_SETTING_BLOCK (Closed)
Description[HBD] Don't advertise Flash if Content Setting is on CONTENT_SETTING_BLOCK
When PreferHtmlOverPlugins feature is enabled, fetch Content Settings and don't advertise Flash if it's on CONTENT_SETTING_BLOCK. This will only work properly when plugin caches are disabled in future CLs.
Here is more information about HTML5 by Default:
https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss
BUG=626728
Committed: https://crrev.com/a5a2bb3171707315ef1e2737edc4677985ca453c
Cr-Commit-Position: refs/heads/master@{#407514}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address issues mentioned in comments #
Total comments: 6
Patch Set 3 : Use auto #
Total comments: 4
Patch Set 4 : Changes pointed in comments #Patch Set 5 : Endlines and temporary PluginPrefs pointer removed #
Total comments: 1
Patch Set 6 : Test fix #Patch Set 7 : std::move and format #Patch Set 8 : Compilation error fix #Patch Set 9 : Fix Windows compilation error #
Messages
Total messages: 51 (31 generated)
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...
Description was changed from ========== [HBD] Don't advertise Flash if Content Setting is on BLOCK When PreferHtmlOverPlugins feature is enabled, fetch Content Settings and don't advertise Flash if it's on CONTENT_SETTING_BLOCK. This will only work properly when plugin caches are disabled in future CLs. Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ========== to ========== [HBD] Don't advertise Flash if Content Setting is on CONTENT_SETTING_BLOCK When PreferHtmlOverPlugins feature is enabled, fetch Content Settings and don't advertise Flash if it's on CONTENT_SETTING_BLOCK. This will only work properly when plugin caches are disabled in future CLs. Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ==========
trizzofo@google.com changed reviewers: + tommycli@chromium.org
tommycli, PTAL
https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:131: if (host_content_settings_map_it == host_content_settings_maps_.end()) I wonder if this can reasonably happen other than programmer error. Same with Line 147. You can leave it as is, but we can ask bauerb. https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:140: return true; I don't think you should return true here, since it may still be disabled on line 150. https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.h:45: void RegisterHostContentSettingsMap( Yes I think these methods should be folded into the existing RegisterResourceContext method. An extra parameter makes sense to me.
https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:134: ContentSetting content_setting = Also it would be good to add a // TODO(trizzofo): Implement site-origin exceptions once we have the top-level frame origin.
https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:131: if (host_content_settings_map_it == host_content_settings_maps_.end()) On 2016/07/19 23:32:51, tommycli wrote: > I wonder if this can reasonably happen other than programmer error. Same with > Line 147. You can leave it as is, but we can ask bauerb. Acknowledged. https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:134: ContentSetting content_setting = On 2016/07/19 23:37:47, tommycli wrote: > Also it would be good to add a // TODO(trizzofo): Implement site-origin > exceptions once we have the top-level frame origin. Done. https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:140: return true; On 2016/07/19 23:32:51, tommycli wrote: > I don't think you should return true here, since it may still be disabled on > line 150. Done. https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2168453002/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.h:45: void RegisterHostContentSettingsMap( On 2016/07/19 23:32:51, tommycli wrote: > Yes I think these methods should be folded into the existing > RegisterResourceContext method. An extra parameter makes sense to me. 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
trizzofo@google.com changed reviewers: + bauerb@chromium.org
bauerb PTAL. Also, PTAL at the comment in chrome_plugin_service_filter.cc. Thanks! https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:130: if (host_content_settings_map_it == host_content_settings_maps_.end()) We were wondering if this can reasonably happen other than programmer error. Same with Line 117.
groby@chromium.org changed reviewers: + groby@chromium.org
Apologies, I have a tiny drive-by comment :) https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:126: std::map<const void*, scoped_refptr<HostContentSettingsMap>>::iterator Please use auto for these things, it's _much_ more readable :) I.e. auto content_settings_it = host_content_settings_map.find(context);
https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:126: std::map<const void*, scoped_refptr<HostContentSettingsMap>>::iterator On 2016/07/20 01:11:49, groby wrote: > Please use auto for these things, it's _much_ more readable :) I.e. > > auto content_settings_it = host_content_settings_map.find(context); Done. Much better now :)
LGTM with some nits/suggestions: https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:130: if (host_content_settings_map_it == host_content_settings_maps_.end()) On 2016/07/20 00:38:52, trizzofo wrote: > We were wondering if this can reasonably happen other than programmer error. > Same with Line 117. Probably not. The map would have to be registered too late or unregistered too early, none of which should happen because they're tied to the lifetime of the profile. https://codereview.chromium.org/2168453002/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2168453002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:42: HostContentSettingsMap* host_content_settings_map, If this method takes a reference, it should maybe take a scoped_refptr as an argument. Same for |plugin_prefs|. https://codereview.chromium.org/2168453002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:115: ResourceContextMap resource_context_map_; Should we name this |plugin_prefs_| to be consistent with |host_content_settings_maps_|?
https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:130: if (host_content_settings_map_it == host_content_settings_maps_.end()) On 2016/07/20 08:41:32, Bernhard Bauer wrote: > On 2016/07/20 00:38:52, trizzofo wrote: > > We were wondering if this can reasonably happen other than programmer error. > > Same with Line 117. > > Probably not. The map would have to be registered too late or unregistered too > early, none of which should happen because they're tied to the lifetime of the > profile. These can be changed to DCHECKs instead then.
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/2168453002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:130: if (host_content_settings_map_it == host_content_settings_maps_.end()) On 2016/07/20 15:35:27, tommycli wrote: > On 2016/07/20 08:41:32, Bernhard Bauer wrote: > > On 2016/07/20 00:38:52, trizzofo wrote: > > > We were wondering if this can reasonably happen other than programmer error. > > > Same with Line 117. > > > > Probably not. The map would have to be registered too late or unregistered too > > early, none of which should happen because they're tied to the lifetime of the > > profile. > > These can be changed to DCHECKs instead then. Done. https://codereview.chromium.org/2168453002/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2168453002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:42: HostContentSettingsMap* host_content_settings_map, On 2016/07/20 08:41:32, Bernhard Bauer wrote: > If this method takes a reference, it should maybe take a scoped_refptr as an > argument. Same for |plugin_prefs|. Done. https://codereview.chromium.org/2168453002/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:115: ResourceContextMap resource_context_map_; On 2016/07/20 08:41:32, Bernhard Bauer wrote: > Should we name this |plugin_prefs_| to be consistent with > |host_content_settings_maps_|? 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...
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2168453002/diff/80001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2168453002/diff/80001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:65: plugin_prefs_[context] = plugin_prefs; Nit: std::move the scoped_refptrs so you have less ref churn.
(Still LGTM otherwise tho')
thestig@chromium.org changed reviewers: + thestig@chromium.org
There's red bots with the print preview test. I guess you should fix that and have me review the print preview test changes.
On 2016/07/22 00:40:27, Lei Zhang wrote: > There's red bots with the print preview test. I guess you should fix that and > have me review the print preview test changes. Yes, I'm working on it right now. I'll add you as a reviewer as soon as I finish.
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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...) 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...
Lei Zhang, PTAL at the browsertest file. Before my changes, the IsPluginAvailable method returned false whenever nullptr was passed as the ResourceContext, so the test was passing. I added a DCHECK in it to assure that the context is always found, then the test started crashing. And when I started passing a ResourceContext to it, the test started failing. I think that the issue with the test was that EnablePluginGroup runs asynchronously and the plugin state was still not updated by the time the code reached IsPluginAvailable. I changed EnablePluginGroup to EnablePlugin and passed a Callback function with run_loop.QuitClosure() as an argument.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
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/2168453002/#ps160001 (title: "Fix Windows compilation error")
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 ========== [HBD] Don't advertise Flash if Content Setting is on CONTENT_SETTING_BLOCK When PreferHtmlOverPlugins feature is enabled, fetch Content Settings and don't advertise Flash if it's on CONTENT_SETTING_BLOCK. This will only work properly when plugin caches are disabled in future CLs. Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ========== to ========== [HBD] Don't advertise Flash if Content Setting is on CONTENT_SETTING_BLOCK When PreferHtmlOverPlugins feature is enabled, fetch Content Settings and don't advertise Flash if it's on CONTENT_SETTING_BLOCK. This will only work properly when plugin caches are disabled in future CLs. Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [HBD] Don't advertise Flash if Content Setting is on CONTENT_SETTING_BLOCK When PreferHtmlOverPlugins feature is enabled, fetch Content Settings and don't advertise Flash if it's on CONTENT_SETTING_BLOCK. This will only work properly when plugin caches are disabled in future CLs. Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 ========== to ========== [HBD] Don't advertise Flash if Content Setting is on CONTENT_SETTING_BLOCK When PreferHtmlOverPlugins feature is enabled, fetch Content Settings and don't advertise Flash if it's on CONTENT_SETTING_BLOCK. This will only work properly when plugin caches are disabled in future CLs. Here is more information about HTML5 by Default: https://docs.google.com/document/d/1iHhg7wxYTgBHsTzcfaZy36UJ33JmjRLro8TZR6lb0ss BUG=626728 Committed: https://crrev.com/a5a2bb3171707315ef1e2737edc4677985ca453c Cr-Commit-Position: refs/heads/master@{#407514} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a5a2bb3171707315ef1e2737edc4677985ca453c Cr-Commit-Position: refs/heads/master@{#407514} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
