Looking at the code I don't think removing is the right move since the setting will be shown if it is explicitly chosen by the user. We need to figure out how it got reversed. Patti would you mind looking at this?

I glanced through the code and didn't see anything obvious, and I'm at a conference right now so I don't have a ton of time to debug.

One other clue is that the icon is correct. I.e. when the string says "Allow" the icon shows the block icon, and when the string says "Block" the icon shows the allow icon.

I ended up debugging this to blame the MD setting secondary UI change:
https://chromium-review.googlesource.com/c/chromium/src/+/750444

This indicates it was an existing issue with the setting.

Trent: in the CL you mentioned that this is not launching in M64. Is that still correct? If so, this is probably not a m64 blocker.

I think the fundamental issue is twofold:
1. The combobox expects the index of the selected element to be equal to the content setting value. E.g. DEFAULT = 0, ALLOW = 1, BLOCK = 2.
2. The ads setting does not have a default, so index 0 maps to ALLOW, and 1 maps to BLOCK.

I chatted with Ryan and it should be fine to remove the constraint that the setting always shows up by default if the site shows intrusive ads. Then we can align the setting with all the others and remove special case page_info code.

Note, if we do the approach in #5, we will need to change both the OIB and site details on desktop to show all three options:
1. Block (default)
2. Allow
3. Block

Patti: I uploaded a WIP change [1] that gets us 90% of the way, but I think it might require some internals tweaking with the SiteSettingSource to get it to 100%. Would you mind taking ownership of it?

[1] https://chromium-review.googlesource.com/797446

> Trent: in the CL you mentioned that this is not launching in M64. Is that still correct?

Yes - although after some considered discussions, a slight change is that the flip will happen on the m64 beta track only, shortly after branching, rather than before.

> I chatted with Ryan and it should be fine to remove the constraint that the setting always shows up by default if the site shows intrusive ads. Then we can align the setting with all the others and remove special case page_info code.

Don't we want it to show up in this case so that users can change the setting easily? 

Honestly I think we'd be fine with it either way. It seemed to me like supporting our use case with the combobox would be a bit intrusive but looking at the code, both solutions are non-trivial.

The only thing the current state gives us is if the user wants to change away from the factory default easily, but in those cases (assuming a page with intrusive ads) we already are going to be displaying UI on the right side of the omnibox.

> It seemed to me like supporting our use case with the combobox would be a bit intrusive but looking at the code, both solutions are non-trivial.

Hmm, I'm not quite sure what you mean by non-trivial? Which bit would be hard?

> The only thing the current state gives us is if the user wants to change away from the factory default easily

I think this is more in-line with the behavior we're heading in for other things like sound, where if the setting is contextually relevant, we will display it in Page Info so that users can change it from there too. Page action icons may some day go away.

Patti and I were wondering whether we could simply add the "Block (Default)" option back in and if that would fix things while otherwise keeping the status quo?

Sorry I just meant non-trivial in the truly rigorous "I can code this up while at a conference" :D. I don't think either change will be really hard.

If as you say we're OK having contextually relevant things in Page Info then that makes a lot of sense, I didn't know that was the future vision.

I see two possible solutions if we want to keep the same logic for displaying the setting:
1. Adding Block (Default) back in.
2. Making the combobox be robust to list index != content setting value (honestly I think that should be fixed regardless since it is very fragile design, if I understand it correctly).

I don't have a strong preference here. Ryan (cc'd) can chime in if he has opinions.

raymes/patricialor: Do you think this is doable by M65?

Hey csharrison@ - thank you for filing this, I noticed it recently in crrev.com/c/788638, but haven't followed up yet. RE #c7, if you're asking me to take over the CL, I can take a look with #1 from #c12 as the planned fix. I'll see if #2 is easily fixable as well, but with a lower priority for right now.

RE #c13 - yes :)

Thanks! You don't have to use my CL if it isn't the right approach, but I think it was mostly on the right track.

I think doing (1) from c#12 should be fine, let's just take a few screenshots to quickly run it by product/ui once it's completed.

I really appreciate the help!

Ok, I've got a WIP that does #1 from #c12 here: https://chromium-review.googlesource.com/c/chromium/src/+/813576 - thanks csharrison@ for your WIP, it was indeed mostly done :)

I've also investigated #2 from #c12 - this should probably go in its own patch, but I'm not sure if it's worth fixing at the moment, since we're adding the default option back into the Ads permission. I've filed a separate bug at  Issue 794410  to hopefully save some time in the future if there's another permission that doesn't have the same number of combobox menu items vs ContentSetting values.

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

commit f2a74af762e90c326f4901a80e54652d6de5f4f8
Author: Patti <patricialor@chromium.org>
Date: Thu Dec 21 04:43:41 2017

Page Info Desktop/Site Details: Add default setting back for Ads permission.

The Ads setting currently doesn't have a default option listed in the drop-down
displayed in PageInfoBubbleView and in Site Details. This currently causes a bug
in the Harmony PageInfoBubbleView where the 'Allow' and 'Block' permissions are
flipped. Fix by adding the default option back in.

Bug:  789587 
Change-Id: Icc99609ee479c7c503d7abb3f027ee8ebe07e52b
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Reviewed-on: https://chromium-review.googlesource.com/813576
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Commit-Queue: Patti <patricialor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525611}
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/browser/resources/settings/site_settings/constants.js
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/browser/resources/settings/site_settings/site_details_permission.html
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/browser/resources/settings/site_settings/site_details_permission.js
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/browser/ui/page_info/page_info_ui.cc
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/browser/ui/page_info/permission_menu_model.cc
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/browser/ui/page_info/permission_menu_model_unittest.cc
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/browser/ui/webui/site_settings_helper.cc
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/browser/ui/webui/site_settings_helper.h
[modify] https://crrev.com/f2a74af762e90c326f4901a80e54652d6de5f4f8/chrome/test/data/webui/settings/site_details_permission_tests.js

This should be fixed (with the patch in #c17).