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

chrome/browser/ui/website_settings/website_settings.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/website_settings/website_settings.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <string>
 #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"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
@@ skipping 52 matching lines @@
 #include "chrome/browser/ui/website_settings/website_settings_infobar_delegate.h"
 #endif
 
 using base::ASCIIToUTF16;
 using base::UTF8ToUTF16;
 using base::UTF16ToUTF8;
 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,
   USER_CERT_DECISIONS_REVOKED,
   END_OF_SSL_CERTIFICATE_DECISIONS_DID_REVOKE_ENUM
 };
 
 // The list of content settings types to display on the Website Settings UI. THE
 // ORDER OF THESE ITEMS IS IMPORTANT. To propose changing it, email
 // security-dev@chromium.org.
@@ skipping 101 matching lines @@
 
 // The list of chooser types that need to display entries in the Website
 // Settings UI. THE ORDER OF THESE ITEMS IS IMPORTANT. To propose changing it,
 // email security-dev@chromium.org.
 WebsiteSettings::ChooserUIInfo kChooserUIInfo[] = {
     {CONTENT_SETTINGS_TYPE_USB_CHOOSER_DATA, &GetUsbChooserContext,
      IDR_BLOCKED_USB, IDR_ALLOWED_USB, IDS_WEBSITE_SETTINGS_USB_DEVICE_LABEL,
      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(
     WebsiteSettingsUI* ui,
     Profile* profile,
     TabSpecificContentSettings* tab_specific_content_settings,
     content::WebContents* web_contents,
     const GURL& url,
     const security_state::SecurityInfo& security_info)
     : TabSpecificContentSettings::SiteDataObserver(
@@ skipping 450 matching lines @@
     if (info.primary_pattern == ContentSettingsPattern::Wildcard() &&
         info.secondary_pattern == ContentSettingsPattern::Wildcard()) {
       permission_info.default_setting = permission_info.setting;
       permission_info.setting = CONTENT_SETTING_DEFAULT;
     } else {
       permission_info.default_setting =
           content_settings_->GetDefaultContentSetting(permission_info.type,
                                                       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) {
     ChooserContextBase* context = ui_info.get_context(profile_);
     const GURL origin = site_url_.GetOrigin();
     auto chosen_objects = context->GetGrantedObjects(origin, origin);
     for (std::unique_ptr<base::DictionaryValue>& object : chosen_objects) {
       chosen_object_info_list.push_back(
           base::MakeUnique<WebsiteSettingsUI::ChosenObjectInfo>(
               ui_info, std::move(object)));
@@ skipping 41 matching lines @@
   info.connection_status = site_connection_status_;
   info.connection_status_description =
       UTF16ToUTF8(site_connection_details_);
   info.identity_status = site_identity_status_;
   info.identity_status_description =
       UTF16ToUTF8(site_identity_details_);
   info.certificate = certificate_;
   info.show_ssl_decision_revoke_button = show_ssl_decision_revoke_button_;
   ui_->SetIdentityInfo(info);
 }