Always prompt for permission on fullscreen and mouse lock on file:// URLs

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/grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 PermissionMenuModel::PermissionMenuModel(
@@ skipping 23 matching lines @@
       label = l10n_util::GetStringUTF16(
           IDS_WEBSITE_SETTINGS_MENU_ITEM_DEFAULT_DETECT_IMPORTANT_CONTENT);
       break;
     case CONTENT_SETTING_NUM_SETTINGS:
       NOTREACHED();
     default:
       break;
   }
   AddCheckItem(CONTENT_SETTING_DEFAULT, label);
 
-  // Media only support CONTENTE_SETTTING_ALLOW for https.
-  if (permission_.type != CONTENT_SETTINGS_TYPE_MEDIASTREAM ||
-      url.SchemeIsSecure()) {
+  // CONTENT_SETTING_ALLOW and CONTENT_SETTING_BLOCK are not allowed for
+  // fullscreen or mouse lock on file:// URLs, because there wouldn't be
+  // a reasonable origin with which to associate the preference.
+  // TODO(estark): Revisit this when crbug.com/455882 is fixed.
+  bool is_exclusive_access_on_file =
+      (permission_.type == CONTENT_SETTINGS_TYPE_FULLSCREEN ||
+       permission_.type == CONTENT_SETTINGS_TYPE_MOUSELOCK) &&
+      url.SchemeIsFile();

[meacer, 2015/02/11 00:45:33]

Looks like you could do an early return from here?

[estark, 2015/02/11 21:21:58]

Agree logically, but I'm hesitant to introduce a return point in the middle of
the function, out of fear that someone will come along and add completely
unrelated code at the bottom of the function without realizing that their code
won't run in this special case. What do you think?

(I'm not usually as argumentative as I come across in this CL, I swear!)

[meacer, 2015/02/11 21:39:51]

I'd say that that would in fact be good, they should have added a test case for
it :) But it's okay if you want to leave it as it is as well.
But arguing is fun!

+
+  // Media only support CONTENT_SETTTING_ALLOW for https.
+  if ((permission_.type != CONTENT_SETTINGS_TYPE_MEDIASTREAM ||
+       url.SchemeIsSecure()) &&
+      !is_exclusive_access_on_file) {
     label = l10n_util::GetStringUTF16(
         IDS_WEBSITE_SETTINGS_MENU_ITEM_ALLOW);
     AddCheckItem(CONTENT_SETTING_ALLOW, label);
   }
 
   if (permission_.type == CONTENT_SETTINGS_TYPE_PLUGINS) {
     label = l10n_util::GetStringUTF16(
         IDS_WEBSITE_SETTINGS_MENU_ITEM_DETECT_IMPORTANT_CONTENT);
     AddCheckItem(CONTENT_SETTING_DETECT_IMPORTANT_CONTENT, label);
   }
 
-  if (permission_.type != CONTENT_SETTINGS_TYPE_FULLSCREEN) {
+  if (permission_.type != CONTENT_SETTINGS_TYPE_FULLSCREEN &&
+      !is_exclusive_access_on_file) {
     label = l10n_util::GetStringUTF16(
         IDS_WEBSITE_SETTINGS_MENU_ITEM_BLOCK);
     AddCheckItem(CONTENT_SETTING_BLOCK, label);
   }
 }
 
 PermissionMenuModel::PermissionMenuModel(const GURL& url,
                                          ContentSetting setting,
                                          const ChangeCallback& callback)
     : ui::SimpleMenuModel(this), callback_(callback) {
@@ skipping 27 matching lines @@
     int command_id,
     ui::Accelerator* accelerator) {
   // Accelerators are not supported.
   return false;
 }
 
 void PermissionMenuModel::ExecuteCommand(int command_id, int event_flags) {
   permission_.setting = static_cast<ContentSetting>(command_id);
   callback_.Run(permission_);
 }