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

chrome/browser/ui/website_settings/website_settings.cc

Index: chrome/browser/ui/website_settings/website_settings.cc
diff --git a/chrome/browser/ui/website_settings/website_settings.cc b/chrome/browser/ui/website_settings/website_settings.cc
index a8549dddb7e8f68511f29b23c857127dbecb6272..e7c3b9095c655a15742d612a232a6f559194785a 100644
--- a/chrome/browser/ui/website_settings/website_settings.cc
+++ b/chrome/browser/ui/website_settings/website_settings.cc
@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/i18n/time_formatting.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ -83,6 +84,10 @@ using content::BrowserThread;
 
 namespace {
 
+// TODO(crbug.com/698469): Remove kPageInfoAlwaysShowAllPermissions
+const base::Feature kPageInfoAlwaysShowAllPermissions{
+    "PageInfoAlwaysShowAllPermissions", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Events for UMA. Do not reorder or change!
 enum SSLCertificateDecisionsDidRevoke {
   USER_CERT_DECISIONS_NOT_REVOKED = 0,
@@ -204,6 +209,27 @@ WebsiteSettings::ChooserUIInfo kChooserUIInfo[] = {
      IDS_WEBSITE_SETTINGS_DELETE_USB_DEVICE, "name"},
 };
 
+// Show a permission unless the following conditions hold:
+// - Its current value is "Ask" (or the the value representing the default).

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

nit: I don't think the parentheses here make this clearer.

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

Also a repeated word: "the the"

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

Updated.

+// - Its built-in default value is "Ask".
+bool ShouldShowPermission(WebsiteSettingsUI::PermissionInfo permission_info) {
+  if (base::FeatureList::IsEnabled(kPageInfoAlwaysShowAllPermissions)) {

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

nit: other single-line if-statements in this file don't seem to use {} so I
would suggest removing for consistency

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

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)

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

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

+    return true;
+  }
+
+  if (permission_info.default_setting != CONTENT_SETTING_ASK) {

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

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.

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

Chrome-specified.

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

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

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());
}

+    return true;
+  }
+
+  switch (permission_info.setting) {
+    case CONTENT_SETTING_DEFAULT:
+    case CONTENT_SETTING_ASK:
+      return false;
+    default:
+      return true;
+  }

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

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;

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

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]

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?

+}
+
 }  // namespace
 
 WebsiteSettings::WebsiteSettings(
@@ -674,7 +700,9 @@ void WebsiteSettings::PresentSitePermissions() {
                                                       NULL);
     }
 
-    permission_info_list.push_back(permission_info);
+    if (ShouldShowPermission(permission_info)) {
+      permission_info_list.push_back(permission_info);
+    }
   }
 
   for (const ChooserUIInfo& ui_info : kChooserUIInfo) {