[Extensions Click-to-Script] Adjust icon grayscaling behavior

Currently, extension icons in the toolbar are grayscaled when the
extension action (i.e., the page action or the browser action) is
disabled. However, this can be confusing, because it makes the extension
look disabled when it might be running on that page.

With runtime host permissions enabled, the extension icon should only be
grayscaled in the toolbar only if the action is currently disabled *and*
the extension cannot currently inject on the page. This addresses the
confusing "disabled" look, as well as preventing the strange case of
"clicking on the extension icon and it immediately becoming disabled."

Bug: 866170

Change-Id: I00c239c6b4a35f52398e433d0a6ed357298e08c6
Reviewed-on: https://chromium-review.googlesource.com/1145630
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577235}

chrome/browser/extensions/extension_action_runner.h

@@ -101,6 +101,9 @@ class ExtensionActionRunner : public content::WebContentsObserver,
                                         const base::Closure& callback) {
     return RequestScriptInjection(extension, run_location, callback);
   }
+  void ClearInjectionsForTesting(const Extension& extension) {
+    pending_scripts_.erase(extension.id());
+  }
 #endif  // defined(UNIT_TEST)
 
  private:

chrome/browser/ui/extensions/extension_action_view_controller.cc

@@ -27,6 +27,7 @@
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_features.h"
 #include "extensions/common/manifest_constants.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_operations.h"
@@ -248,6 +249,34 @@ void ExtensionActionViewController::OnExtensionHostDestroyed(
   OnPopupClosed();
 }
 
+ExtensionActionViewController::PageInteractionStatus
+ExtensionActionViewController::GetPageInteractionStatus(
+    content::WebContents* web_contents) {
+  // We give priority to kPending, because it's the one that's most important
+  // for users to see.
+  if (HasBeenBlocked(web_contents))
+    return PageInteractionStatus::kPending;
+
+  // We consider an extension active on a page if either the extension action
+  // is active (in which case it can be clicked) or if the extension has
+  // permission to acccess the page (in which case it can inject scripts and
+  // intercept webRequests).
+  // NOTE(devlin): We could theoretically adjust this to only be considered
+  // active if the extension *did* act on the page, rather than if it *could*.
+  // This is a bit more complex, and it's unclear if this is a better UX, since
+  // it would lead to much less determinism in terms of what extensions look
+  // like on a given host.
+  int tab_id = SessionTabHelper::IdForTab(web_contents).id();
+  if (extension_action_->GetIsVisible(tab_id) ||
+      extension_->permissions_data()->GetPageAccess(
+          web_contents->GetLastCommittedURL(), tab_id, /*error=*/nullptr) ==
+          extensions::PermissionsData::PageAccess::kAllowed) {
+    return PageInteractionStatus::kActive;
+  }
+
+  return PageInteractionStatus::kNone;
+}
+
 bool ExtensionActionViewController::ExtensionIsValid() const {
   return extension_registry_->enabled_extensions().Contains(extension_->id());
 }
@@ -386,21 +415,34 @@ ExtensionActionViewController::GetIconImageSource(
   }
   image_source->SetBadge(std::move(badge));
 
-  // If the extension doesn't want to run on the active web contents, we
-  // grayscale it to indicate that.
-  image_source->set_grayscale(!IsEnabled(web_contents));
-  // If the action *does* want to run on the active web contents and is also
+  bool grayscale = false;
+  bool was_blocked = false;
+  if (base::FeatureList::IsEnabled(
+          extensions::features::kRuntimeHostPermissions)) {
+    PageInteractionStatus interaction_status =
+        GetPageInteractionStatus(web_contents);
+    // With the runtime host permissions feature, we only grayscale the icon if
+    // it cannot interact with the page and the icon is disabled.
+    grayscale = interaction_status == PageInteractionStatus::kNone;
+    was_blocked = interaction_status == PageInteractionStatus::kPending;
+  } else {
+    // Without runtime host permissions enabled, grayscaling is purely used to
+    // indicate "clickability", and not any kind of access.
+    grayscale = !extension_action_->GetIsVisible(tab_id);
+    // was_blocked is always false without runtime host permissions.
+  }
+
+  image_source->set_grayscale(grayscale);
+  image_source->set_paint_blocked_actions_decoration(was_blocked);
+
+  // If the action has an active page action on the web contents and is also
   // overflowed, we add a decoration so that the user can see which overflowed
   // action wants to run (since they wouldn't be able to see the change from
   // grayscale to color).
   bool is_overflow =
       toolbar_actions_bar_ && toolbar_actions_bar_->in_overflow_mode();
-
-  bool has_blocked_actions = HasBeenBlocked(web_contents);
-  image_source->set_paint_blocked_actions_decoration(has_blocked_actions);
   image_source->set_paint_page_action_decoration(
-      !has_blocked_actions && is_overflow &&
-      PageActionWantsToRun(web_contents));
+      !was_blocked && is_overflow && PageActionWantsToRun(web_contents));
 
   return image_source;
 }

chrome/browser/ui/extensions/extension_action_view_controller.h

@@ -97,6 +97,21 @@ class ExtensionActionViewController
   // ExtensionHostObserver:
   void OnExtensionHostDestroyed(const extensions::ExtensionHost* host) override;
 
+  // The status of the extension's interaction for the page.
+  enum class PageInteractionStatus {
+    // The extension cannot run on the page and cannot be clicked on the page.
+    kNone,
+    // The extension tried to inject on the page, but is pending user approval.
+    kPending,
+    // The extension has permission to run on the page, or is clickable on the
+    // page and has no pending injections.
+    kActive,
+  };
+
+  // Returns the PageInteractionStatus for the current page.
+  PageInteractionStatus GetPageInteractionStatus(
+      content::WebContents* web_contents);
+
   // Checks if the associated |extension| is still valid by checking its
   // status in the registry. Since the OnExtensionUnloaded() notifications are
   // not in a deterministic order, it's possible that the view tries to refresh

chrome/browser/ui/extensions/extension_action_view_controller_unittest.cc

@@ -2,17 +2,29 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/ui/extensions/extension_action_view_controller.h"
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/run_loop.h"
+#include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/extensions/extension_action.h"
+#include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_action_runner.h"
-#include "chrome/browser/ui/extensions/extension_action_view_controller.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/scripting_permissions_modifier.h"
+#include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/extensions/icon_with_badge_image_source.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_bar.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/extension_features.h"
 #include "extensions/common/user_script.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -64,6 +76,11 @@ TEST_P(ToolbarActionsBarUnitTest, ExtensionActionWantsToRunAppearance) {
 }
 
 TEST_P(ToolbarActionsBarUnitTest, ExtensionActionBlockedActions) {
+  // Blocked actions are only present with the runtime host permissions feature.
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      extensions::features::kRuntimeHostPermissions);
+
   scoped_refptr<const extensions::Extension> browser_action_ext =
       CreateAndAddExtension(
           "browser action",
@@ -189,3 +206,118 @@ TEST_P(ToolbarActionsBarUnitTest, ExtensionActionContextMenu) {
   check_visibility_string(toolbar_actions_bar()->GetActions()[0],
                           IDS_EXTENSIONS_KEEP_BUTTON_IN_TOOLBAR);
 }
+
+// Tests the behavior for icon grayscaling with the runtime host permissions
+// feature enabled.
+TEST_P(ToolbarActionsBarUnitTest, GrayscaleIcon) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      extensions::features::kRuntimeHostPermissions);
+
+  scoped_refptr<const extensions::Extension> extension =
+      extensions::ExtensionBuilder("extension")
+          .SetAction(extensions::ExtensionBuilder::ActionType::BROWSER_ACTION)
+          .SetLocation(extensions::Manifest::INTERNAL)
+          .AddPermission("https://www.google.com/*")
+          .Build();
+  extensions::ExtensionService* service =
+      extensions::ExtensionSystem::Get(profile())->extension_service();
+  service->GrantPermissions(extension.get());
+  service->AddExtension(extension.get());
+
+  extensions::ScriptingPermissionsModifier permissions_modifier(profile(),
+                                                                extension);
+  permissions_modifier.SetWithholdHostPermissions(true);
+  ASSERT_EQ(1u, toolbar_actions_bar()->GetIconCount());
+  const GURL kUrl("https://www.google.com/");
+  AddTab(browser(), kUrl);
+
+  enum class ActionState {
+    kEnabled,
+    kDisabled,
+  };
+  enum class PageAccess {
+    kGranted,
+    kPending,
+    kNone,
+  };
+  enum class Opacity {
+    kGrayscale,
+    kFull,
+  };
+  enum class BlockedActions {
+    kPainted,
+    kNotPainted,
+  };
+
+  struct {
+    ActionState action_state;
+    PageAccess page_access;
+    Opacity expected_opacity;
+    BlockedActions expected_blocked_actions;
+  } test_cases[] = {
+      {ActionState::kEnabled, PageAccess::kNone, Opacity::kFull,
+       BlockedActions::kNotPainted},
+      {ActionState::kEnabled, PageAccess::kPending, Opacity::kFull,
+       BlockedActions::kPainted},
+      {ActionState::kEnabled, PageAccess::kGranted, Opacity::kFull,
+       BlockedActions::kNotPainted},
+
+      {ActionState::kDisabled, PageAccess::kNone, Opacity::kGrayscale,
+       BlockedActions::kNotPainted},
+      {ActionState::kDisabled, PageAccess::kPending, Opacity::kFull,
+       BlockedActions::kPainted},
+      {ActionState::kDisabled, PageAccess::kGranted, Opacity::kFull,
+       BlockedActions::kNotPainted},
+  };
+
+  auto* controller = static_cast<ExtensionActionViewController*>(
+      toolbar_actions_bar()->GetActions()[0]);
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ExtensionAction* extension_action =
+      extensions::ExtensionActionManager::Get(profile())->GetExtensionAction(
+          *extension);
+  extensions::ExtensionActionRunner* action_runner =
+      extensions::ExtensionActionRunner::GetForWebContents(web_contents);
+  int tab_id = SessionTabHelper::IdForTab(web_contents).id();
+  const gfx::Size kSize = toolbar_actions_bar()->GetViewSize();
+
+  for (size_t i = 0; i < base::size(test_cases); ++i) {
+    SCOPED_TRACE(
+        base::StringPrintf("Running test case %d", static_cast<int>(i)));
+    const auto& test_case = test_cases[i];
+
+    // Set up the proper state.
+    extension_action->SetIsVisible(
+        tab_id, test_case.action_state == ActionState::kEnabled);
+    switch (test_case.page_access) {
+      case PageAccess::kNone:
+        // Page access should already be "none", but verify.
+        EXPECT_EQ(extensions::PermissionsData::PageAccess::kWithheld,
+                  extension->permissions_data()->GetPageAccess(
+                      kUrl, tab_id, /*error=*/nullptr));
+        break;
+      case PageAccess::kPending:
+        action_runner->RequestScriptInjectionForTesting(
+            extension.get(), extensions::UserScript::DOCUMENT_IDLE,
+            base::DoNothing());
+        break;
+      case PageAccess::kGranted:
+        permissions_modifier.GrantHostPermission(kUrl);
+        break;
+    }
+
+    std::unique_ptr<IconWithBadgeImageSource> image_source =
+        controller->GetIconImageSourceForTesting(web_contents, kSize);
+    EXPECT_EQ(test_case.expected_opacity == Opacity::kGrayscale,
+              image_source->grayscale());
+    EXPECT_EQ(test_case.expected_blocked_actions == BlockedActions::kPainted,
+              image_source->paint_blocked_actions_decoration());
+
+    // Clean up permissions state.
+    if (test_case.page_access == PageAccess::kGranted)
+      permissions_modifier.RemoveGrantedHostPermission(kUrl);
+    action_runner->ClearInjectionsForTesting(*extension);
+  }
+}