Integrates a custom JumpList of Windows 7....

Integrates a custom JumpList of Windows 7 into Chromium.
This change adds an option "--enable-custom-jumplist" that uses the ICustomDestinationList interface to add "Most Visited" pages, "Recently Closed" pages, and "Tasks" to the JumpList of Chromium.
This change registers the CustomJumpList class into an observer of TabRestoreService so it can update the JumpList when a user adds/removes a tab.
This change stores icon files used by a custom JumpList under "$(User Data Dir)/JumpListIcons" so Taskbar can show JumpList icons even when Chromium is not running.

BUG=8037
TEST=Right-click the taskbar icon of Chromium on Windows 7.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21522

[Glen Murphy, 2009-07-15 22:32:51]

LG, some questions:

http://codereview.chromium.org/56175/diff/14061/14066
File chrome/browser/custom_jumplist_win.cc (right):

http://codereview.chromium.org/56175/diff/14061/14066#newcode38
Line 38: // TODO(hbono): to be deleted them when we use Windows SDK 7.0.
Can you file and reference a bug for this?

http://codereview.chromium.org/56175/diff/14061/14066#newcode512
Line 512: //   "chrome.exe <url-to-the-web-page>".
If the user has arguments defined in their shortcut, will those get applied when
opening from a jumplist? (I assume so, just double-checking).

http://codereview.chromium.org/56175/diff/14061/14069
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/56175/diff/14061/14069#newcode553
Line 553: // Enables the custom JumpList on Windows 7.
What conditions will have to be met before we turn this on by default?

[Hironori Bono, 2009-07-17 09:09:08]

Thank you for your review and comments.
As noted in my reply, I noticed this change had a problem and I have been
writing another change to fix it. When I fix this problem, I'm going to update
this change.

Regards,

http://codereview.chromium.org/56175/diff/14061/14066
File chrome/browser/custom_jumplist_win.cc (right):

http://codereview.chromium.org/56175/diff/14061/14066#newcode38
Line 38: // TODO(hbono): to be deleted them when we use Windows SDK 7.0.
On 2009/07/15 22:32:51, Glen Murphy wrote:
> Can you file and reference a bug for this?

Thank you for noticing this.
I filed Issue 16903 <http://crbug.com/16903> for this.

http://codereview.chromium.org/56175/diff/14061/14066#newcode512
Line 512: //   "chrome.exe <url-to-the-web-page>".
On 2009/07/15 22:32:51, Glen Murphy wrote:
> If the user has arguments defined in their shortcut, will those get applied
when
> opening from a jumplist? (I assume so, just double-checking).

Yes. But I noticed today this causes a problem: when users has URLs defined in
their shortcuts, those URLs also get applied when opening from a JumpList.
Before fixing this issue, it is better to create another change that adds a
method to the CommandLine class, fixes a very small issue in
PrefixedSwitchString(), and adds unit tests.
When I finish them, I'm going to update this change.

http://codereview.chromium.org/56175/diff/14061/14069
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/56175/diff/14061/14069#newcode553
Line 553: // Enables the custom JumpList on Windows 7.
On 2009/07/15 22:32:51, Glen Murphy wrote:
> What conditions will have to be met before we turn this on by default?

I think it is good to turn this option by default when we finalize items to be
added to our customized JumpList, e.g. its "Task" category currently has "New
Window" and "New Incognito Window" but I'm wondering if they are sufficient.

[cpu_(ooo_6.6-7.5), 2009-07-20 22:02:52]

My bigger question is that it seems we are doing file IO in the UI thread... is
that so?

http://codereview.chromium.org/56175/diff/15047/15055
File chrome/browser/custom_jumplist_win.cc (right):

http://codereview.chromium.org/56175/diff/15047/15055#newcode107
Line 107: // END OF MICROSOFT CODE
please use "end of windows 7 sdk definitions".

http://codereview.chromium.org/56175/diff/15047/15055#newcode121
Line 121: };
Seems that lines 109 to 121 are to be above the line 107. Feel free to have two
"namespace { }" in this file.

http://codereview.chromium.org/56175/diff/15047/15055#newcode135
Line 135: return InitPropVariantFromString(value.c_str(), &property_);
I looked at the documentation an it is not clear what this function does... is
it making a copy of the string or just storing the ptr to yours? if it is the
later then we might have a bug.

http://codereview.chromium.org/56175/diff/15047/15055#newcode584
Line 584: file_util::Delete(icon_dir_, true);
The delete probably fails if the shell or us is holding open handles to the
icons. What happens then?

[Ben Goodger (Google), 2009-07-20 22:08:43]

http://codereview.chromium.org/56175/diff/15047/15054
File chrome/browser/custom_jumplist_win.h (right):

http://codereview.chromium.org/56175/diff/15047/15054#newcode92
Line 92: class CustomJumpList : public TabRestoreService::Observer {
I would probably just omit "Custom" from the class name... it's a given that an
application providing a jump-list is providing a custom one.

http://codereview.chromium.org/56175/diff/15047/15052
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/56175/diff/15047/15052#newcode289
Line 289: scoped_ptr<CustomJumpList> jumplist_;
I'm not sure that this is the best place for this object to live - it seems more
likely that the jumplist is an application-level service, right? Maybe it should
at least live on the Browser/BrowserView object. The rationale is that the
RenderWidgetHostViewWin is at a lower level of abstraction in the Chrome code
than application-level features like jumplists.

[Hironori Bono, 2009-07-22 06:01:43]

Thank you for your comments and sorry for my slow update.
I have applied your comments as much as possible. Also I have created a new Task
JumpListUpdateTask so that I can move all file operations and OS-specific
operations into the task.
Is it possible to review the updated change?

http://codereview.chromium.org/56175/diff/15047/15055
File chrome/browser/custom_jumplist_win.cc (right):

http://codereview.chromium.org/56175/diff/15047/15055#newcode107
Line 107: // END OF MICROSOFT CODE
On 2009/07/20 22:02:52, cpu wrote:
> please use "end of windows 7 sdk definitions". 

Done. Thank you for correcting my poor wording.

http://codereview.chromium.org/56175/diff/15047/15055#newcode121
Line 121: };
On 2009/07/20 22:02:52, cpu wrote:
> Seems that lines 109 to 121 are to be above the line 107. Feel free to have
two
> "namespace { }" in this file.

Done. I was wondering if I can use two or more anonymous namespaces in a file.
Thank you for noticing this.

http://codereview.chromium.org/56175/diff/15047/15055#newcode135
Line 135: return InitPropVariantFromString(value.c_str(), &property_);
On 2009/07/20 22:02:52, cpu wrote:
> I looked at the documentation an it is not clear what this function does... is
> it making a copy of the string or just storing the ptr to yours? if it is the
> later then we might have a bug.

Done. This InitPropVariantFromString() is an inline function defined in
<provarutil.h>. It creates a copy of an input string with a SHStrDupW() call.
So, I think it is OK to call it without making a copy by ourselves. I have added
a comment that writes this here.

http://codereview.chromium.org/56175/diff/15047/15055#newcode584
Line 584: file_util::Delete(icon_dir_, true);
On 2009/07/20 22:02:52, cpu wrote:
> The delete probably fails if the shell or us is holding open handles to the
> icons. What happens then?

Simply it leaves a garbage file until we try deleting again.

By the way, as you concerned, it is very bad to call such file operations in a
UI thread. (I forgot that this function is called from a UI thread. Thank you
for noticing this.)
I created a class "JumpListUpdateTask" that implements the Task class, and I
moved all file operations in its Run() method. (This task is posted to a file
thread when we complete all queries to HistoryService.)

http://codereview.chromium.org/56175/diff/15047/15054
File chrome/browser/custom_jumplist_win.h (right):

http://codereview.chromium.org/56175/diff/15047/15054#newcode92
Line 92: class CustomJumpList : public TabRestoreService::Observer {
On 2009/07/20 22:08:43, Ben Goodger wrote:
> I would probably just omit "Custom" from the class name... it's a given that
an
> application providing a jump-list is providing a custom one.

Done. Thank you for noticing this.

http://codereview.chromium.org/56175/diff/15047/15052
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/56175/diff/15047/15052#newcode289
Line 289: scoped_ptr<CustomJumpList> jumplist_;
On 2009/07/20 22:08:43, Ben Goodger wrote:
> I'm not sure that this is the best place for this object to live - it seems
more
> likely that the jumplist is an application-level service, right? Maybe it
should
> at least live on the Browser/BrowserView object. The rationale is that the
> RenderWidgetHostViewWin is at a lower level of abstraction in the Chrome code
> than application-level features like jumplists.

Done. You are correct. It is definitely better to move this class to
BrowserView. I have moved this class to BrowserView. Thank you for noticing
this.

[Ben Goodger (Google), 2009-07-22 06:07:57]

LGTM if Carlos is happy with the windowsy bits.

Just a few minor nit comments.

Thanks for completing this!

http://codereview.chromium.org/56175/diff/15065/15068
File chrome/browser/jumplist_win.h (right):

http://codereview.chromium.org/56175/diff/15065/15068#newcode186
Line 186: #endif  // CHROME_BROWSER_JUMPLIST_WIN_H_
nit: no need for _WIN in the file name or in these #defines... jump-lists are
inherently windows specific.

http://codereview.chromium.org/56175/diff/15065/15069
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/56175/diff/15065/15069#newcode1492
Line 1492: }
nit: You don't need all the NULL checks unless you have reason to believe
something is NULL for a valid reason. This is simpler:

if (JumpList::Enabled()) {
  jumplist_.reset(new JumpList);
  jumplist_->AddObserver(browser_->profile());
}

[cpu_(ooo_6.6-7.5), 2009-07-22 21:12:22]

LGTM with one comment.

http://codereview.chromium.org/56175/diff/15065/15067
File chrome/browser/jumplist_win.cc (right):

http://codereview.chromium.org/56175/diff/15065/15067#newcode727
Line 727: most_visited_pages_,
note that the Jumplist object can be destroyed before the task gets into the
Run() method. Make sure that the two last parameters are properly recounted (as
they seem to be) and their subobjects are as well (I am not sure), so that the
task does not refer freed objects.

[Hironori Bono, 2009-07-24 13:01:09]

Thank you so much for your reviews and comments. They are definitely helpful for
me to improve this change.
I have finally submitted this change as r21522.

http://codereview.chromium.org/56175/diff/15065/15067
File chrome/browser/jumplist_win.cc (right):

http://codereview.chromium.org/56175/diff/15065/15067#newcode727
Line 727: most_visited_pages_,
On 2009/07/22 21:12:22, cpu wrote:
> note that the Jumplist object can be destroyed before the task gets into the
> Run() method. Make sure that the two last parameters are properly recounted
(as
> they seem to be) and their subobjects are as well (I am not sure), so that the
> task does not refer freed objects. 

Done. Thank you for your suggestion.
As far as I tested on my local PC (i.e. deleted this JumpList object in this
function), it seems ShellLinkItem objects (which inherits from ref_counted<T>)
in these listed are properly recounted.

By the way, I noticed most part of this JumpList class was platform-independent
since I moved platform-dependent code into this JumpListUpdateTask. That is, we
may write unit-tests for this JumpList class when we write a mock task, such as
MockJumpListUpdateTask, which does not do anything in its Run() method. I'm
implementing its prototype to prove this idea.

http://codereview.chromium.org/56175/diff/15065/15068
File chrome/browser/jumplist_win.h (right):

http://codereview.chromium.org/56175/diff/15065/15068#newcode186
Line 186: #endif  // CHROME_BROWSER_JUMPLIST_WIN_H_
On 2009/07/22 06:08:00, Ben Goodger wrote:
> nit: no need for _WIN in the file name or in these #defines... jump-lists are
> inherently windows specific.

Done. Thank you.

http://codereview.chromium.org/56175/diff/15065/15069
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/56175/diff/15065/15069#newcode1492
Line 1492: }
On 2009/07/22 06:08:00, Ben Goodger wrote:
> nit: You don't need all the NULL checks unless you have reason to believe
> something is NULL for a valid reason. This is simpler:
> 
> if (JumpList::Enabled()) {
>   jumplist_.reset(new JumpList);
>   jumplist_->AddObserver(browser_->profile());
> }
> 

Done. Thank you for your suggestion.