Add option to install an ephemeral app to the Windows jump list

chrome/browser/ui/views/apps/jumplist_updater_win.cc

+// Copyright 2014 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/views/apps/jumplist_updater_win.h"
+
+#include <propvarutil.h>
+#include <shobjidl.h>
+#include <windows.h>
+// This include needs to go out of order to prevent compilation errors because

[tapted, 2014/02/12 10:16:28]

It's actually pretty common to always ensure <windows.h> is the first include,
before any other windows-related includes. E.g. chrome_browser_main_win.cc does
this. I think windows headers regularly get confused if it's not done this way -
that might also fix the issue, and be a bit more robust.

+// propidl.h does not check whether PID_FIRST_USABLE is already defined.
+#include <propkey.h>
+
+#include "base/command_line.h"
+#include "base/path_service.h"
+#include "base/win/scoped_comptr.h"
+#include "base/win/scoped_propvariant.h"
+#include "base/win/windows_version.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_switches.h"
+
+namespace {
+
+void AddShellLink(base::win::ScopedComPtr<IObjectCollection> collection,
+                  const std::wstring& application,
+                  const std::wstring& switches,
+                  const JumpListUpdater::ListItem& list_item) {
+  base::win::ScopedComPtr<IShellLink> link;
+  HRESULT result = link.CreateInstance(CLSID_ShellLink, NULL,
+                                       CLSCTX_INPROC_SERVER);
+  if (FAILED(result))
+    return;
+
+  // Set the application path to invoke for each link.
+  result = link->SetPath(application.c_str());

[tapted, 2014/02/12 10:16:28]

( would become SetPath(command_line.GetProgram().value()); )

+  if (FAILED(result))
+    return;
+
+  // Set the command line args.

[tapted, 2014/02/12 10:16:28]

And, e.g., I think this would be nicer if you could just do

link->SetArguments(command_line.GetArgumentsString().c_str());

+  std::wstring arguments(switches);
+  if (!list_item.command_line_args.empty()) {
+    arguments.push_back(L' ');
+    arguments += list_item.command_line_args;
+  }
+  if (!arguments.empty()) {
+    result = link->SetArguments(arguments.c_str());
+    if (FAILED(result))
+      return;
+  }
+
+  // Set the icon. This can be allowed to fail.
+  if (!list_item.icon_path.empty())
+    link->SetIconLocation(list_item.icon_path.value().c_str(), 0);

[tapted, 2014/02/12 10:16:28]

This could potentially use chrome.exe, and just provide an icon index as the
second argument (or a negative icon "id"); adding a new icon resource to
`chrome_exe.rc`. Did you consider this?

I'm actually not sure of all the pros/cons. I know chrome.exe already has a few
icon assets on it, and it would avoid some of the extra gyp stuff you needed to
do.

+
+  // Set the title. This must be added as a property.
+  base::win::ScopedComPtr<IPropertyStore> property_store;
+  result = link.QueryInterface(property_store.Receive());
+  if (FAILED(result))
+    return;
+
+  base::win::ScopedPropVariant property_title;
+  result = InitPropVariantFromString(list_item.title.c_str(),

[tapted, 2014/02/12 10:16:28]

I'm a bit unfamiliar with this stuff, but can you use things like
`SetStringValueForPropertyStore` from win_util.h to avoid some of this
boilerplate?

+                                     property_title.Receive());
+  if (FAILED(result))
+    return;
+
+  result = property_store->SetValue(PKEY_Title, property_title.get());
+  if (FAILED(result))
+    return;
+
+  result = property_store->Commit();
+  if (FAILED(result))
+    return;
+
+  // Add this IShellLink to the given collection.
+  collection->AddObject(link);
+}
+
+void AddTasks(base::win::ScopedComPtr<ICustomDestinationList> jumplist,
+              const std::wstring& application,
+              const std::wstring& switches,
+              const std::vector<JumpListUpdater::ListItem>& items) {
+  // Create a collection to store items for the "Tasks" category of the
+  // jump list.
+  base::win::ScopedComPtr<IObjectCollection> collection;
+  HRESULT result = collection.CreateInstance(CLSID_EnumerableObjectCollection,
+                                             NULL, CLSCTX_INPROC_SERVER);
+  if (FAILED(result))
+    return;
+
+  // Add list items.
+  for (std::vector<JumpListUpdater::ListItem>::const_iterator it =
+          items.begin(); it != items.end(); ++it) {
+    AddShellLink(collection, application, switches, *it);
+  }
+
+  // Cast to IObjectArray and update the jump list's "Tasks".
+  base::win::ScopedComPtr<IObjectArray> object_array;
+  result = collection.QueryInterface(object_array.Receive());
+  if (FAILED(result))
+    return;
+
+  jumplist->AddUserTasks(object_array);
+}
+
+}  // namespace
+
+// static
+void JumpListUpdater::AppendArgs(const std::string& switch_string,
+                                 const base::string16& value,
+                                 base::string16* command_line_args) {
+  DCHECK(command_line_args);
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+  if (value.empty())
+    command_line.AppendSwitch(switch_string);
+  else
+    command_line.AppendSwitchNative(switch_string, value);
+
+  if (!command_line_args->empty())
+    command_line_args->push_back(L' ');
+  *command_line_args += command_line.GetCommandLineString();
+}
+
+JumpListUpdater::JumpListUpdater(
+    Profile* profile,
+    const base::string16& app_user_model_id)
+        : profile_(profile),
+          app_user_model_id_(app_user_model_id) {
+  PathService::Get(base::FILE_EXE, &chrome_path_);
+
+  // Set up common command line switches.
+  base::FilePath user_data_dir = CommandLine::ForCurrentProcess()->
+      GetSwitchValuePath(switches::kUserDataDir);

[tapted, 2014/02/12 10:16:28]

Do other things do it like this? It feels unreliable - e.g. if the argument on
the command line is a relative path or something. Most things get the user data
dir from PathService. Is there a downside to doing that, and always adding it?

+  if (!user_data_dir.empty())

[tapted, 2014/02/12 10:16:28]

nit: curlies for multi-line if

+    AppendArgs(switches::kUserDataDir,
+               user_data_dir.value(),
+               &default_chrome_switches_);
+}
+
+JumpListUpdater::~JumpListUpdater() {}
+
+void JumpListUpdater::Update(const std::vector<ListItem>& items) {
+  // Check all the preconditions for creating a jump list.
+  if (chrome_path_.empty())
+    return;
+
+  if (base::win::GetVersion() < base::win::VERSION_WIN7)
+    return;
+
+  // Create the jump list.
+  base::win::ScopedComPtr<ICustomDestinationList> jumplist;
+  HRESULT result = jumplist.CreateInstance(CLSID_DestinationList, NULL,
+                                           CLSCTX_INPROC_SERVER);
+  if (FAILED(result))
+    return;
+
+  // Associate the jump list with this application.
+  jumplist->SetAppID(app_user_model_id_.c_str());
+
+  // Start transaction to modify the jump list.
+  UINT max_slots; // unused

[tapted, 2014/02/12 10:16:28]

nit: `  // Unused.` (with 2 spaces between code and comments -- the "? errors"
button in codereview still picks up these, but it's otherwise pretty useless

+  base::win::ScopedComPtr<IObjectArray> removed; // unused
+  result = jumplist->BeginList(&max_slots, __uuidof(*removed),
+                               reinterpret_cast<void**>(&removed));

[tapted, 2014/02/12 10:16:28]

would `removed.ReceiveVoid()` work here in place of the reinterpret_cast?

+  if (FAILED(result))
+    return;
+
+  // Add items to the predefined "Tasks" category.
+  AddTasks(jumplist,
+           chrome_path_.value(),
+           default_chrome_switches_,
+           items);
+
+  // Commit changes to the jump list.
+  jumplist->CommitList();
+}