Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285553002:
[HBD] Gate the advertising of Flash on Site Engagement. (Closed)
Description[HBD] Gate the advertising of Flash on Site Engagement.
When the PreferHtmlOverPlugins feature is active, and the plugins content
setting isn't one of ALLOW or BLOCK, each site's engagement score is
now consulted to determine whether or not Flash is added to the list of
available plugins. The score must be above a certain threshold for Flash
to be advertised.
The amount of engagement required to activate Flash defaults to 1, which
is currently the equivalent of navigating directly to a site via the
omnibox, or spending ~1 minute interacting with the site after visiting
via a link. This value is controllable via a variations parameter under
the PreferHtmlOverPlugins feature.
This CL adds a thread-agnostic interface to the site engagement service,
which is necessary as ChromePluginServiceFilter::IsPluginAvailable
runs exclusively on the IO thread, from which is it not possible to
create a SiteEngagementService object. This new interface relies on the
fact that the underlying HostContentSettingsMap class methods can be
called from any thread.
This CL additionally removes the fallback logic from the site
engagement service for incognito support, as this is already baked into
the underlying HostContentSettingsMap. A minor cleanup of
ChromePluginServiceFilter is also undertaken.
BUG=626728, 641627, 641630
Committed: https://crrev.com/8583adb8ee4711d11421d53579567bc68eaf4d54
Cr-Commit-Position: refs/heads/master@{#419623}
Patch Set 1 #Patch Set 2 : Check URL #
Total comments: 5
Patch Set 3 : Thread safety, unit tests. Removing observer #Patch Set 4 : Incognito support #Patch Set 5 : Remove dead code #Patch Set 6 : Tidying #
Total comments: 13
Patch Set 7 : Address comments #
Total comments: 17
Patch Set 8 : Addressing comments #
Total comments: 2
Patch Set 9 : Rebase #Patch Set 10 : Always respect ALLOW or BLOCK #Patch Set 11 : Address nits #
Total comments: 12
Patch Set 12 : Address comments #
Total comments: 4
Patch Set 13 : Comments #Patch Set 14 : Address nit #Messages
Total messages: 107 (74 generated)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Description was changed from ========== [HBD] Gate the advertising of Flash on site engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. If the site does not have sufficient engagement, an observer is registered that will call PluginService::PurgePluginListCache() if the site's engagement reaches the threshold before navigation. This ensures that dynamic Flash content will work correctly if a user has used a site sufficiently. A future CL will move the site engagement code out of an anonymous namespace and into a more generic location. This will be needed when the Flash prompt is implemented. BUG=626728 ========== to ========== [HBD] Gate the advertising of Flash on site engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. If the site does not have sufficient engagement, an observer is registered that will call PluginService::PurgePluginListCache() if the site's engagement reaches the threshold before navigation. This ensures that dynamic Flash content will work correctly if a user has used a site sufficiently. A future CL will move the site engagement code out of an anonymous namespace and into a more generic location. This will be needed when the Flash prompt is implemented. BUG=626728 ==========
dominickn@chromium.org changed reviewers: + tommycli@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thanks!
+cc calamity
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...) linux_chromium_asan_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 dominickn@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: This issue passed the CQ dry run.
dominickn: Thanks for the quick turnaround. Here you are. A unit test would be good too since this is a pretty complicated thing. https://codereview.chromium.org/2285553002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2285553002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:111: : profile_(profile), url_() {} nit: I I don't think you'll need to explicitly call url_() in the initializer list. https://codereview.chromium.org/2285553002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:116: const GURL& url) { Since we only want the origin, maybe the parameter should just be a url::Origin type https://codereview.chromium.org/2285553002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:128: Observe(nullptr, nullptr, GURL::EmptyGURL()); Actually I'm not sure if we want to stop observing on navigations. If the navigation is to the same site origin, I don't think Blink actually re-requests the plugin list, so we won't begin observing again... https://codereview.chromium.org/2285553002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:160: Profile* profile; Man I really think this class should be per-profile now instead of with this wonky Profile* -> ContextInfo map That can be in a separate CL though https://codereview.chromium.org/2285553002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:290: service, url); The main issue I'm seeing is: we might want to observe on multiple origins. A single profile could access both a.com (too low threshold), and then b.com (too low threshold), and purge the Blink-held plugin list when either of those two domains exceeds the threshold. Ideally, the PurgePluginListCache can take an origin so we don't purge every single renderer's cached plugin list, only certain domains, but that can be a future optimization.
one further comment: Since I'm guessing managing multiple FlashEngagementObservers might be non-trivial. It might be easier to split that out into a separate CL
Regarding the unit test: There's a lot of machinery here (and there's no existing unit test to follow) - could you give some guidance on how to set one up properly for Flash? Regarding managing FlashEngagementObservers: I think the easiest way to manage them is to only have one per profile, and call PurgePluginListCache with the origin for which engagement has just reached the right threshold for. It may be worth implementing an origin-scoped version of that method before this one lands now that I think about it. WDYT?
On 2016/08/26 21:48:28, dominickn wrote: > Regarding the unit test: There's a lot of machinery here (and there's no > existing unit test to follow) - could you give some guidance on how to set one > up properly for Flash? > > Regarding managing FlashEngagementObservers: I think the easiest way to manage > them is to only have one per profile, and call PurgePluginListCache with the > origin for which engagement has just reached the right threshold for. It may be > worth implementing an origin-scoped version of that method before this one lands > now that I think about it. WDYT? Re: unit_test, you might be out of luck. If it's too hard, a separate CL seems fine to me. It just requires one before we can call this feature "done" since it's so complicated. Re multiple observers: Yes, I agree that one observer per profile makes sense. You'll just have to keep track of which origins you care about internally. Re enhancing PurgePluginListCache: Seems desirable from an engineering standpoint, but not strictly-speaking required to launch HBD. Costs 1 extra IPC Roundtrip per renderer of nonmatching origin if we do the simple thing of purging every renderer's list. I leave it to your discretion as to whether you want to tackle that one. Tommy
Additionally, this CL is going to get more complicated because I've just realised that IsPluginAvailable is running on the IO thread, and that DCHECKs when trying to access the SiteEngagementService. We need to be on the UI thread for site engagement.
On 2016/08/26 21:55:41, dominickn wrote: > Additionally, this CL is going to get more complicated because I've just > realised that IsPluginAvailable is running on the IO thread, and that DCHECKs > when trying to access the SiteEngagementService. We need to be on the UI thread > for site engagement. Arg. That is unfortunate. Re unit tests: Maybe look at https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_info_messa...
On 2016/08/26 22:04:44, tommycli wrote: > On 2016/08/26 21:55:41, dominickn wrote: > > Additionally, this CL is going to get more complicated because I've just > > realised that IsPluginAvailable is running on the IO thread, and that DCHECKs > > when trying to access the SiteEngagementService. We need to be on the UI > thread > > for site engagement. > > Arg. That is unfortunate. > > Re unit tests: Maybe look at > https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_info_messa... Next problem: the Blink getPlugins() call is routed through RenderFrameMessageFilter::GetPluginsCallback, which calls ChromePluginServiceFilter::IsPluginAvailable with an empty GURL object. That means that we don't have a URL to query site engagement with anyway. On top of that, the render_frame_id and render_process_id passed by GetPluginsCallback are -1 and NONE respectively, so we can't even use those to identify a URL (or fetch a WebContents for that matter). It looks like this code is run independent of the URL and it'll need to be refactored. I think the correct way to handle the threading issue is to post to the UI thread from RenderFrameMessageFilter::GetPluginsCallback to retrieve the site engagement scores as a std::map, and then pass the map to ChromePluginServiceFilter::IsPluginAvailable. That doesn't solve the lack of a URL or the inability to get a WebContents though, both of which we're going to need. I'm at the very edge of my expertise here - my naive expectation is that there should be some way to pass the origin up from Blink via IPC. tommy, can you provide any more pointers / cc people with more knowledge about this code?
Yep exactly. The CL to pass the origin from Blink is here. It was a ton of work by Tomas. https://codereview.chromium.org/2156803002/ On Aug 28, 2016 8:03 PM, <dominickn@chromium.org> wrote: > On 2016/08/26 22:04:44, tommycli wrote: > > On 2016/08/26 21:55:41, dominickn wrote: > > > Additionally, this CL is going to get more complicated because I've > just > > > realised that IsPluginAvailable is running on the IO thread, and that > DCHECKs > > > when trying to access the SiteEngagementService. We need to be on the > UI > > thread > > > for site engagement. > > > > Arg. That is unfortunate. > > > > Re unit tests: Maybe look at > > > https://cs.chromium.org/chromium/src/chrome/browser/ > plugins/plugin_info_message_filter_unittest.cc?sq=package:chromium > > Next problem: the Blink getPlugins() call is routed through > RenderFrameMessageFilter::GetPluginsCallback, which calls > ChromePluginServiceFilter::IsPluginAvailable with an empty GURL object. > That > means that we don't have a URL to query site engagement with anyway. > > On top of that, the render_frame_id and render_process_id passed by > GetPluginsCallback are -1 and NONE respectively, so we can't even use > those to > identify a URL (or fetch a WebContents for that matter). It looks like > this code > is run independent of the URL and it'll need to be refactored. > > I think the correct way to handle the threading issue is to post to the UI > thread from RenderFrameMessageFilter::GetPluginsCallback to retrieve the > site > engagement scores as a std::map, and then pass the map to > ChromePluginServiceFilter::IsPluginAvailable. That doesn't solve the lack > of a > URL or the inability to get a WebContents though, both of which we're > going to > need. I'm at the very edge of my expertise here - my naive expectation is > that > there should be some way to pass the origin up from Blink via IPC. > > tommy, can you provide any more pointers / cc people with more knowledge > about > this code? > > https://codereview.chromium.org/2285553002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh excellent! Assuming that works and lands, that will solve the origin issue. Then all I have to add here is the hop to the UI thread. :)
Description was changed from ========== [HBD] Gate the advertising of Flash on site engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. If the site does not have sufficient engagement, an observer is registered that will call PluginService::PurgePluginListCache() if the site's engagement reaches the threshold before navigation. This ensures that dynamic Flash content will work correctly if a user has used a site sufficiently. A future CL will move the site engagement code out of an anonymous namespace and into a more generic location. This will be needed when the Flash prompt is implemented. BUG=626728 ========== to ========== [HBD] Gate the advertising of Flash on site engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-safe interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the thread safety of the underlying HostContentSettingsMap class which holds engagement scores. BUG=626728,641627,641630 ==========
Description was changed from ========== [HBD] Gate the advertising of Flash on site engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-safe interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the thread safety of the underlying HostContentSettingsMap class which holds engagement scores. BUG=626728,641627,641630 ========== to ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-safe interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the thread safety of the underlying HostContentSettingsMap class which holds engagement scores. BUG=626728,641627,641630 ==========
The CQ bit was checked by dominickn@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_asan_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 dominickn@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...
Description was changed from ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-safe interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the thread safety of the underlying HostContentSettingsMap class which holds engagement scores. BUG=626728,641627,641630 ========== to ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-safe interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the thread safety of the underlying HostContentSettingsMap class which holds engagement scores. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. BUG=626728,641627,641630 ==========
dominickn@chromium.org changed reviewers: + calamity@chromium.org
Revamped version: * adds a thread safe interface to site engagement that can be accessed from the IO thread * adds unit tests for the new interface * refactors site engagement's incognito handling because it's more complex than necessary * adds unit tests for ChromePluginServiceFilter::IsPluginAvailable PTAL, thanks!
The CQ bit was checked by dominickn@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...
Patchset #5 (id:70001) has been deleted
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
Description was changed from ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, each site's enaggement score is now consulted to determine whether or not Flash is added to the lsit of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-safe interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the thread safety of the underlying HostContentSettingsMap class which holds engagement scores. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. BUG=626728,641627,641630 ========== to ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. BUG=626728,641627,641630 ==========
https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:239: if (!UpdateScoreDict(score_dict_.get()) || !settings_map_) Is the settings_map_ ever null? We may want to DCHECK it instead. https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:179: const GURL& origin, Is this used anywhere? https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:191: void CheckScoreFromSettingsOnThread( Does this actually send things to a different thread? The base class seems to have a TestBrowserThreadBundle which just pretends threads exist. You may have to use a legit browser test for this so that real threads are instantiated. Perhaps consult the thread wizards. https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:71: ChromeRenderViewHostTestHarness::SetUp(); You should call SiteEngagementScore::SetParamValuesForTesting() here.
The CQ bit was checked by dominickn@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...
https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:239: if (!UpdateScoreDict(score_dict_.get()) || !settings_map_) On 2016/09/13 04:14:37, calamity wrote: > Is the settings_map_ ever null? We may want to DCHECK it instead. It could be nullptr in tests (where it uses the private constructor that doesn't initialize settings_map_). The constructor also doesn't assume that it gets a non-nullptr (e.g the nullptr check in GetScoreDictForSettings). Unless we mandate that a non-null pointer is passed in during tests as well, I don't think that it makes sense to DCHECK here, since you could legitimately create a SiteEngagementScore using a nullptr. Otherwise, we should go the whole hog and enforce a non-nullptr settings_map_ - including in the private constructor. That makes SiteEngagementScore tests more complicated (that class would probably need to be converted to subclass ChromeRenderViewHostTestHarness so that there's a profile() etc. around). https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:179: const GURL& origin, On 2016/09/13 04:14:37, calamity wrote: > Is this used anywhere? The GURL argument? I pulled it down so that there would be one less argument outside the initializer list. It made sense just to have that in the delegated constructor. The underlying origin_ member is used as the key when saving to content settings. https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:191: void CheckScoreFromSettingsOnThread( On 2016/09/13 04:14:37, calamity wrote: > Does this actually send things to a different thread? The base class seems to > have a TestBrowserThreadBundle which just pretends threads exist. You may have > to use a legit browser test for this so that real threads are instantiated. > > Perhaps consult the thread wizards. I think if I set the thread options as REAL_IO_THREAD | REAL_DB_THREAD, it will be backed by real message loops for those threads (see test_browser_thread_bundle.h). Added that in the constructor for this class. https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:71: ChromeRenderViewHostTestHarness::SetUp(); On 2016/09/13 04:14:37, calamity wrote: > You should call SiteEngagementScore::SetParamValuesForTesting() here. Done.
Description was changed from ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. BUG=626728,641627,641630 ========== to ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter.cc:209: } Hmm I think this might cause non-pepper plugins to be disabled? This check should perhaps go inside GetPluginContentSetting()
https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter.cc:209: } On 2016/09/13 06:00:04, raymes wrote: > Hmm I think this might cause non-pepper plugins to be disabled? This check > should perhaps go inside GetPluginContentSetting() This branch is only taken when the plugin is Flash and the PreferHtmlOverPlugins feature is active, so any other plugin won't ever enter this branch and hit this check.
site_engagement lgtm. https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:239: if (!UpdateScoreDict(score_dict_.get()) || !settings_map_) On 2016/09/13 04:42:14, dominickn wrote: > On 2016/09/13 04:14:37, calamity wrote: > > Is the settings_map_ ever null? We may want to DCHECK it instead. > > It could be nullptr in tests (where it uses the private constructor that doesn't > initialize settings_map_). The constructor also doesn't assume that it gets a > non-nullptr (e.g the nullptr check in GetScoreDictForSettings). > > Unless we mandate that a non-null pointer is passed in during tests as well, I > don't think that it makes sense to DCHECK here, since you could legitimately > create a SiteEngagementScore using a nullptr. Otherwise, we should go the whole > hog and enforce a non-nullptr settings_map_ - including in the private > constructor. That makes SiteEngagementScore tests more complicated (that class > would probably need to be converted to subclass ChromeRenderViewHostTestHarness > so that there's a profile() etc. around). To me, the condition the DCHECK here enforces is 'You shouldn't be calling Commit() without a settings map'. The existing tests don't do this, and future tests never should. I think it would be a useful tripwire that we can remove later if it proves to be inconvenient. https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:179: const GURL& origin, On 2016/09/13 04:42:14, dominickn wrote: > On 2016/09/13 04:14:37, calamity wrote: > > Is this used anywhere? > > The GURL argument? I pulled it down so that there would be one less argument > outside the initializer list. It made sense just to have that in the delegated > constructor. > > The underlying origin_ member is used as the key when saving to content > settings. Seems weird to have a param that's never used but I'll leave it to you. https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:191: void CheckScoreFromSettingsOnThread( On 2016/09/13 04:42:14, dominickn wrote: > On 2016/09/13 04:14:37, calamity wrote: > > Does this actually send things to a different thread? The base class seems to > > have a TestBrowserThreadBundle which just pretends threads exist. You may have > > to use a legit browser test for this so that real threads are instantiated. > > > > Perhaps consult the thread wizards. > > I think if I set the thread options as REAL_IO_THREAD | REAL_DB_THREAD, it will > be backed by real message loops for those threads (see > test_browser_thread_bundle.h). Added that in the constructor for this class. Ah cool.
dominickn: Awesome, I'm very pumped about the through unit tests you added to ChromePluginServiceFilter. That greatly increases my confidence that our code actually works. Thanks! See comments below. https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Nit since 2014 or something, we just removed the (c), so it's just "Copyright 2016" in the newer files. https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:46: public: nit: Indentation in this class looks funny. git cl format? https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:54: return PluginPrefs::GetForTestingProfile(profile->GetOriginalProfile()); It seems odd that there's a special clause for incognito profiles here, but not in the production code. How is it handled in the production code? Also, it seems odd that PluginPrefs::GetForTestingProfile exists in the current code, but is never called. In general, is there a way we can avoid introducing this overridden method? Reading the comment for PluginPrefs::GetForTestingProfile, it seems like it registers a valid PluginPrefs instance for the testing profile. Maybe we just need to call it in SetUp? https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:85: void IsPluginAvailable(const GURL& url, For clarity, I recommend just making this method return a boolean. I think you can do that with this kind of pattern: bool IsPluginAvailable(...) { bool is_available = false; base::RunLoop run_loop; content::BrowserThread::PostTask( ... base::Bind(..., &is_available); ); run_loop.Run(); return is_available; } (Sanity check me to make sure there's no thread safety issues) And then use explicit EXPECT_TRUE, EXPECT_FALSE in the test bodies. https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:123: InitializeOnUIThread(false); It's odd to call this method at the beginning of each test. Can we consolidate this within SetUp? For the incognito case, I would recommend just "manually" registering the resource context within the body of the test, since it's harmless to register both the actual and the incognito profile. Is the incognito profile actually registered in production though? https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:137: // Activate PreferHtmlOverPlugins. Must be done inline or else Does the ScopedFeatureList work here? https://cs.chromium.org/chromium/src/chrome/browser/plugins/flash_download_in... https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:280: TEST_F(ChromePluginServiceFilterTest, PreferHtmlOverPluginsIncognito) { Very glad we are also testing Incognito mode. Is there a reason we are not testing the exact same stuff within Incognito mode though? (Testing BLOCK instead of ALLOW and DETECT)? Should we test all 6 combos? I'm not sure myself - what are the implications of Incoginto mode with site engagement?
The CQ bit was checked by dominickn@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...
Thanks! https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:239: if (!UpdateScoreDict(score_dict_.get()) || !settings_map_) On 2016/09/13 06:33:34, calamity wrote: > On 2016/09/13 04:42:14, dominickn wrote: > > On 2016/09/13 04:14:37, calamity wrote: > > > Is the settings_map_ ever null? We may want to DCHECK it instead. > > > > It could be nullptr in tests (where it uses the private constructor that > doesn't > > initialize settings_map_). The constructor also doesn't assume that it gets a > > non-nullptr (e.g the nullptr check in GetScoreDictForSettings). > > > > Unless we mandate that a non-null pointer is passed in during tests as well, I > > don't think that it makes sense to DCHECK here, since you could legitimately > > create a SiteEngagementScore using a nullptr. Otherwise, we should go the > whole > > hog and enforce a non-nullptr settings_map_ - including in the private > > constructor. That makes SiteEngagementScore tests more complicated (that class > > would probably need to be converted to subclass > ChromeRenderViewHostTestHarness > > so that there's a profile() etc. around). > > To me, the condition the DCHECK here enforces is 'You shouldn't be calling > Commit() without a settings map'. The existing tests don't do this, and future > tests never should. I think it would be a useful tripwire that we can remove > later if it proves to be inconvenient. Done. https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:179: const GURL& origin, On 2016/09/13 06:33:34, calamity wrote: > On 2016/09/13 04:42:14, dominickn wrote: > > On 2016/09/13 04:14:37, calamity wrote: > > > Is this used anywhere? > > > > The GURL argument? I pulled it down so that there would be one less argument > > outside the initializer list. It made sense just to have that in the delegated > > constructor. > > > > The underlying origin_ member is used as the key when saving to content > > settings. > > Seems weird to have a param that's never used but I'll leave it to you. Acknowledged. https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/09/13 18:25:04, tommycli wrote: > Nit since 2014 or something, we just removed the (c), so it's just "Copyright > 2016" in the newer files. Done. https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:46: public: On 2016/09/13 18:25:04, tommycli wrote: > nit: Indentation in this class looks funny. git cl format? Done. https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:54: return PluginPrefs::GetForTestingProfile(profile->GetOriginalProfile()); On 2016/09/13 18:25:04, tommycli wrote: > It seems odd that there's a special clause for incognito profiles here, but not > in the production code. How is it handled in the production code? > > Also, it seems odd that PluginPrefs::GetForTestingProfile exists in the current > code, but is never called. > > In general, is there a way we can avoid introducing this overridden method? > Reading the comment for PluginPrefs::GetForTestingProfile, it seems like it > registers a valid PluginPrefs instance for the testing profile. Maybe we just > need to call it in SetUp? Ah, the trick here was calling it in SetUp - I tried a variety of other permutations. Thanks! https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:85: void IsPluginAvailable(const GURL& url, On 2016/09/13 18:25:04, tommycli wrote: > For clarity, I recommend just making this method return a boolean. > > I think you can do that with this kind of pattern: > > bool IsPluginAvailable(...) { > bool is_available = false; > > base::RunLoop run_loop; > content::BrowserThread::PostTask( > ... > base::Bind(..., &is_available); > ); > run_loop.Run(); > > return is_available; > } > > (Sanity check me to make sure there's no thread safety issues) And then use > explicit EXPECT_TRUE, EXPECT_FALSE in the test bodies. Done - good suggestion! https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:123: InitializeOnUIThread(false); On 2016/09/13 18:25:04, tommycli wrote: > It's odd to call this method at the beginning of each test. Can we consolidate > this within SetUp? > > For the incognito case, I would recommend just "manually" registering the > resource context within the body of the test, since it's harmless to register > both the actual and the incognito profile. > > Is the incognito profile actually registered in production though? > Done. ChromePluginServiceFilter::RegisterResourceContext is called in both the regular profile impl init and the off the record profile impl init, so I assume that incognito will be registered separately. https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:137: // Activate PreferHtmlOverPlugins. Must be done inline or else On 2016/09/13 18:25:04, tommycli wrote: > Does the ScopedFeatureList work here? > https://cs.chromium.org/chromium/src/chrome/browser/plugins/flash_download_in... Ah, that's cool, hadn't seen it before. That works for the simple case of turning the feature on and off, but I think I still need to do the more complicated setup in order to test the custom parameter (since there's currently only support for explicitly associating a variations parameter by field trial and group, but not by feature). https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:280: TEST_F(ChromePluginServiceFilterTest, PreferHtmlOverPluginsIncognito) { On 2016/09/13 18:25:04, tommycli wrote: > Very glad we are also testing Incognito mode. Is there a reason we are not > testing the exact same stuff within Incognito mode though? (Testing BLOCK > instead of ALLOW and DETECT)? Should we test all 6 combos? I'm not sure myself - > what are the implications of Incoginto mode with site engagement? All content settings should behave the same in incognito - values are read from the original profile if they exist there, but are written to the temporary incognito profile. Site engagement has separate tests for incognito that verify this behaviour, as does HostContentSettingsMap (HostContentSettingsMapTest::OffTheRecord). I did think of one more combination - changes to ALLOW/BLOCK etc. should be reflected immediately from the original profile to the incognito one, so I added a test for that. But otherwise, I felt comfortable with the coverage from other unit tests to not cover all the permutations. WDYT?
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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@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...
Description was changed from ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. Otherwise, the requisite content setting will be consulted to check whether to offer Flash. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 ========== to ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, and the plugins content setting isn't one of ALLOW or BLOCK, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 ==========
On 2016/09/14 01:06:56, dominickn wrote: > Thanks! > > https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_score.cc (right): > > https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_score.cc:239: if > (!UpdateScoreDict(score_dict_.get()) || !settings_map_) > On 2016/09/13 06:33:34, calamity wrote: > > On 2016/09/13 04:42:14, dominickn wrote: > > > On 2016/09/13 04:14:37, calamity wrote: > > > > Is the settings_map_ ever null? We may want to DCHECK it instead. > > > > > > It could be nullptr in tests (where it uses the private constructor that > > doesn't > > > initialize settings_map_). The constructor also doesn't assume that it gets > a > > > non-nullptr (e.g the nullptr check in GetScoreDictForSettings). > > > > > > Unless we mandate that a non-null pointer is passed in during tests as well, > I > > > don't think that it makes sense to DCHECK here, since you could legitimately > > > create a SiteEngagementScore using a nullptr. Otherwise, we should go the > > whole > > > hog and enforce a non-nullptr settings_map_ - including in the private > > > constructor. That makes SiteEngagementScore tests more complicated (that > class > > > would probably need to be converted to subclass > > ChromeRenderViewHostTestHarness > > > so that there's a profile() etc. around). > > > > To me, the condition the DCHECK here enforces is 'You shouldn't be calling > > Commit() without a settings map'. The existing tests don't do this, and future > > tests never should. I think it would be a useful tripwire that we can remove > > later if it proves to be inconvenient. > > Done. > > https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... > File chrome/browser/engagement/site_engagement_score.h (right): > > https://codereview.chromium.org/2285553002/diff/110001/chrome/browser/engagem... > chrome/browser/engagement/site_engagement_score.h:179: const GURL& origin, > On 2016/09/13 06:33:34, calamity wrote: > > On 2016/09/13 04:42:14, dominickn wrote: > > > On 2016/09/13 04:14:37, calamity wrote: > > > > Is this used anywhere? > > > > > > The GURL argument? I pulled it down so that there would be one less argument > > > outside the initializer list. It made sense just to have that in the > delegated > > > constructor. > > > > > > The underlying origin_ member is used as the key when saving to content > > > settings. > > > > Seems weird to have a param that's never used but I'll leave it to you. > > Acknowledged. > > https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... > File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): > > https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... > chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:1: // Copyright > (c) 2016 The Chromium Authors. All rights reserved. > On 2016/09/13 18:25:04, tommycli wrote: > > Nit since 2014 or something, we just removed the (c), so it's just "Copyright > > 2016" in the newer files. > > Done. > > https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... > chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:46: public: > On 2016/09/13 18:25:04, tommycli wrote: > > nit: Indentation in this class looks funny. git cl format? > > Done. > > https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... > chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:54: return > PluginPrefs::GetForTestingProfile(profile->GetOriginalProfile()); > On 2016/09/13 18:25:04, tommycli wrote: > > It seems odd that there's a special clause for incognito profiles here, but > not > > in the production code. How is it handled in the production code? > > > > Also, it seems odd that PluginPrefs::GetForTestingProfile exists in the > current > > code, but is never called. > > > > In general, is there a way we can avoid introducing this overridden method? > > Reading the comment for PluginPrefs::GetForTestingProfile, it seems like it > > registers a valid PluginPrefs instance for the testing profile. Maybe we just > > need to call it in SetUp? > > Ah, the trick here was calling it in SetUp - I tried a variety of other > permutations. Thanks! > > https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... > chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:85: void > IsPluginAvailable(const GURL& url, > On 2016/09/13 18:25:04, tommycli wrote: > > For clarity, I recommend just making this method return a boolean. > > > > I think you can do that with this kind of pattern: > > > > bool IsPluginAvailable(...) { > > bool is_available = false; > > > > base::RunLoop run_loop; > > content::BrowserThread::PostTask( > > ... > > base::Bind(..., &is_available); > > ); > > run_loop.Run(); > > > > return is_available; > > } > > > > (Sanity check me to make sure there's no thread safety issues) And then use > > explicit EXPECT_TRUE, EXPECT_FALSE in the test bodies. > > Done - good suggestion! > > https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... > chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:123: > InitializeOnUIThread(false); > On 2016/09/13 18:25:04, tommycli wrote: > > It's odd to call this method at the beginning of each test. Can we consolidate > > this within SetUp? > > > > For the incognito case, I would recommend just "manually" registering the > > resource context within the body of the test, since it's harmless to register > > both the actual and the incognito profile. > > > > Is the incognito profile actually registered in production though? > > > > Done. > > ChromePluginServiceFilter::RegisterResourceContext is called in both the regular > profile impl init and the off the record profile impl init, so I assume that > incognito will be registered separately. > > https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... > chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:137: // Activate > PreferHtmlOverPlugins. Must be done inline or else > On 2016/09/13 18:25:04, tommycli wrote: > > Does the ScopedFeatureList work here? > > > https://cs.chromium.org/chromium/src/chrome/browser/plugins/flash_download_in... > > Ah, that's cool, hadn't seen it before. That works for the simple case of > turning the feature on and off, but I think I still need to do the more > complicated setup in order to test the custom parameter (since there's currently > only support for explicitly associating a variations parameter by field trial > and group, but not by feature). > > https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... > chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:280: > TEST_F(ChromePluginServiceFilterTest, PreferHtmlOverPluginsIncognito) { > On 2016/09/13 18:25:04, tommycli wrote: > > Very glad we are also testing Incognito mode. Is there a reason we are not > > testing the exact same stuff within Incognito mode though? (Testing BLOCK > > instead of ALLOW and DETECT)? Should we test all 6 combos? I'm not sure myself > - > > what are the implications of Incoginto mode with site engagement? > > All content settings should behave the same in incognito - values are read from > the original profile if they exist there, but are written to the temporary > incognito profile. Site engagement has separate tests for incognito that verify > this behaviour, as does HostContentSettingsMap > (HostContentSettingsMapTest::OffTheRecord). > > I did think of one more combination - changes to ALLOW/BLOCK etc. should be > reflected immediately from the original profile to the incognito one, so I added > a test for that. But otherwise, I felt comfortable with the coverage from other > unit tests to not cover all the permutations. WDYT? Dom: After our meeting, I was thinking: We should probably apply the same logic for when to hide Flash to when to intercept the "Download Flash" link. In other words, only intercept Flash download navigations when we've also hidden it from the list. We could call the same logic as in the body of IsPluginAvailable - but the only hole is that the site's SEI score may have changed, and so it wouldn't be foolproof. Another thing is maybe we could mark a flag somewhere that we've hidden the Flash plugin, but then it's not clear to me where to do it - perhaps within ChromePluginServiceFilter, but it would really be per-tab. This comment probably doesn't directly apply to this CL, but just something to think about... I will review the other code in detail tomorrow morning after a good nights sleep.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm very through tests. thank you! https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/130001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:280: TEST_F(ChromePluginServiceFilterTest, PreferHtmlOverPluginsIncognito) { On 2016/09/14 01:06:56, dominickn wrote: > On 2016/09/13 18:25:04, tommycli wrote: > > Very glad we are also testing Incognito mode. Is there a reason we are not > > testing the exact same stuff within Incognito mode though? (Testing BLOCK > > instead of ALLOW and DETECT)? Should we test all 6 combos? I'm not sure myself > - > > what are the implications of Incoginto mode with site engagement? > > All content settings should behave the same in incognito - values are read from > the original profile if they exist there, but are written to the temporary > incognito profile. Site engagement has separate tests for incognito that verify > this behaviour, as does HostContentSettingsMap > (HostContentSettingsMapTest::OffTheRecord). > > I did think of one more combination - changes to ALLOW/BLOCK etc. should be > reflected immediately from the original profile to the incognito one, so I added > a test for that. But otherwise, I felt comfortable with the coverage from other > unit tests to not cover all the permutations. WDYT? Acknowledged. https://codereview.chromium.org/2285553002/diff/150001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/150001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:64: content::BrowserThread::PostTask( nit: you can actually use PostTaskAndReply with the QuitClosure as the last argument to further make your code a bit simpler
The CQ bit was checked by dominickn@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...
dominickn@chromium.org changed reviewers: + bauerb@chromium.org
Thanks! +bauerb: PTAL at chrome/browser/plugins. Thanks! https://codereview.chromium.org/2285553002/diff/150001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/150001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:64: content::BrowserThread::PostTask( On 2016/09/14 17:12:38, tommycli wrote: > nit: you can actually use PostTaskAndReply with the QuitClosure as the last > argument to further make your code a bit simpler Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #11 (id:210001) has been deleted
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:50: return base::WrapUnique(static_cast<base::DictionaryValue*>(value.release())); There is now DictionaryValue::From(), which will do the downcast in a safe manner and work on directly on unique_ptr<>s. https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:218: content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, You can also use PostTaskAndReply here. https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter.cc:228: if (policy_url.is_empty() || When would this be the case again? https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:52: SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD); Why do you need a real IO thread? https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... File chrome/browser/plugins/plugins_field_trial.h (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... chrome/browser/plugins/plugins_field_trial.h:29: static bool HasSufficientEngagementForFlash(HostContentSettingsMap* settings, I think it would make more sense to expose the engagement threshold from this class rather than this method that just delegates to SiteEngagementService.
The CQ bit was checked by dominickn@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...
Thanks - PTAnL! https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:50: return base::WrapUnique(static_cast<base::DictionaryValue*>(value.release())); On 2016/09/15 09:05:15, Bernhard Bauer wrote: > There is now DictionaryValue::From(), which will do the downcast in a safe > manner and work on directly on unique_ptr<>s. Great, done. https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service_unittest.cc:218: content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, On 2016/09/15 09:05:15, Bernhard Bauer wrote: > You can also use PostTaskAndReply here. Done. https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter.cc:228: if (policy_url.is_empty() || On 2016/09/15 09:05:15, Bernhard Bauer wrote: > When would this be the case again? Changed to DCHECK. https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:52: SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD); On 2016/09/15 09:05:15, Bernhard Bauer wrote: > Why do you need a real IO thread? Can we get away with testing without a real IO thread? It was suggested to add this to make sure that we were actually posting to different message loops (e.g. in the site engagement service test too). Happy to take advice here. https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... File chrome/browser/plugins/plugins_field_trial.h (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... chrome/browser/plugins/plugins_field_trial.h:29: static bool HasSufficientEngagementForFlash(HostContentSettingsMap* settings, On 2016/09/15 09:05:15, Bernhard Bauer wrote: > I think it would make more sense to expose the engagement threshold from this > class rather than this method that just delegates to SiteEngagementService. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:52: SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD); On 2016/09/15 12:16:00, dominickn wrote: > On 2016/09/15 09:05:15, Bernhard Bauer wrote: > > Why do you need a real IO thread? > > Can we get away with testing without a real IO thread? It was suggested to add > this to make sure that we were actually posting to different message loops (e.g. > in the site engagement service test too). Happy to take advice here. I think the test would still work if you use the same message loop, or even call IsPluginAvailable() from the main thread. I'm not really sure what using a different message loop would test -- thread safety tends not to be a thing that is easily testable. https://codereview.chromium.org/2285553002/diff/250001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/250001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:48: if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY)) You could get rid of this check now if you instead handle DictionaryValue::From() returning null.
The CQ bit was checked by dominickn@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...
https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... File chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc (right): https://codereview.chromium.org/2285553002/diff/230001/chrome/browser/plugins... chrome/browser/plugins/chrome_plugin_service_filter_unittest.cc:52: SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD); On 2016/09/15 15:39:54, Bernhard Bauer wrote: > On 2016/09/15 12:16:00, dominickn wrote: > > On 2016/09/15 09:05:15, Bernhard Bauer wrote: > > > Why do you need a real IO thread? > > > > Can we get away with testing without a real IO thread? It was suggested to add > > this to make sure that we were actually posting to different message loops > (e.g. > > in the site engagement service test too). Happy to take advice here. > > I think the test would still work if you use the same message loop, or even call > IsPluginAvailable() from the main thread. I'm not really sure what using a > different message loop would test -- thread safety tends not to be a thing that > is easily testable. I don't think it's appropriate to call IsPluginAvailable from the UI thread, because it's only run from the UI thread. The first iteration of this CL assumed that it would be fine, but you're not allowed to create a SiteEngagementService object from any thread but the UI thread (due to the underlying keyed service factory - you should get a DCHECK failure if this happens). So I added the new methods in which can access scores without creating a SiteEngagementService, and hence it's important to actually run these tests on the IO thread to ensure that if you violate a DCHECK_CURRENTLY_ON or a thread validity assumption, it crashes. In any case, I've removed the bundle options. https://codereview.chromium.org/2285553002/diff/250001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/250001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:48: if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY)) On 2016/09/15 15:39:54, Bernhard Bauer wrote: > You could get rid of this check now if you instead handle > DictionaryValue::From() returning null. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2285553002/diff/250001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/250001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:48: if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY)) On 2016/09/16 00:26:01, dominickn wrote: > On 2016/09/15 15:39:54, Bernhard Bauer wrote: > > You could get rid of this check now if you instead handle > > DictionaryValue::From() returning null. > > Done. Sorry, what I meant was: DictionaryValue::From() includes a type check and will return null in that case. So you can move the call to DictionaryValue::From() up and check whether the returned value is null, and return an empty dictionary in that case. Right now you will return null if the value is of a different type, which is probably not what you want.
We've been through a few rounds of nits on this now. Are there any more substantive comments on the contents of the CL? There's a few other things that this is holding up now, and the M55 branch is coming up in a few weeks. https://codereview.chromium.org/2285553002/diff/250001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.cc (right): https://codereview.chromium.org/2285553002/diff/250001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.cc:48: if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY)) On 2016/09/16 10:32:38, Bernhard (slow until Sep 27) wrote: > On 2016/09/16 00:26:01, dominickn wrote: > > On 2016/09/15 15:39:54, Bernhard Bauer wrote: > > > You could get rid of this check now if you instead handle > > > DictionaryValue::From() returning null. > > > > Done. > > Sorry, what I meant was: > DictionaryValue::From() includes a type check and will return null in that case. > So you can move the call to DictionaryValue::From() up and check whether the > returned value is null, and return an empty dictionary in that case. Right now > you will return null if the value is of a different type, which is probably not > what you want. Done.
lgtm
The CQ bit was checked by dominickn@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: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org, tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2285553002/#ps290001 (title: "Address nit")
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] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, and the plugins content setting isn't one of ALLOW or BLOCK, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 ========== to ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, and the plugins content setting isn't one of ALLOW or BLOCK, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:290001)
Message was sent while issue was closed.
Description was changed from ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, and the plugins content setting isn't one of ALLOW or BLOCK, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 ========== to ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, and the plugins content setting isn't one of ALLOW or BLOCK, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 Committed: https://crrev.com/8583adb8ee4711d11421d53579567bc68eaf4d54 Cr-Commit-Position: refs/heads/master@{#419623} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/8583adb8ee4711d11421d53579567bc68eaf4d54 Cr-Commit-Position: refs/heads/master@{#419623}
Message was sent while issue was closed.
On 2016/09/20 01:26:55, commit-bot: I haz the power wrote: > Patchset 14 (id:??) landed as > https://crrev.com/8583adb8ee4711d11421d53579567bc68eaf4d54 > Cr-Commit-Position: refs/heads/master@{#419623} Woohoo, thanks for this!
Message was sent while issue was closed.
Description was changed from ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, and the plugins content setting isn't one of ALLOW or BLOCK, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 Committed: https://crrev.com/8583adb8ee4711d11421d53579567bc68eaf4d54 Cr-Commit-Position: refs/heads/master@{#419623} ========== to ========== [HBD] Gate the advertising of Flash on Site Engagement. When the PreferHtmlOverPlugins feature is active, and the plugins content setting isn't one of ALLOW or BLOCK, each site's engagement score is now consulted to determine whether or not Flash is added to the list of available plugins. The score must be above a certain threshold for Flash to be advertised. The amount of engagement required to activate Flash defaults to 1, which is currently the equivalent of navigating directly to a site via the omnibox, or spending ~1 minute interacting with the site after visiting via a link. This value is controllable via a variations parameter under the PreferHtmlOverPlugins feature. This CL adds a thread-agnostic interface to the site engagement service, which is necessary as ChromePluginServiceFilter::IsPluginAvailable runs exclusively on the IO thread, from which is it not possible to create a SiteEngagementService object. This new interface relies on the fact that the underlying HostContentSettingsMap class methods can be called from any thread. This CL additionally removes the fallback logic from the site engagement service for incognito support, as this is already baked into the underlying HostContentSettingsMap. A minor cleanup of ChromePluginServiceFilter is also undertaken. BUG=626728,641627,641630 Committed: https://crrev.com/8583adb8ee4711d11421d53579567bc68eaf4d54 Cr-Commit-Position: refs/heads/master@{#419623} ========== |
