Add a flag to control whether there is a shortcut for the app list / launcher in the Windows taskba…

chrome/browser/ui/views/app_list/app_list_controller_win.cc

Index: chrome/browser/ui/views/app_list/app_list_controller_win.cc
diff --git a/chrome/browser/ui/views/app_list/app_list_controller_win.cc b/chrome/browser/ui/views/app_list/app_list_controller_win.cc
index 982a0171829e228301c6a8df6edfc271e8579393..6c30cb0c13df5529dca0feae68bd32533f55d0a9 100644
--- a/chrome/browser/ui/views/app_list/app_list_controller_win.cc
+++ b/chrome/browser/ui/views/app_list/app_list_controller_win.cc
@@ -5,11 +5,13 @@
 #include <sstream>
 
 #include "base/command_line.h"
+#include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/path_service.h"
 #include "base/time.h"
 #include "base/timer.h"
 #include "base/utf_string_conversions.h"
+#include "base/win/shortcut.h"
 #include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -23,7 +25,10 @@
 #include "chrome/browser/ui/views/browser_dialogs.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
+#include "content/public/browser/browser_thread.h"
+#include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
+#include "grit/google_chrome_strings.h"
 #include "ui/app_list/app_list_view.h"
 #include "ui/app_list/pagination_model.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -40,6 +45,34 @@ namespace {
 // amount.
 static const int kAnchorOffset = 25;
 
+CommandLine GetAppListCommandLine() {
+  CommandLine* current = CommandLine::ForCurrentProcess();
+  CommandLine command_line(current->GetProgram());
+
+  if (current->HasSwitch(switches::kUserDataDir)) {

[grt (UTC plus 2), 2012/11/01 23:18:57]

use command_line.CopySwitchesFrom(*current, ...) instead

[benwells, 2012/11/02 01:33:55]

Done.

+    FilePath user_data_dir = current->GetSwitchValuePath(
+        switches::kUserDataDir);
+    command_line.AppendSwitchPath(switches::kUserDataDir, user_data_dir);
+  }
+
+  command_line.AppendSwitch(switches::kShowAppList);
+  return command_line;
+}
+
+string16 GetAppModelId() {
+  // The AppModelId should be the same for all profiles in a user data directory
+  // but different for different user data directories, so base it on the
+  // initial profile in the current user data directory.
+  FilePath initial_profile_path;
+  CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kUserDataDir)) {
+    initial_profile_path =
+        command_line->GetSwitchValuePath(switches::kUserDataDir).AppendASCII(
+            chrome::kInitialProfile);
+  }
+  return ShellIntegration::GetAppListAppModelIdForProfile(initial_profile_path);
+}
+
 class AppListControllerDelegateWin : public AppListControllerDelegate {
  public:
   AppListControllerDelegateWin();
@@ -86,9 +119,7 @@ class AppListController {
       views::BubbleBorder::ArrowLocation* arrow,
       gfx::Point* anchor);
   void UpdateArrowPositionAndAnchorPoint(app_list::AppListView* view);
-  CommandLine GetAppListCommandLine();
   string16 GetAppListIconPath();
-  string16 GetAppModelId();
 
   // Check if the app list or the taskbar has focus. The app list is kept
   // visible whenever either of these have focus, which allows it to be
@@ -264,8 +295,8 @@ void AppListController::GetArrowLocationAndUpdateAnchor(
 
 void AppListController::UpdateArrowPositionAndAnchorPoint(
     app_list::AppListView* view) {
-  static const int kArrowSize = 10;
-  static const int kPadding = 20;
+  const int kArrowSize = 10;

[grt (UTC plus 2), 2012/11/01 23:18:57]

just curious: why these changes?

[benwells, 2012/11/02 01:33:55]

I was asked in another change just to use const, not static const, in a similar
situation.

I got the impression it was a stylistic thing, I'm not aware of the practical
difference in this code. Is it allocated in a different section?

[grt (UTC plus 2), 2012/11/02 04:00:36]

i think they're equivalent (the compiler should know that it doesn't really need
to store the constant anywhere since the address of it isn't taken).  i was just
wondering.  thanks.

+  const int kPadding = 20;
 
   gfx::Size preferred = view->GetPreferredSize();
   // Add the size of the arrow to the space needed, as the preferred size is
@@ -287,20 +318,6 @@ void AppListController::UpdateArrowPositionAndAnchorPoint(
   view->SetAnchorPoint(anchor);
 }
 
-CommandLine AppListController::GetAppListCommandLine() {
-  CommandLine* current = CommandLine::ForCurrentProcess();
-  CommandLine command_line(current->GetProgram());
-
-  if (current->HasSwitch(switches::kUserDataDir)) {
-    FilePath user_data_dir = current->GetSwitchValuePath(
-        switches::kUserDataDir);
-    command_line.AppendSwitchPath(switches::kUserDataDir, user_data_dir);
-  }
-
-  command_line.AppendSwitch(switches::kShowAppList);
-  return command_line;
-}
-
 string16 AppListController::GetAppListIconPath() {
   FilePath icon_path;
   if (!PathService::Get(base::DIR_MODULE, &icon_path))
@@ -314,17 +331,6 @@ string16 AppListController::GetAppListIconPath() {
   return result;
 }
 
-string16 AppListController::GetAppModelId() {
-  static const wchar_t kAppListId[] = L"ChromeAppList";
-  // The AppModelId should be the same for all profiles in a user data directory
-  // but different for different user data directories, so base it on the
-  // initial profile in the current user data directory.
-  FilePath initial_profile_path =
-      g_browser_process->profile_manager()->GetInitialProfileDir();
-  return ShellIntegration::GetAppModelIdForProfile(kAppListId,
-                                                   initial_profile_path);
-}
-
 void AppListController::CheckTaskbarOrViewHasFocus() {
 #if !defined(USE_AURA)
   // Don't bother checking if the view has been closed.
@@ -357,6 +363,69 @@ void AppListController::CheckTaskbarOrViewHasFocus() {
 #endif
 }
 
+// Check that a taskbar shortcut exists if it should, or does not exist if
+// it should not. A taskbar shortcut should exist if the switch
+// kShowAppListShortcut is set. The shortcut will be created or deleted in
+// |user_data_dir| and will use a Windows Application Model Id of
+// |app_model_id|.
+void DoCheckAppListTaskbarShortcut(const FilePath user_data_dir,

[grt (UTC plus 2), 2012/11/01 23:18:57]

nit: i think there's a pseudo-convention of naming funcs like this
"BlahBlahOnFileThread".

[benwells, 2012/11/02 01:33:55]

Done.

+                                   const string16& app_model_id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+
+  const string16 shortcut_name = l10n_util::GetStringUTF16(
+      IDS_APP_LIST_SHORTCUT_NAME);
+  const FilePath shortcut_path(user_data_dir.Append(shortcut_name).
+      AddExtension(L"lnk"));
+  const bool should_show = CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kShowAppListShortcut);
+
+  // This will not reshow a shortcut if it has been unpinned manually by the
+  // user, as that will not delete the shortcut file.
+  if (should_show && !file_util::PathExists(shortcut_path)) {
+    FilePath chrome_exe;
+    if (!PathService::Get(base::FILE_EXE, &chrome_exe))
+      return;
+
+    base::win::ShortcutProperties shortcut_properties;
+    shortcut_properties.set_target(chrome_exe);
+    shortcut_properties.set_working_dir(chrome_exe.DirName());
+
+    string16 wide_switches(GetAppListCommandLine().GetArgumentsString());
+    shortcut_properties.set_arguments(wide_switches);
+    shortcut_properties.set_description(shortcut_name);
+
+    FilePath icon_path;
+    if (!PathService::Get(base::DIR_MODULE, &icon_path))

[grt (UTC plus 2), 2012/11/02 03:09:52]

this is unsafe, see comment below.

+      return;
+
+    icon_path = icon_path.Append(chrome::kBrowserResourcesDll);
+
+    // Icons are added to the resources of the DLL using icon indexes. The
+    // icon index for the app list icon is in IDI_APP_LIST. Creating shortcuts
+    // needs to specify a resource index, which are different to icon indexes.
+    // They are 0 based and contiguous. As Google Chrome builds have an extra
+    // icon (SXS) the icon for Google Chrome builds need to be one higher.
+#if defined(GOOGLE_CHROME_BUILD)
+    const int kIconIndex = 26;

[grt (UTC plus 2), 2012/11/01 23:18:57]

is there a constant in a generated .h file that can be used here instead?

[benwells, 2012/11/02 01:33:55]

Not that I could find. See
chrome/installer/util/google_chrome_sxs_distribution.cc / GetIconIndex for a
similar case.

I would like to be proven wrong here though, I don't like this constant.
Annoyingly in a different API call windows accepts IDI_APP_LIST (which is
different).

[grt (UTC plus 2), 2012/11/02 03:09:52]

Since the path to chrome.dll changes on each update, it isn't safe to have
shortcuts fetch their icons from it. Rather, this icon should be duplicated in
chrome_exe.rc like IDR_MAINFRAME and friends. The icon path for the shortcut
should be the path to chrome.exe. I believe the icon index doesn't take the
resource identifiers into account, so I think you are stuck with making the
index either 5 (one more than the SxS index) or 1 (one more than the mainframe
index).

+#else
+    const int kIconIndex = 25;
+#endif
+    shortcut_properties.set_icon(icon_path, kIconIndex);
+    shortcut_properties.set_app_id(app_model_id);
+    shortcut_properties.set_dual_mode(false);
+
+    base::win::CreateOrUpdateShortcutLink(shortcut_path, shortcut_properties,
+                                          base::win::SHORTCUT_CREATE_ALWAYS);
+    base::win::TaskbarPinShortcutLink(shortcut_path.value().c_str());
+    return;
+  }
+
+  if (!should_show && file_util::PathExists(shortcut_path)) {
+    base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str());
+    file_util::Delete(shortcut_path, false);
+  }
+}
+
 }  // namespace
 
 namespace app_list_controller {
@@ -365,4 +434,13 @@ void ShowAppList() {
   g_app_list_controller.Get().ShowAppList();
 }
 
+void CheckAppListTaskbarShortcut() {
+  FilePath user_data_dir(g_browser_process->profile_manager()->user_data_dir());
+  string16 app_model_id = GetAppModelId();
+  content::BrowserThread::PostTask(

[grt (UTC plus 2), 2012/11/01 23:18:57]

i think the blocking pool is appropriate here (see
BrowserThread::GetBlockingPool in
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OaA-9rnjOqI/H0cDC...)

[benwells, 2012/11/02 01:33:55]

Is that safe? chrome/browser/shell_integration_win.cc MigrateWin7ShortcutsInPath
runs on the FILE thread and will mess with this icon.

[grt (UTC plus 2), 2012/11/02 03:09:52]

I could be mistaken, but at a high level this operation seems appropriate for
the blocking pool.   The comments around PostBlockingPoolTask in
browser_thread.h seem to agree.  File IO is certainly allowed there.  If there
are DCHECKs in shell_integration_win that require the FILE thread (are there?),
then perhaps those can be relaxed to allow operation in the blocking pool.

[grt (UTC plus 2), 2012/11/02 04:00:36]

After chatting, I now understand your concern.  I think you're right that it's a
good idea to keep this on the FILE thread.  Please add a comment here so that
future generations don't convert this to the blocking pool without considering
that other bits and piece of Chrome might be updating shortcuts on the FILE
thread.

+      content::BrowserThread::FILE,
+      FROM_HERE,

[grt (UTC plus 2), 2012/11/01 23:18:57]

nit: move to previous line

[benwells, 2012/11/02 01:33:55]

Done.

+      base::Bind(&DoCheckAppListTaskbarShortcut, user_data_dir, app_model_id));
+}
+
 }  // namespace app_list_controller