Note that it still seems to show up in the OIB.

Sorry for the title changes, I'd forgotten all the correct terminology :)

In any case, we should add ADS to the desktop site details page with the same logic as how it is shown in the OIB:
1. The SubresourceFilterExperimentalUI flag must be present
2. The WebsiteSetting for "subresource_filter_data" must be present.

See ShouldShowPermission(...) in the page_info code:
https://cs.chromium.org/chromium/src/chrome/browser/ui/page_info/page_info.cc?rcl=db031ee9b80f76c9fade75e6a7961280e9f91b97&l=137

Patti: assigning to you for triage purposes

Thanks estark / csharrison. Sorry for the delay in adding this to Site Details - I'd postponed work on it in favour of other deadlines since I wasn't aware of any launch plans, but if there are experiments going out with this on then this should be a priority.

I have a WIP here https://chromium-review.googlesource.com/c/chromium/src/+/717998.

No worries, we are trying to do an initial launch in M63 but if the setting will be in the OIB for that release we should be OK.

@csharrison - question, what should Site Details do when the user clicks "Reset site settings" at the bottom of the page to clear their settings? At the moment in the above CL I linked in #c7, for some reason this sets some data in CONTENT_SETTINGS_TYPES_ADS_DATA and it shows up in page info until you refresh, upon which it disappears. Is this a bug I should be fixing or is the Ads setting never supposed to be set back to CONTENT_SETTING_DEFAULT?

Hm, it is a great question. The code is pretty subtle. I can apply your patch today and take a closer look and get back to you.

In general, we try to populate the ADS_DATA setting on a page load, to know whether or not the page should show the ADS setting in the OIB / Site Details page. This corresponds to the site being on an activation list for ad blocking (e.g. it shows intrusive ads).

How are you testing this feature? In general permission.site should not be on our list and should not be showing the feature in the OIB unless you're running with special flags.

patricialor: I applied your patch and found the issue. The problem is here:

https://cs.chromium.org/chromium/src/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc?rcl=bfa00e890077e86da600bdb50ca3cc0a765a2957&l=207

In general, we had made an assumption that a user manually setting the ADS setting implies that the website setting is already set. I think this assumption is flaky, given this patch so I think the best path forward is to remove the call to SetSiteMetadata in SubresourceFilterContentSettingsManager::OnContentSettingChanged.

This was just used as a UI tweak so I think removing it should be OK in your patch.

Added a blockedon bug, which I'll try to land today.

Thanks very much for the explanation and fix, much appreciated. I'll leave my patch as-is then.

FYI shivanisha@ is working on integrating this setting with an Enterprise policy for M64, so we'll likely be making modifications here afterwards.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03056533c42a4111543537988616d5f75849940f

commit 03056533c42a4111543537988616d5f75849940f
Author: Patti <patricialor@chromium.org>
Date: Tue Oct 24 07:32:25 2017

Settings: Add the Ads permission to Site Details.

The Ads permission is missing from Site Details, which is supposed to show all
site permissions. Add it.

Manual test - Navigate to https://permission.site, open the Page Info bubble by
clicking the icon next to the omnibox, and click "Site settings". Verify the
"Ads" permission shows up on the list and that clicking the drop-down next to
it only shows "Allow" and "Block".

Bug:  774139 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I4e355766a49c670ae378bc0b3856f11e71a94129
Reviewed-on: https://chromium-review.googlesource.com/717998
Commit-Queue: Patti <patricialor@chromium.org>
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511050}
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/app/settings_strings.grdp
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/browser/resources/settings/site_settings/constants.js
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/browser/resources/settings/site_settings/site_details.html
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/browser/resources/settings/site_settings/site_details.js
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/browser/resources/settings/site_settings/site_details_permission.html
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/browser/resources/settings/site_settings/site_details_permission.js
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/browser/ui/webui/site_settings_helper.cc
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/browser/ui/webui/site_settings_helper.h
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/test/data/webui/settings/site_details_permission_tests.js
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/test/data/webui/settings/site_details_tests.js
[modify] https://crrev.com/03056533c42a4111543537988616d5f75849940f/chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js