Show media router toolbar icon ephemerally for active local routes and issues

chrome/browser/ui/toolbar/media_router_contextual_menu.cc

 // Copyright 2015 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 <string>
+
 #include "base/logging.h"
 #include "base/metrics/user_metrics.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/extensions/component_migration_helper.h"
 #include "chrome/browser/media/router/media_router_factory.h"
 #include "chrome/browser/media/router/mojo/media_router_mojo_impl.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/toolbar/component_toolbar_actions_factory.h"
 #include "chrome/browser/ui/toolbar/media_router_contextual_menu.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "extensions/common/constants.h"
 #include "ui/base/models/menu_model_delegate.h"
 
 #if defined(GOOGLE_CHROME_BUILD)
 #include "chrome/browser/signin/signin_manager_factory.h"
-#include "chrome/common/pref_names.h"
-#include "components/prefs/pref_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #endif  // defined(GOOGLE_CHROME_BUILD)
 
 MediaRouterContextualMenu::MediaRouterContextualMenu(Browser* browser)
+    : MediaRouterContextualMenu(browser,
+                                ToolbarActionsModel::Get(browser->profile())) {}
+
+MediaRouterContextualMenu::~MediaRouterContextualMenu() {}
+
+MediaRouterContextualMenu::MediaRouterContextualMenu(
+    Browser* browser,
+    ToolbarActionsModel* toolbar_actions_model)
     : browser_(browser),
-      menu_model_(this) {
+      menu_model_(this),
+      toolbar_actions_model_(toolbar_actions_model) {

[Devlin, 2016/09/21 17:15:12]

It looks like we only ever use this for the ComponentMigrationHelper - could we
just use that instead?

[takumif, 2016/09/21 17:27:15]

This is to be able to inject a non-NULL ToolbarActionsModel in test. I would
have used TestingProfile::Builder::AddTestingFactory(), except the profile is
already instantiated in MediaRouterContextualMenuUnitTest.

[Devlin, 2016/09/21 17:32:57]

What uses it that needs it to be non-null, rather than passing in the non-null
ComponentMigrationHelper?  It looks like the helper is all that's used here.

[takumif, 2016/09/21 17:45:45]

Right, it's only used for the migration helper. But if the ContextualMenu were
to have a ComponentMigrationHelper* property, it'd have to call

ToolbarActionsModel::Get(profile)->component_migration_helper()

in the ctor, which wouldn't work in tests.

[Devlin, 2016/09/21 17:52:45]

Why?  We have a ctor explicitly for injecting dependencies, so this could just
become
MediaRouterContextualMenu(Browser* browser,
                          ComponentMigrationHelper* component_migration_helper)
    : browser_(browser),
      migration_helper_(helper) {}

The "real" ctor already calls ToolbarActionsModel::Get(), so there's on
difference there.

[takumif, 2016/09/21 17:59:16]

I was thinking that if the real ctor gets called in tests (which happens
whenever MRAction is instantiated) and it had to call

ToolbarActionsModel::Get(profile)->component_migration_helper()

with null ToolbarActionsModel, then that wouldn't work. But I can make it check
if the ToolbarActionsModel is null, and set ComponentMigrationHelper* only if
the ToolbarActionsModel is non-null. Would that be better?

[Devlin, 2016/09/21 18:17:30]

So these are tests that construct a MediaRouterContextualMenu via the main ctor,
but then don't trigger any of the functionality that uses ComponentActionHelper,
is that correct?

That seems a little fragile to me.  Ideally, we'd be able to DCHECK that
component actions helper is never null, because otherwise an unrelated code
change (e.g. calling GetAlwaysShowActionPref()) will cause a failure.

Which tests do this currently?

[takumif, 2016/09/21 18:49:31]

Sorry, I'd mistakenly thought that both the MRContextualMenu and MRAction unit
tests were failing, but it turns out MRAction tests were fine, thanks to

extensions::extension_action_test_util::CreateToolbarModelForProfile().

This works for MRContextualMenu tests as well, so injecting
ToolbarActionsModel/ComponentMigrationHelper is no longer necessary.

   menu_model_.AddItemWithStringId(IDC_MEDIA_ROUTER_ABOUT,
                                   IDS_MEDIA_ROUTER_ABOUT);
   menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
   menu_model_.AddItemWithStringId(IDC_MEDIA_ROUTER_LEARN_MORE,
                                   IDS_MEDIA_ROUTER_LEARN_MORE);
   menu_model_.AddItemWithStringId(IDC_MEDIA_ROUTER_HELP,
                                   IDS_MEDIA_ROUTER_HELP);
-  menu_model_.AddItemWithStringId(IDC_MEDIA_ROUTER_REMOVE_TOOLBAR_ACTION,
-                                  IDS_MEDIA_ROUTER_REMOVE_TOOLBAR_ACTION);
+  menu_model_.AddCheckItemWithStringId(
+      IDC_MEDIA_ROUTER_ALWAYS_SHOW_TOOLBAR_ACTION,
+      IDS_MEDIA_ROUTER_ALWAYS_SHOW_TOOLBAR_ACTION);
   menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
   menu_model_.AddItemWithStringId(IDC_MEDIA_ROUTER_MANAGE_DEVICES,
                                   IDS_MEDIA_ROUTER_MANAGE_DEVICES);
 #endif
 #if defined(GOOGLE_CHROME_BUILD)
   menu_model_.AddCheckItemWithStringId(IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE,
                                        IDS_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE);
 #endif  // defined(GOOGLE_CHROME_BUILD)
   menu_model_.AddItemWithStringId(IDC_MEDIA_ROUTER_REPORT_ISSUE,
                                   IDS_MEDIA_ROUTER_REPORT_ISSUE);
 }
 
-MediaRouterContextualMenu::~MediaRouterContextualMenu() {
+bool MediaRouterContextualMenu::GetAlwaysShowActionPref() const {
+  return toolbar_actions_model_->component_migration_helper()
+      ->GetComponentActionPref(
+          ComponentToolbarActionsFactory::kMediaRouterActionId);
+}
+
+void MediaRouterContextualMenu::SetAlwaysShowActionPref(bool always_show) {
+  toolbar_actions_model_->component_migration_helper()->SetComponentActionPref(
+      ComponentToolbarActionsFactory::kMediaRouterActionId, always_show);
 }
 
 bool MediaRouterContextualMenu::IsCommandIdChecked(int command_id) const {
 #if defined(GOOGLE_CHROME_BUILD)
   if (command_id == IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE) {
     return browser_->profile()->GetPrefs()->GetBoolean(
         prefs::kMediaRouterEnableCloudServices);
   }
 #endif  // defined(GOOGLE_CHROME_BUILD)
+  if (command_id == IDC_MEDIA_ROUTER_ALWAYS_SHOW_TOOLBAR_ACTION) {
+    return GetAlwaysShowActionPref();
+  }
   return false;
 }
 
 bool MediaRouterContextualMenu::IsCommandIdEnabled(int command_id) const {
   return true;
 }
 
 bool MediaRouterContextualMenu::IsCommandIdVisible(int command_id) const {
 #if defined(GOOGLE_CHROME_BUILD)
   if (command_id == IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE) {
@@ skipping 16 matching lines @@
   const char kCastLearnMorePageUrl[] =
       "https://support.google.com/chromecast/answer/2998338";
 
 #if defined(GOOGLE_CHROME_BUILD)
   PrefService* pref_service;
 #endif  // defined(GOOGLE_CHROME_BUILD)
   switch (command_id) {
     case IDC_MEDIA_ROUTER_ABOUT:
       chrome::ShowSingletonTab(browser_, GURL(kAboutPageUrl));
       break;
+    case IDC_MEDIA_ROUTER_ALWAYS_SHOW_TOOLBAR_ACTION:
+      SetAlwaysShowActionPref(!GetAlwaysShowActionPref());
+      break;
 #if defined(GOOGLE_CHROME_BUILD)
     case IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE:
       pref_service = browser_->profile()->GetPrefs();
       pref_service->SetBoolean(prefs::kMediaRouterEnableCloudServices,
           !pref_service->GetBoolean(prefs::kMediaRouterEnableCloudServices));
 
       // If this has been set before, this is a no-op.
-      pref_service->SetBoolean(prefs::kMediaRouterCloudServicesPrefSet,
-                               true);
+      pref_service->SetBoolean(prefs::kMediaRouterCloudServicesPrefSet, true);
       break;
 #endif  // defined(GOOGLE_CHROME_BUILD)
     case IDC_MEDIA_ROUTER_HELP:
       chrome::ShowSingletonTab(browser_, GURL(kCastHelpCenterPageUrl));
       base::RecordAction(base::UserMetricsAction(
           "MediaRouter_Ui_Navigate_Help"));
       break;
     case IDC_MEDIA_ROUTER_LEARN_MORE:
       chrome::ShowSingletonTab(browser_, GURL(kCastLearnMorePageUrl));
       break;
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
     case IDC_MEDIA_ROUTER_MANAGE_DEVICES:
       chrome::ShowSingletonTab(browser_, GURL(chrome::kChromeUICastURL));
       break;
 #endif
-    case IDC_MEDIA_ROUTER_REMOVE_TOOLBAR_ACTION:
-      RemoveMediaRouterComponentAction();
-      break;
     case IDC_MEDIA_ROUTER_REPORT_ISSUE:
       ReportIssue();
       break;
     default:
       NOTREACHED();
   }
 }
 
 void MediaRouterContextualMenu::ReportIssue() {
   // Opens feedback page loaded from the media router extension.
   // This is temporary until feedback UI is redesigned.
   // TODO(crbug.com/597778): remove reference to MediaRouterMojoImpl
   media_router::MediaRouterMojoImpl* media_router =
       static_cast<media_router::MediaRouterMojoImpl*>(
           media_router::MediaRouterFactory::GetApiForBrowserContext(
               static_cast<content::BrowserContext*>(browser_->profile())));
   if (media_router->media_route_provider_extension_id().empty())
     return;
   std::string feedback_url(extensions::kExtensionScheme +
                            std::string(url::kStandardSchemeSeparator) +
                            media_router->media_route_provider_extension_id() +
                            "/feedback.html");
   chrome::ShowSingletonTab(browser_, GURL(feedback_url));
 }
-
-void MediaRouterContextualMenu::RemoveMediaRouterComponentAction() {
-  ToolbarActionsModel::Get(browser_->profile())->component_migration_helper()
-      ->OnActionRemoved(ComponentToolbarActionsFactory::kMediaRouterActionId);
-}