Views/Permissions: Use shorter strings for site setting drop downs.

chrome/browser/ui/website_settings/permission_menu_model.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/permission_menu_model.h"
 
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/plugins/plugin_utils.h"
 #include "chrome/browser/plugins/plugins_field_trial.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/common/origin_util.h"
 #include "ppapi/features/features.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/material_design/material_design_controller.h"
 
 PermissionMenuModel::PermissionMenuModel(
     Profile* profile,
     const GURL& url,
     const WebsiteSettingsUI::PermissionInfo& info,
     const ChangeCallback& callback)
     : ui::SimpleMenuModel(this),
       host_content_settings_map_(
           HostContentSettingsMapFactory::GetForProfile(profile)),
       permission_(info),
       callback_(callback) {
   DCHECK(!callback_.is_null());
   base::string16 label;
 
+  // Retrieve the string to show for the default setting for this permission.
   ContentSetting effective_default_setting = permission_.default_setting;
 
 #if BUILDFLAG(ENABLE_PLUGINS)
   effective_default_setting = PluginsFieldTrial::EffectiveContentSetting(
       host_content_settings_map_, permission_.type,
       permission_.default_setting);
 #endif  // BUILDFLAG(ENABLE_PLUGINS)
 
   switch (effective_default_setting) {
     case CONTENT_SETTING_ALLOW:
@@ skipping 21 matching lines @@
       NOTREACHED();
     default:
       break;
   }
 
   // The Material UI for site settings uses comboboxes instead of menubuttons,
   // which means the elements of the menu themselves have to be shorter, instead
   // of simply setting a shorter label on the menubutton.
   if (ui::MaterialDesignController::IsSecondaryUiMaterial()) {
     label = WebsiteSettingsUI::PermissionActionToUIString(
-        profile, info.type, info.setting, effective_default_setting,
+        profile, info.type, CONTENT_SETTING_DEFAULT, effective_default_setting,
         info.source);
   }
 
   AddCheckItem(CONTENT_SETTING_DEFAULT, label);
 
+  // Retrieve the string to show for allowing the permission.
   // Notifications does not support CONTENT_SETTING_ALLOW in incognito.
   bool allow_disabled_for_notifications =
       permission_.is_incognito &&
       permission_.type == CONTENT_SETTINGS_TYPE_NOTIFICATIONS;
   // Media only supports CONTENT_SETTTING_ALLOW for secure origins.
   bool is_media_permission =
       permission_.type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC ||
       permission_.type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA;
   if (!allow_disabled_for_notifications &&
       (!is_media_permission || content::IsOriginSecure(url))) {
     label = l10n_util::GetStringUTF16(
         IDS_WEBSITE_SETTINGS_MENU_ITEM_ALLOW);
+    if (ui::MaterialDesignController::IsSecondaryUiMaterial()) {
+      label = WebsiteSettingsUI::PermissionActionToUIString(
+          profile, info.type, CONTENT_SETTING_ALLOW, effective_default_setting,
+          info.source);
+    }
     AddCheckItem(CONTENT_SETTING_ALLOW, label);
   }
 
   // TODO(tommycli): With the HTML5 by Default feature, Flash is treated the
   // same as any other permission with ASK, i.e. there is no ASK exception.
   // Once the feature flag is gone, remove this block of code entirely.
   if (permission_.type == CONTENT_SETTINGS_TYPE_PLUGINS &&
       !PluginUtils::ShouldPreferHtmlOverPlugins(host_content_settings_map_)) {
     label = l10n_util::GetStringUTF16(
         IDS_WEBSITE_SETTINGS_MENU_ITEM_DETECT_IMPORTANT_CONTENT);
     AddCheckItem(CONTENT_SETTING_DETECT_IMPORTANT_CONTENT, label);
   }
 
+  // Retrieve the string to show for blocking the permission.

[palmer, 2017/03/07 23:39:46]

Nit: These comments suggest that moving this code into well-named helper
functions might clarify the intent of the code. E.g.

  label = GetBlockingPermissionLabel(profile, info.type, CONTENT_SETTING_BLOCK,
                                     effective_default_setting, info.source);

or such.

I could be wrong; your call. :)

[Patti Lor, 2017/03/08 06:58:41]

Yeah, I did consider this - it's pretty simple for the "blocked" permission
type, but looking at "allowed" and "default" it's a lot more unclear - "allowed"
permissions aren't added to the menu at all based on certain conditions, and
default permissions are a lot more complicated since you also need access to the
HostContentSettingsMapFactory etc, resulting in them all having pretty different
function signatures (and adding them as methods is probably also bad)... so I
decided just to leave it after adding the comments.

Thanks for the suggestion though - just giving some context into why I decided
not to do that in case you were interested -- let me know if you still think
it's worth doing.

   label = l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_MENU_ITEM_BLOCK);
+  if (ui::MaterialDesignController::IsSecondaryUiMaterial()) {
+    label = WebsiteSettingsUI::PermissionActionToUIString(
+        profile, info.type, CONTENT_SETTING_BLOCK, effective_default_setting,
+        info.source);
+  }
   AddCheckItem(CONTENT_SETTING_BLOCK, label);
 }
 
 PermissionMenuModel::PermissionMenuModel(Profile* profile,
                                          const GURL& url,
                                          ContentSetting setting,
                                          const ChangeCallback& callback)
     : ui::SimpleMenuModel(this),
       host_content_settings_map_(
           HostContentSettingsMapFactory::GetForProfile(profile)),
@@ skipping 22 matching lines @@
 }
 
 bool PermissionMenuModel::IsCommandIdEnabled(int command_id) const {
   return true;
 }
 
 void PermissionMenuModel::ExecuteCommand(int command_id, int event_flags) {
   permission_.setting = static_cast<ContentSetting>(command_id);
   callback_.Run(permission_);
 }