Stop pretending that a Flash setting of DETECT_IMPORTANT_CONTENT is ASK.

Stop pretending that a Flash setting of DETECT_IMPORTANT_CONTENT is ASK.

This restores the UI behaviour of showing "Detect" instead of "Ask" as the default setting for Flash.

BUG=697238
--------------------------------
TEST=Following steps:
1) Visit https://permission.site
2) Click on the security indicator (lock icon) in the omnibox.
3) Check that the Flash setting is "Detect (by default)" instead of "Ask (by default)". [1]

[1] Note: For Googlers, it might say "by policy" instead of "by default". That's okay, as long as it's "Detect (by policy)" instead of "Ask by policy".

[lgarron, 2017-03-04 00:11:28]

Description was changed from

==========
Display CONTENT_SETTING_DETECT_IMPORTANT_CONTENT as "Detect" instead of "Ask"
again.

BUG=697238
==========

to

==========
Stop pretending that a Flash setting of DETECT_IMPORTANT_CONTENT is ASK.

BUG=697238
==========

[lgarron, 2017-03-04 00:15:38]

Description was changed from

==========
Stop pretending that a Flash setting of DETECT_IMPORTANT_CONTENT is ASK.

BUG=697238
==========

to

==========
Stop pretending that a Flash setting of DETECT_IMPORTANT_CONTENT is ASK.

This restores the UI behaviour of showing "Detect" instead of "Ask" as the
default setting for Flash.

BUG=697238

--------------------------------
TEST=Following steps:
1) Visit https://permission.site
2) Click on the security indicator (lock icon) in the omnibox.
3) Check that the Flash setting is "Detect (by default)" instead of "Ask (by
default)". [1]

[1] Note: For Googlers, it might say "by policy" instead of "by default". That's
okay, as long as it's "Detect (by policy)" instead of "Ask by policy".
==========

[lgarron, 2017-03-04 00:15:44]

The CQ bit was checked by lgarron@chromium.org to run a CQ dry run

[lgarron, 2017-03-04 00:16:01]

lgarron@chromium.org changed reviewers:
+ raymes@chromium.org, tommycli@chromium.org

[lgarron, 2017-03-04 00:16:02]

raymes@, tommycli@, could you review?

[commit-bot: I haz the power, 2017-03-04 00:16:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2017-03-04 00:20:16]

On 2017/03/04 00:16:02, lgarron wrote:
> raymes@, tommycli@, could you review?

To confirm I understand everything completely: All the Ask (default) permissions
in the Omnibox dropdown UI is going to be hidden by default. We don't want to
hide Flash. Therefore we're making Flash say "Detect" there. At some point in
the future we're going to make it Ask again.

Is that an accurate representation? If so, lgtm

Tommy

[commit-bot: I haz the power, 2017-03-04 00:52:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 00:52:04]

Dry run: This issue passed the CQ dry run.

[lgarron, 2017-03-04 00:58:58]

On 2017/03/04 at 00:20:16, tommycli wrote:
> On 2017/03/04 00:16:02, lgarron wrote:
> > raymes@, tommycli@, could you review?
> 
> To confirm I understand everything completely: All the Ask (default)
permissions in the Omnibox dropdown UI is going to be hidden by default. We
don't want to hide Flash. Therefore we're making Flash say "Detect" there. At
some point in the future we're going to make it Ask again.
> 
> Is that an accurate representation? If so, lgtm
> 
> Tommy

Yes.

If we're going to change Flash back to "Ask", we should actually change the
default content setting for Flash, though. (The current approach passes
inconsistent information to the UI.)

[tommycli, 2017-03-04 01:00:28]

On 2017/03/04 00:58:58, lgarron wrote:
> On 2017/03/04 at 00:20:16, tommycli wrote:
> > On 2017/03/04 00:16:02, lgarron wrote:
> > > raymes@, tommycli@, could you review?
> > 
> > To confirm I understand everything completely: All the Ask (default)
> permissions in the Omnibox dropdown UI is going to be hidden by default. We
> don't want to hide Flash. Therefore we're making Flash say "Detect" there. At
> some point in the future we're going to make it Ask again.
> > 
> > Is that an accurate representation? If so, lgtm
> > 
> > Tommy
> 
> Yes.
> 
> If we're going to change Flash back to "Ask", we should actually change the
> default content setting for Flash, though. (The current approach passes
> inconsistent information to the UI.)

Yes, agreed, that would be ideal. We will have to change all the places in the
code that do:

if (setting == ASK)
  setting = DETECT;

to the inverse.

[raymes, 2017-03-05 23:55:40]

3 thoughts:
1)I think that we should separate the behavioral changes of what content setting
is actually used internally from the string changes. We were in an intermediate
state where we had both DETECT and ASK doing 2 different things until HBD
shipped. We should move away from that and just use 1 of those settings, but we
should do it orthogonally from this strings change. I think it would be best to
get rid of DETECT and use ASK.

2) Do we also want to change the wording in about:settings/content? Currently it
still talks about "ask". I would suggest just leaving it the same there for now,
but open to ideas.

3) We should talk to UX about these string changes and make sure they're
comfortable.

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_permission_context.cc (left):

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/flash_permission_context.cc:50: return
CONTENT_SETTING_ASK;
I'm not sure if we want to do this? Won't this have potential behavioral
implications? We just want to change the UI.

[raymes, 2017-03-05 23:56:51]

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_menu_model.cc (right):

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_menu_model.cc:54: // Once the
feature flag is gone, migrate the actual setting to ASK.
I think this TODO is inaccurate now?

[raymes, 2017-03-06 00:00:00]

On 2017/03/05 23:55:40, raymes wrote:
> 3 thoughts:
> 1)I think that we should separate the behavioral changes of what content
setting
> is actually used internally from the string changes. We were in an
intermediate
> state where we had both DETECT and ASK doing 2 different things until HBD
> shipped. We should move away from that and just use 1 of those settings, but
we
> should do it orthogonally from this strings change. I think it would be best
to
> get rid of DETECT and use ASK.

I defer to Tommy on this point though, because he knows the code better than me
and perhaps this CL is already doing that. The change in
flash_permission_context.cc didn't seem to be needed though?

[tommycli, 2017-03-06 19:10:26]

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_permission_context.cc (left):

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/flash_permission_context.cc:50: return
CONTENT_SETTING_ASK;
On 2017/03/05 23:55:40, raymes wrote:
> I'm not sure if we want to do this? Won't this have potential behavioral
> implications? We just want to change the UI.

I agree. I missed that this was in FlashPermissionContext. I agree with Raymes
that this seems like it will have negative side effects.

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_menu_model.cc (right):

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_menu_model.cc:54: // Once the
feature flag is gone, migrate the actual setting to ASK.
On 2017/03/05 23:56:50, raymes wrote:
> I think this TODO is inaccurate now?

Yes that's right. it's inaccurate. Though maybe it should be replaced with a
TODO that says it should always be ASK once HBD's SEI threshold is turned all
the way up.

[lgarron, 2017-03-09 02:47:19]

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_permission_context.cc (left):

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/flash_permission_context.cc:50: return
CONTENT_SETTING_ASK;
On 2017/03/06 at 19:10:26, tommycli wrote:
> On 2017/03/05 23:55:40, raymes wrote:
> > I'm not sure if we want to do this? Won't this have potential behavioral
> > implications? We just want to change the UI.
> 
> I agree. I missed that this was in FlashPermissionContext. I agree with Raymes
that this seems like it will have negative side effects.

Could you explain to me what those effects are?
If the general behaviour is not ASK yet, I don't quite understand why returning
DETECT here is not alright. (Can we fix the call sites to interpret DETECT
correctly?)

[raymes, 2017-03-09 03:01:07]

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_permission_context.cc (left):

https://codereview.chromium.org/2731083002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/flash_permission_context.cc:50: return
CONTENT_SETTING_ASK;
On 2017/03/09 02:47:19, lgarron wrote:
> On 2017/03/06 at 19:10:26, tommycli wrote:
> > On 2017/03/05 23:55:40, raymes wrote:
> > > I'm not sure if we want to do this? Won't this have potential behavioral
> > > implications? We just want to change the UI.
> > 
> > I agree. I missed that this was in FlashPermissionContext. I agree with
Raymes
> that this seems like it will have negative side effects.
> 
> Could you explain to me what those effects are?
> If the general behaviour is not ASK yet, I don't quite understand why
returning
> DETECT here is not alright. (Can we fix the call sites to interpret DETECT
> correctly?)

Ask means "show a prompt" in the permissions code, and that's what we want to
happen in this case. This actually happens to work because of the way the code
is written, but I don't think we should rely on that (which is why this test
doesn't fail). There is other code that assumes that ASK means show a prompt,
which is intuitive.

I think the right solution is to get rid of DETECT as an option which I hope
we'll be able to do soon.