Page Info: Hide default permissions with a value of Ask if the default is Ask.

Page Info: Hide default permissions with a value of Ask if the default is Ask.

The feature comes with a temporary Finch killswitch called PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour of showing all permissions.

[1] https://cs.chromium.org/chromium/src/components/content_settings/core/common/content_settings.h?type=cs&q=CONTENT_SETTING_DEFAULT&sq=package:chromium&l=25

BUG=657267, 698467
TEST=Perform the following steps on Mac and Windows:
1) Visit https://permission.site and click the lock icon in the URL bar to open the Page Info bubble. Verify that there are only 5 permissions shown below "Cookies", and that these don't include "Notifications" or "Location". Verify that the button at the bottom says "More settings".
2) On the page, click on "Location" (select "Allow" in the permission prompt) and "Notifications" (select "Block" in the permission prompt).
3) Open the Page Info bubble again and verify that Location and Notifications are now visible in addition to the permissions from step 1, with respective values of "Allow" and "Block".
4) Change the Location dropdown in Page Info from "Allow" to "Ask". Close and re-open Page Info and verify that "Location" is not listed anymore.
5) Relaunch Chrome with --enable-features="PageInfoAlwaysShowAllPermissions", open Page Info, and verify that all permissions are shown (currently 11 on desktop).

[lgarron, 2017-02-21 23:49:48]

Description was changed from

==========
Page Info: hide permissions with default vaules behind a button.

BUG=657267
==========

to

==========
Page Info: hide permissions with default values behind a button.

BUG=657267
==========

[lgarron, 2017-02-24 00:54:45]

Description was changed from

==========
Page Info: hide permissions with default values behind a button.

BUG=657267
==========

to

==========
Page Info: hide permissions with default values behind a button.

The feature comes with a Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour. Once we
land the platform-specific implementations to expand the list of permissions, it
can be disabled by default.

BUG=657267
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

[lgarron, 2017-02-24 00:56:47]

Description was changed from

==========
Page Info: hide permissions with default values behind a button.

The feature comes with a Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour. Once we
land the platform-specific implementations to expand the list of permissions, it
can be disabled by default.

BUG=657267
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

to

==========
Page Info: Add cross-platform logic to hide permissions with default values
behind a button.

The default value is taken to be CONTENT_SETTING_DEFAULT, which means that 

The feature comes with a Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour. Once we
land the platform-specific implementations to expand the list of permissions, it
can be disabled by default.

BUG=657267
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

[lgarron, 2017-02-24 01:04:05]

Description was changed from

==========
Page Info: Add cross-platform logic to hide permissions with default values
behind a button.

The default value is taken to be CONTENT_SETTING_DEFAULT, which means that 

The feature comes with a Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour. Once we
land the platform-specific implementations to expand the list of permissions, it
can be disabled by default.

BUG=657267
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

to

==========
Page Info: Add cross-platform logic to hide permissions with default values
behind a button.

The default value is taken to be CONTENT_SETTING_DEFAULT [1], which means that
the site does not inherit a built-in browser permission.

In follow-up CLs, the "Site settings" button will be modified for each platform
to expand the list of permissions in-line to all permissions.
This CL adds stub implementations of UpdatePermissionButton() that are called
with the proper behaviour.

The feature comes with a Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour of
showing old permissions. Once we land the platform-specific implementations to
expand the list of permissions, it can be disabled by default.

[1]
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

BUG=657267
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

[lgarron, 2017-02-24 01:07:25]

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

[lgarron, 2017-02-24 01:07:27]

raymes@, could you review the cross-platform code?
palmer@, could you review the cocoa/views stubs? (Feel free to take a look at
the rest, too.)

[lgarron, 2017-02-24 01:21:23]

lgarron@chromium.org changed reviewers:
+ isherman@chromium.org

[lgarron, 2017-02-24 01:21:23]

isherman@, could you review histograms.xml?

[palmer, 2017-02-24 01:55:02]

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings_ui.cc:288:
VisiblePermissions visible_permissions) {
Nit: I'd expect |VisiblePermissions| to be some kind of collection of
(hypothetical) |Permissions| objects. Perhaps a better (?) name for the type
would be |PermissionsVisibility|.

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings_ui.cc:292: case
VISIBLE_PERMISSIONS_SOME_BUT_NOT_ALL:
Nit: "Some" is vague. Maybe |PERMISSIONS_VISIBILITY_ONLY_NON_DEFAULT|?

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings_ui.h:46: // A value that
represents a what subset of permissions are shown. Thisis used
Typo: "This is"

[lgarron, 2017-02-24 02:15:27]

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings_ui.cc:288:
VisiblePermissions visible_permissions) {
On 2017/02/24 at 01:55:02, palmer wrote:
> Nit: I'd expect |VisiblePermissions| to be some kind of collection of
(hypothetical) |Permissions| objects. Perhaps a better (?) name for the type
would be |PermissionsVisibility|.

I was on the fence, but I need something that conveys description ("this is a
value that conveys that there are N permissions shown") vs. prescription ("this
is a setting that controls how many permissions are show").
PermissionsVisibility sounds like the latter.

Is AmountOfVisiblePermissions too long?

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings_ui.cc:292: case
VISIBLE_PERMISSIONS_SOME_BUT_NOT_ALL:
On 2017/02/24 at 01:55:02, palmer wrote:
> Nit: "Some" is vague. Maybe |PERMISSIONS_VISIBILITY_ONLY_NON_DEFAULT|?

The problem with that is that there can be 0 non-default permissions (in fact,
probably by far the most common case).
Since VisiblePermissions is used to adjust the UI based on how many permissions
are actually shown, we need a good way to express that the number of permissions
shown is, well... "at least 1, but not all".
I'm open to other names that make this clear, though.

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/2702923002/diff/110001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings_ui.h:46: // A value that
represents a what subset of permissions are shown. Thisis used
On 2017/02/24 at 01:55:02, palmer wrote:
> Typo: "This is"

Fixed.

[lgarron, 2017-02-24 02:41:55]

CCing dependent CL reviewers

Views: https://codereview.chromium.org/2717613003 (msw@)
Cocoa: https://codereview.chromium.org/2712863005 (rsesek@)

[lgarron, 2017-02-24 03:44:27]

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

[commit-bot: I haz the power, 2017-02-24 03:44:40]

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

[commit-bot: I haz the power, 2017-02-24 04:04:19]

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

[commit-bot: I haz the power, 2017-02-24 04:04:20]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Ilya Sherman, 2017-02-24 21:55:36]

histograms.xml lgtm

[Robert Sesek, 2017-02-24 22:18:14]

rsesek@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2017-02-24 22:18:14]

cocoa/ LGTM

[raymes, 2017-02-27 01:40:49]

Hey Lucas, I think there's a meeting about this tomorrow? It would be good to
hold off on this until after we meet but happy to review afterwards. Thanks!

[lgarron, 2017-03-04 02:56:51]

Description was changed from

==========
Page Info: Add cross-platform logic to hide permissions with default values
behind a button.

The default value is taken to be CONTENT_SETTING_DEFAULT [1], which means that
the site does not inherit a built-in browser permission.

In follow-up CLs, the "Site settings" button will be modified for each platform
to expand the list of permissions in-line to all permissions.
This CL adds stub implementations of UpdatePermissionButton() that are called
with the proper behaviour.

The feature comes with a Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour of
showing old permissions. Once we land the platform-specific implementations to
expand the list of permissions, it can be disabled by default.

[1]
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

BUG=657267
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

to

==========
Page Info: Hide default permissions with a value of Ask if the default is Ask.

The feature comes with a temporary Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour of
showing all permissions.

[1]
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

BUG=657267
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

[lgarron, 2017-03-04 03:22:22]

Description was changed from

==========
Page Info: Hide default permissions with a value of Ask if the default is Ask.

The feature comes with a temporary Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour of
showing all permissions.

[1]
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

BUG=657267
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

to

==========
Page Info: Hide default permissions with a value of Ask if the default is Ask.

The feature comes with a temporary Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour of
showing all permissions.

[1]
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

BUG=657267, 698467
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

[lgarron, 2017-03-04 03:34:31]

Okes-dokes, we have greatly simplified.

raymes@, could you review again?

[lgarron, 2017-03-04 03:41:07]

Description was changed from

==========
Page Info: Hide default permissions with a value of Ask if the default is Ask.

The feature comes with a temporary Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour of
showing all permissions.

[1]
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

BUG=657267, 698467
TEST=Perform the following steps:
1) Launch Chrome with --disable-features="PageInfoAlwaysShowAllPermissions"
2) Visit https://permission.site and click the lock icon in the URL bar. Verify
that there are no permissions shown below "Cookies".
3) Click on "Notifications" and "Location" in the page, and "Block" the
permission prompt for each.
4) Visit https://permission.site and click the lock icon in the URL bar. Verify
that the "Location" and "Notifications" permissions are listed below "Cookies",
and no other permissions are.
==========

to

==========
Page Info: Hide default permissions with a value of Ask if the default is Ask.

The feature comes with a temporary Finch killswitch called
PageInfoAlwaysShowAllPermissions.
This commit sets the default to Enabled, which keeps the old behaviour of
showing all permissions.

[1]
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

BUG=657267, 698467
TEST=Perform the following steps on Mac and Windows:
1) Visit https://permission.site and click the lock icon in the URL bar to open
the Page Info bubble. Verify that there are only 5 permissions shown below
"Cookies", and that these don't include "Notifications" or "Location". Verify
that the button at the bottom says "More settings".
2) On the page, click on "Location" (select "Allow" in the permission prompt)
and "Notifications" (select "Block" in the permission prompt).
3) Open the Page Info bubble again and verify that Location and Notifications
are now visible in addition to the permissions from step 1, with respective
values of "Allow" and "Block".
4) Change the Location dropdown in Page Info from "Allow" to "Ask". Close and
re-open Page Info and verify that "Location" is not listed anymore.
5) Relaunch Chrome with --enable-features="PageInfoAlwaysShowAllPermissions",
open Page Info, and verify that all permissions are shown (currently 11 on
desktop).
==========

[raymes, 2017-03-05 23:47:11]

Thanks Lucas!

https://codereview.chromium.org/2702923002/diff/170001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2702923002/diff/170001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings_ui.cc:295: return
l10n_util::GetStringUTF16(IDS_PAGE_INFO_SITE_SETTINGS_LINK);
Strange that the behavior changes depending on whether there are no permissions
shown vs some?

https://codereview.chromium.org/2702923002/diff/250001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2702923002/diff/250001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8664: +        More settings
We should check in with UI review for guidance on the string. Were you imagining
it would go back to "Site settings" after the site settings UI is in place? Is
it really worth changing it before then?

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:213: // - Its current
value is "Ask" (or the the value representing the default).
nit: I don't think the parentheses here make this clearer.

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:216: if
(base::FeatureList::IsEnabled(kPageInfoAlwaysShowAllPermissions)) {
nit: other single-line if-statements in this file don't seem to use {} so I
would suggest removing for consistency

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:220: if
(permission_info.default_setting != CONTENT_SETTING_ASK) {
Should this be comparing to the user-specified default or the chrome-specified
default? Currently it's the user specified default. I don't think there will be
much difference in practice, but it's probably better to decide now.

We can easily access the Chrome-specified default through WebsiteSettingsInfo if
needed.

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:230: }
Do you think we really want to hide settings that are set to ASK, even if it's
not due to ASK being the default?

I think in practice it won't matter, but I would suggest the code would be
clearer here as:

if (permission_info.setting != CONTENT_SETTING_DEFAULT)
  return true;

return false;

[elawrence, 2017-03-06 19:28:02]

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:213: // - Its current
value is "Ask" (or the the value representing the default).
On 2017/03/05 23:47:11, raymes wrote:
> nit: I don't think the parentheses here make this clearer.

Also a repeated word: "the the"

[lgarron, 2017-03-09 02:41:24]

https://codereview.chromium.org/2702923002/diff/250001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2702923002/diff/250001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8664: +        More settings
On 2017/03/05 at 23:47:11, raymes wrote:
> We should check in with UI review for guidance on the string. Were you
imagining it would go back to "Site settings" after the site settings UI is in
place? Is it really worth changing it before then?

I'l double-check with Emily. But I consider it *very* important to change the
string to give a cue that we're not showing all the settings anymore.

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:213: // - Its current
value is "Ask" (or the the value representing the default).
On 2017/03/06 at 19:28:02, elawrence wrote:
> On 2017/03/05 23:47:11, raymes wrote:
> > nit: I don't think the parentheses here make this clearer.
> 
> Also a repeated word: "the the"

Updated.

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:216: if
(base::FeatureList::IsEnabled(kPageInfoAlwaysShowAllPermissions)) {
On 2017/03/05 at 23:47:11, raymes wrote:
> nit: other single-line if-statements in this file don't seem to use {} so I
would suggest removing for consistency

Fine, but: https://bugs.chromium.org/p/chromium/issues/detail?id=699840
(I wish `git cl format` would finally just pick *something*, even removing the
braces: https://bugs.chromium.org/p/chromium/issues/detail?id=651609)

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:220: if
(permission_info.default_setting != CONTENT_SETTING_ASK) {
On 2017/03/05 at 23:47:11, raymes wrote:
> Should this be comparing to the user-specified default or the chrome-specified
default? Currently it's the user specified default. I don't think there will be
much difference in practice, but it's probably better to decide now.
> 
> We can easily access the Chrome-specified default through WebsiteSettingsInfo
if needed.

Chrome-specified.

I thought that was what was happening here; will have to check when I can
compile on corp again.

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:230: }
On 2017/03/05 at 23:47:11, raymes wrote:
> Do you think we really want to hide settings that are set to ASK, even if it's
not due to ASK being the default?
> 
> I think in practice it won't matter, but I would suggest the code would be
clearer here as:
> 
> if (permission_info.setting != CONTENT_SETTING_DEFAULT)
>   return true;
> 
> return false;

Because of line 220, we always show permissions whose default is not ask.

As far as I know, the CONTENT_SETTING_ASK cannot be reached in practice unless
someone hacks their settings or we perform a settings migration and explicitly
set that value. But semantically, this is what how I meant [1].

Are you saying that in theory we should show the permission for a.example.com in
the following case?
- Global default: ASK
- [*.]example.com: BLOCK
- a.example.com: ASK
(Ignoring that that this UI is totally borked anyhow [2].)

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=657267#c35
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=614618

[raymes, 2017-03-09 03:25:19]

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:216: if
(base::FeatureList::IsEnabled(kPageInfoAlwaysShowAllPermissions)) {
On 2017/03/09 02:41:24, lgarron wrote:
> On 2017/03/05 at 23:47:11, raymes wrote:
> > nit: other single-line if-statements in this file don't seem to use {} so I
> would suggest removing for consistency
> 
> Fine, but: https://bugs.chromium.org/p/chromium/issues/detail?id=699840
> (I wish `git cl format` would finally just pick *something*, even removing the
> braces: https://bugs.chromium.org/p/chromium/issues/detail?id=651609)

I agree, I don't feel strongly which way, it's just consistency that I desire!

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:220: if
(permission_info.default_setting != CONTENT_SETTING_ASK) {
On 2017/03/09 02:41:24, lgarron wrote:
> On 2017/03/05 at 23:47:11, raymes wrote:
> > Should this be comparing to the user-specified default or the
chrome-specified
> default? Currently it's the user specified default. I don't think there will
be
> much difference in practice, but it's probably better to decide now.
> > 
> > We can easily access the Chrome-specified default through
WebsiteSettingsInfo
> if needed.
> 
> Chrome-specified.
> 
> I thought that was what was happening here; will have to check when I can
> compile on corp again.

To be clear by Chrome-specified default I mean the one that chrome ships with

Currently it's just grabbing the value from the HostContentSettingsMap that has
the *,* pattern - which is the user-specified default. We can get the
chrome-specified default by accessing
ContentSettingsRegistry::GetInstance()->Get(type)->InitialDefaultValue();

Note: that function doesn't actually exist yet, but implementing it would be
trivial:

ContentSetting ContentSettingsInfo::InitialDefaultValue() {
  return ValueToContentSetting(website_settings_info->initial_default_value());
}

https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/website_settings.cc:230: }
On 2017/03/09 02:41:24, lgarron wrote:
> On 2017/03/05 at 23:47:11, raymes wrote:
> > Do you think we really want to hide settings that are set to ASK, even if
it's
> not due to ASK being the default?
> > 
> > I think in practice it won't matter, but I would suggest the code would be
> clearer here as:
> > 
> > if (permission_info.setting != CONTENT_SETTING_DEFAULT)
> >   return true;
> > 
> > return false;
> 
> Because of line 220, we always show permissions whose default is not ask.
> 
> As far as I know, the CONTENT_SETTING_ASK cannot be reached in practice unless
> someone hacks their settings or we perform a settings migration and explicitly
> set that value. But semantically, this is what how I meant [1].
> 
> Are you saying that in theory we should show the permission for
http://a.example.com in
> the following case?
> - Global default: ASK
> - [*.]example.com: BLOCK
> - http://a.example.com: ASK
> (Ignoring that that this UI is totally borked anyhow [2].)
> 
> [1] https://bugs.chromium.org/p/chromium/issues/detail?id=657267#c35
> [2] https://bugs.chromium.org/p/chromium/issues/detail?id=614618

Yeah I guess I'm thinking in theory if the value was meaningfully ASK from some
situation other than the Chrome-default, then we should show the setting. For
example, if the default was BLOCK but enterprise policy set it to ASK then I
think we should show it. I actually think that it's possible that could happen
today.

Thinking about it in more detail, if the source of the setting is something
other than the chrome-specified default I think we should probably show it. We
could incorporate permission_info.source to check that if we wanted, e.g.

if (permission_info.source == default provider AND 
    permission_info.setting == chrome default AND 
    permission_info.setting == ASK) {
  hide
} else {
  show
}

What would you think about that approach?

[lgarron, 2017-03-11 02:17:17]

On 2017/03/09 at 03:25:19, raymes wrote:
>
https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
> File chrome/browser/ui/website_settings/website_settings.cc (right):
> 
>
https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
> chrome/browser/ui/website_settings/website_settings.cc:216: if
(base::FeatureList::IsEnabled(kPageInfoAlwaysShowAllPermissions)) {
> On 2017/03/09 02:41:24, lgarron wrote:
> > On 2017/03/05 at 23:47:11, raymes wrote:
> > > nit: other single-line if-statements in this file don't seem to use {} so
I
> > would suggest removing for consistency
> > 
> > Fine, but: https://bugs.chromium.org/p/chromium/issues/detail?id=699840
> > (I wish `git cl format` would finally just pick *something*, even removing
the
> > braces: https://bugs.chromium.org/p/chromium/issues/detail?id=651609)
> 
> I agree, I don't feel strongly which way, it's just consistency that I desire!
> 
>
https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
> chrome/browser/ui/website_settings/website_settings.cc:220: if
(permission_info.default_setting != CONTENT_SETTING_ASK) {
> On 2017/03/09 02:41:24, lgarron wrote:
> > On 2017/03/05 at 23:47:11, raymes wrote:
> > > Should this be comparing to the user-specified default or the
chrome-specified
> > default? Currently it's the user specified default. I don't think there will
be
> > much difference in practice, but it's probably better to decide now.
> > > 
> > > We can easily access the Chrome-specified default through
WebsiteSettingsInfo
> > if needed.
> > 
> > Chrome-specified.
> > 
> > I thought that was what was happening here; will have to check when I can
> > compile on corp again.
> 
> To be clear by Chrome-specified default I mean the one that chrome ships with
> 
> Currently it's just grabbing the value from the HostContentSettingsMap that
has the *,* pattern - which is the user-specified default. We can get the
chrome-specified default by accessing
ContentSettingsRegistry::GetInstance()->Get(type)->InitialDefaultValue();
> 
> Note: that function doesn't actually exist yet, but implementing it would be
trivial:
> 
> ContentSetting ContentSettingsInfo::InitialDefaultValue() {
>   return
ValueToContentSetting(website_settings_info->initial_default_value());
> }
> 
>
https://codereview.chromium.org/2702923002/diff/250001/chrome/browser/ui/webs...
> chrome/browser/ui/website_settings/website_settings.cc:230: }
> On 2017/03/09 02:41:24, lgarron wrote:
> > On 2017/03/05 at 23:47:11, raymes wrote:
> > > Do you think we really want to hide settings that are set to ASK, even if
it's
> > not due to ASK being the default?
> > > 
> > > I think in practice it won't matter, but I would suggest the code would be
> > clearer here as:
> > > 
> > > if (permission_info.setting != CONTENT_SETTING_DEFAULT)
> > >   return true;
> > > 
> > > return false;
> > 
> > Because of line 220, we always show permissions whose default is not ask.
> > 
> > As far as I know, the CONTENT_SETTING_ASK cannot be reached in practice
unless
> > someone hacks their settings or we perform a settings migration and
explicitly
> > set that value. But semantically, this is what how I meant [1].
> > 
> > Are you saying that in theory we should show the permission for
http://a.example.com in
> > the following case?
> > - Global default: ASK
> > - [*.]example.com: BLOCK
> > - http://a.example.com: ASK
> > (Ignoring that that this UI is totally borked anyhow [2].)
> > 
> > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=657267#c35
> > [2] https://bugs.chromium.org/p/chromium/issues/detail?id=614618
> 
> Yeah I guess I'm thinking in theory if the value was meaningfully ASK from
some situation other than the Chrome-default, then we should show the setting.
For example, if the default was BLOCK but enterprise policy set it to ASK then I
think we should show it. I actually think that it's possible that could happen
today.
> 
> Thinking about it in more detail, if the source of the setting is something
other than the chrome-specified default I think we should probably show it. We
could incorporate permission_info.source to check that if we wanted, e.g.
> 
> if (permission_info.source == default provider AND 
>     permission_info.setting == chrome default AND 
>     permission_info.setting == ASK) {
>   hide
> } else {
>   show
> }
> 
> What would you think about that approach?

Shelving this until we can come to a consensus with Enamel + UI review. :-(