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

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

Patch summary:
- This patch adds an entry to install an ephemeral app to the Windows
jump list of a packaged app. Jump lists are available in Win 7 and
later.
- Windows OS specific code was moved from NativeAppWindowViews to
derived class NativeAppWindowViewsWin.

BUG=339253

[tmdiep, 2014-02-10 04:33:22]

benwells: Please review this patch containing the first UI surface for pinning
ephemeral apps. Thanks.

[benwells, 2014-02-10 21:31:20]

I started looking then realized Trent might be a good reviewer for this.

https://codereview.chromium.org/158643002/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/158643002/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:1034: void
NativeAppWindowCocoa::UpdateAppMenu() {
Nit: add NOTIMPLEMENTED and a TODO to implement this.

https://codereview.chromium.org/158643002/diff/1/chrome/browser/ui/gtk/apps/n...
File chrome/browser/ui/gtk/apps/native_app_window_gtk.cc (right):

https://codereview.chromium.org/158643002/diff/1/chrome/browser/ui/gtk/apps/n...
chrome/browser/ui/gtk/apps/native_app_window_gtk.cc:722: void
NativeAppWindowGtk::UpdateAppMenu() {}
Nit: add NOTIMPLEMENTED

[tapted, 2014-02-12 10:16:27]

Hooray! removing #ifdefs from native_app_window_views \o/!

I jumped around a bit in the review, so you might want to read all the comments
first. Some might be a bit off the mark -- Windows APIs still feel like a lot of
voodoo to me sometimes.

It might also be nice to split out the #ifdef-removal (and refactoring around
that) to a separate CL, since it's orthogonal (and less controversial ;).

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/jumplist_updater_win.cc (right):

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:10: // This include needs
to go out of order to prevent compilation errors because
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.

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:36: result =
link->SetPath(application.c_str());
( would become SetPath(command_line.GetProgram().value()); )

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:40: // Set the command line
args.
And, e.g., I think this would be nicer if you could just do

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

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:54:
link->SetIconLocation(list_item.icon_path.value().c_str(), 0);
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.

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:63: result =
InitPropVariantFromString(list_item.title.c_str(),
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?

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:134:
GetSwitchValuePath(switches::kUserDataDir);
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?

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:135: if
(!user_data_dir.empty())
nit: curlies for multi-line if

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:162: UINT max_slots; //
unused
nit: `  // Unused.` (with 2 spaces between code and comments -- the "? errors"
button in codereview still picks up these, but it's otherwise pretty useless

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.cc:165:
reinterpret_cast<void**>(&removed));
would `removed.ReceiveVoid()` work here in place of the reinterpret_cast?

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/jumplist_updater_win.h (right):

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.h:12: #include
"ui/gfx/image/image.h"
nit: don't think this is used

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.h:38: : title(title),
icon_path(icon_path), command_line_args(args) {}
I don't think anything's calling this constructor. Remove?

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.h:40: ListItem(const
base::string16& title, const base::FilePath& icon_path)
nit: non-trivial constructors shouldn't be defined in headers (I think clang
emits errors, but clang doesn't (yet!) usually run on windows-only files).

This class also has a non-trivial destructor which will be auto-generated (and
also inlined), so it needs a ~ListItem(); declared here and given an empty body
in the .cc

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.h:44: // Append a switch and
value to command line args.
This feels a bit out of place here (e.g. it's not clear why this is special to
JumpListUpdater). And dealing with strings means that some of the "append stuff"
logic is repeated from CommandLine::GetCommandLineString() - I think you can
lean on CommandLine more.

I'm thinking that it might be neater to have data members just: profile_,
app_user_model_. Then construct the full CommandLine each time in Update(), like

PathService::Get(base::FILE_EXE, &chrome_path);
CommandLine command_line(chrome_path);
AppendCommonSwitches(&command_line);
AddTasks(jumplist, command_line, items);  // Note different signature.

Then, in ListItem, replace `command_line_args` with a `CommandLine args`
(initialized with CommandLine::NO_PROGRAM in the constructor).

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/jumplist_updater_win.h:54: void Update(const
std::vector<ListItem>& items);
Actually, partway through the review, I just found `ShellLinkItemList` from
`src/chrome/browser/jumplist_win.h`. I think this function could potentially
take a `const ShellLinkItemList& items`.  Is the ShellLinkItem class usable in
place of `ListItem` (maybe after some refactoring to make it more flexible)?

Then... I think we need to consider exposing `AddShellLink` form the anonymous
namespace in jumplist_win.cc. It really is too similar to justify maintaining
them both (unless there's a good reason :). It might make my ~CommandLine
comments obsolete (unless jumplist_win.cc gets refactored as well), but I think
there are strong code-hygiene benefits if we share code with jumplist_win.cc.

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_app_window_views.h (right):

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_views.h:75: Profile* profile()
const { return shell_window_->profile(); }
nit: there's usually a guideline that `const` functions shouldn't return
non-const pointers - was it needed for something?

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_app_window_views_win.cc (right):

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_views_win.cc:35: const
base::FilePath web_app_path,
nit: const reference, same for icon_file

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_views_win.cc:58:
!base::CreateDirectory(web_app_path))
nit (although you're just moving it..): curlies for multi-line if

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_app_window_views_win.h (right):

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_views_win.h:35: void
OnShortcutInfoLoaded(
nit: move non-overrides up below ActivateParentDesktopIfNecessary

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_views_win.h:42:
scoped_ptr<JumpListUpdater> jumplist_updater_;
I don't think this is really needed - it doesn't hold any state that can't just
be fetched each time in UpdateAppMenu(). The only bit that really feels
heavyweight is generating the app_id/taskbar group, and this isn't specific to
JumpListUpdater, so it probably belongs here instead. (i.e. replace this data
member with `wstring taskbar_app_id` or similar)

I think if we introduce a ~"modify jumplist" API then we will need some extra
state, but it shouldn't be windows-specific (i.e. would go on a parent class,
and be more generic).

[grt (UTC plus 2), 2014-02-12 16:04:43]

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_app_window_views_win.cc (right):

https://codereview.chromium.org/158643002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_views_win.cc:67: const wchar_t
kInstallIconFileName[] = L"star.ico";
can this be in a resource of chrome.exe or something? is there a benefit to it
being a new file?

[tmdiep, 2014-02-12 22:06:20]

Thanks for the awesome detailed review!

I'll split this into 3 patches:
1. Move Windows specific code into NativeAppWindowViewsWin.
2. Move as much boilerplate for dealing with Windows jump lists as possible out
of jumplist_win and into some util class or set of functions, then refactor
jumplist_win.
3. Jump list for app windows.

I'll address your review comments in these new patches. Hopefully I won't miss
any.