Move Windows Jump List boilerplate out of jumplist_win for reuse

Move Windows Jump List boilerplate out of jumplist_win for reuse

JumpListUpdater encapsulates the boilerplate for updating Windows
jump lists. Most of this code was moved out of jumplist_win for
reuse by app windows. COM interface definitions were removed from
jumplist_win.cc.

BUG=339253
TEST=Right-click on the Chrome icon in the Windows 7+ taskbar and
verify that the functionality has not changed.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251960

[tmdiep, 2014-02-18 02:56:24]

tapted: This is the second patch split from
https://codereview.chromium.org/158643002. I've gone through all your comments
in jumplist_updater_win.h/cc from the original patch and addressed them in this
patch. I hope I haven't forgotten anything.

Thanks!

https://codereview.chromium.org/168583011/diff/30001/chrome/browser/jumplist_...
File chrome/browser/jumplist_win.cc (right):

https://codereview.chromium.org/168583011/diff/30001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.cc:47: const char* kSwitchNames[] = {
switches::kUserDataDir };
On 2014/02/12 10:16:28, tapted wrote:
> 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?

(Copying comment from the original patch that this patch was split from)

A lot of code copies the value of this command line switch instead of using
PathService. My guess is that the --user-data-dir override is not often present
in the command line args. So we prefer not add this switch and use the default
user data dir in any new chrome.exe process if not given.

[tapted, 2014-02-18 06:12:42]

nice! - lgtm

I think it's OK to add BUG=339253 to this too, since it will give OWNERS some
additional context/rationale.

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_upda...
File chrome/browser/jumplist_updater_win.h (right):

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_upda...
chrome/browser/jumplist_updater_win.h:35: void AppendArgument(const
std::wstring& value);
I'd be tempted to add a

CommandLine* GetCommandline() function rather than these. But I think this is OK
too.

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_win.cc
File chrome/browser/jumplist_win.cc (right):

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_win....
chrome/browser/jumplist_win.cc:109: incognito->AppendArgument(
incognito->AppendSwitch(switches::kIncognito)

or

incognito->GetCommandLine()->AppendSwitch(switches::kIncognito);

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_win.h
File chrome/browser/jumplist_win.h (right):

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_win....
chrome/browser/jumplist_win.h:24: class FilePath;
nit: this should be an include, rather than forward-dec since JumpList has a
(non-pointer) FilePath member.

[tmdiep, 2014-02-18 06:58:34]

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_upda...
File chrome/browser/jumplist_updater_win.h (right):

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_upda...
chrome/browser/jumplist_updater_win.h:35: void AppendArgument(const
std::wstring& value);
On 2014/02/18 06:12:43, tapted wrote:
> I'd be tempted to add a
> 
> CommandLine* GetCommandline() function rather than these. But I think this is
OK
> too.

Agreed. I initially wanted to hide the use of CommandLine from the interface,
but it quickly became annoying.

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_win.cc
File chrome/browser/jumplist_win.cc (right):

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_win....
chrome/browser/jumplist_win.cc:109: incognito->AppendArgument(
On 2014/02/18 06:12:43, tapted wrote:
> incognito->AppendSwitch(switches::kIncognito)
> 
> or
> 
> incognito->GetCommandLine()->AppendSwitch(switches::kIncognito);

Done.

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_win.h
File chrome/browser/jumplist_win.h (right):

https://codereview.chromium.org/168583011/diff/7/chrome/browser/jumplist_win....
chrome/browser/jumplist_win.h:24: class FilePath;
On 2014/02/18 06:12:43, tapted wrote:
> nit: this should be an include, rather than forward-dec since JumpList has a
> (non-pointer) FilePath member.

Done.

[tmdiep, 2014-02-18 07:07:56]

jhawkins: This patch is just some refactoring in order to reuse the jump list
code for app windows. There should be no functional change. Thanks in advance.

[James Hawkins, 2014-02-18 16:28:44]

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
File chrome/browser/jumplist_updater_win.h (right):

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:23: // users always use "chrome.exe" as
the application. A custom application path
Optional nit: This last sentence is implied by the second-to-last sentence. 
Consider removing it.

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:37: void SetTitle(const std::wstring&
title) {
setters are like getters, i.e., set_title.

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:105: // Start a transaction that updates
the JumpList of this application.
Starts

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:110: // Commit the update.
Commits

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:113: // Update the predefined "Tasks"
category of the JumpList.
Updates

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:116: // Update an unregistered category of
the JumpList.
Updates

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:131: base::FilePath application_path_;
Why store it as a member variable if it's a constant?

[tmdiep, 2014-02-18 22:33:47]

Thanks for the review. All comments addressed in patch set 6.

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
File chrome/browser/jumplist_updater_win.h (right):

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:23: // users always use "chrome.exe" as
the application. A custom application path
On 2014/02/18 16:28:44, James Hawkins wrote:
> Optional nit: This last sentence is implied by the second-to-last sentence. 
> Consider removing it.

Removed.

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:37: void SetTitle(const std::wstring&
title) {
On 2014/02/18 16:28:44, James Hawkins wrote:
> setters are like getters, i.e., set_title.

Done.

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:105: // Start a transaction that updates
the JumpList of this application.
On 2014/02/18 16:28:44, James Hawkins wrote:
> Starts

Done. As well as similar comments below.

https://codereview.chromium.org/168583011/diff/30007/chrome/browser/jumplist_...
chrome/browser/jumplist_updater_win.h:131: base::FilePath application_path_;
On 2014/02/18 16:28:44, James Hawkins wrote:
> Why store it as a member variable if it's a constant?

Just an optimization to avoid retrieving the value multiple times since it is
used in AddTasks() and AddCustomCategory(). But good point. I've removed the
member variable.

[James Hawkins, 2014-02-18 22:39:47]

lgtm

[tmdiep, 2014-02-18 22:44:04]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-02-18 22:44:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/168583011/390001

[commit-bot: I haz the power, 2014-02-19 06:10:30]

Change committed as 251960