Add Windows desktop shortcut for multiple profiles.

Add Windows desktop shortcut for multiple profiles.

BUG=87770
TEST=new unit tests added with this CL.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111156

[Robert Sesek, 2011-11-16 15:49:16]

[Unsolicited feedback pre-review]

Does this functionality really belong in the ProfileInfoCache? Could instead
another class be created to handle the shortcut functionality and use the
ProfileManagerObserver interface?

[Miranda Callahan, 2011-11-16 16:22:22]

On 2011/11/16 15:49:16, rsesek wrote:
> [Unsolicited feedback pre-review]
> 
> Does this functionality really belong in the ProfileInfoCache? Could instead
> another class be created to handle the shortcut functionality and use the
> ProfileManagerObserver interface?

I agree that all the #if-deffing is bad. Alas, the ProfileManagerObserver only
deals with Profile creation, and specifically for the nitty-gritty of observing
the asynchronous loading phase of creation. It's really badly named (TODO to
self -- fix this).

I could, however, use the NotificationSystem, and create a Win-only utility
object to listen for ProfileInfoCache changes. I would have to add details to
the notifications to give, for example, before and after data for names to be
changed. But this would completely remove any knowledge of Win-specificity from
the PIC. Except for the creation of the Win PICWatcher object; though maybe that
could go in an existing Win-only class. I'll investigate.

[Miranda Callahan, 2011-11-16 23:28:29]

On 2011/11/16 16:22:22, Miranda Callahan wrote:
> On 2011/11/16 15:49:16, rsesek wrote:
> > [Unsolicited feedback pre-review]
> > 
> > Does this functionality really belong in the ProfileInfoCache? Could instead
> > another class be created to handle the shortcut functionality and use the
> > ProfileManagerObserver interface?
> 
> I agree that all the #if-deffing is bad. Alas, the ProfileManagerObserver only
> deals with Profile creation, and specifically for the nitty-gritty of
observing
> the asynchronous loading phase of creation. It's really badly named (TODO to
> self -- fix this).
> 
> I could, however, use the NotificationSystem, and create a Win-only utility
> object to listen for ProfileInfoCache changes. I would have to add details to
> the notifications to give, for example, before and after data for names to be
> changed. But this would completely remove any knowledge of Win-specificity
from
> the PIC. Except for the creation of the Win PICWatcher object; though maybe
that
> could go in an existing Win-only class. I'll investigate.

No need to look at this tonight; I see some more refactoring opportunities, and
I need to add tests. Will let you know when it's time.

[Miranda Callahan, 2011-11-17 21:30:21]

Ready for review, although I'm still working on tests for the new
ProfileInfoCache stuff. Tests have been added for the shell_util and
browser_distribution side.

Robert Sesek and Sailesh -- review the non-shell-util stuff (and anything else
you like).

Robert Shield -- please review the shell util changes.

[Robert Sesek, 2011-11-17 21:53:19]

I think you're missing a file or three.

http://codereview.chromium.org/8502033/diff/35005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/35005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:470: if
(ShellUtil::GetChromeShortcutName(dist, false, name, &shortcut))
Should this logic be in this method, or should this be in the ShortcutManager?

[Miranda Callahan, 2011-11-17 22:04:20]

I'll say! Added.

On 2011/11/17 21:53:19, rsesek wrote:
> I think you're missing a file or three.
> 
>
http://codereview.chromium.org/8502033/diff/35005/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_info_cache.cc (right):
> 
>
http://codereview.chromium.org/8502033/diff/35005/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_info_cache.cc:470: if
> (ShellUtil::GetChromeShortcutName(dist, false, name, &shortcut))
> Should this logic be in this method, or should this be in the ShortcutManager?

Yes, I thought about this refactoring, and then it slipped my mind. Agreed that
it should be in the shortcutmanager -- will change forthwith.

[Miranda Callahan, 2011-11-18 00:15:08]

On 2011/11/17 22:04:20, Miranda Callahan wrote:
> I'll say! Added.
> 
> On 2011/11/17 21:53:19, rsesek wrote:
> > I think you're missing a file or three.
> > 
> >
>
http://codereview.chromium.org/8502033/diff/35005/chrome/browser/profiles/pro...
> > File chrome/browser/profiles/profile_info_cache.cc (right):
> > 
> >
>
http://codereview.chromium.org/8502033/diff/35005/chrome/browser/profiles/pro...
> > chrome/browser/profiles/profile_info_cache.cc:470: if
> > (ShellUtil::GetChromeShortcutName(dist, false, name, &shortcut))
> > Should this logic be in this method, or should this be in the
ShortcutManager?
> 
> Yes, I thought about this refactoring, and then it slipped my mind. Agreed
that
> it should be in the shortcutmanager -- will change forthwith.

Ready for review. I think there could be more refactoring of the balancing of
work between shell_util and the profile_shortcut_manager, but I'll take a fresh
look tomorrow morning.

[Miranda Callahan, 2011-11-18 00:17:45]

On 2011/11/18 00:15:08, Miranda Callahan wrote:
> On 2011/11/17 22:04:20, Miranda Callahan wrote:
> > I'll say! Added.
> > 
> > On 2011/11/17 21:53:19, rsesek wrote:
> > > I think you're missing a file or three.
> > > 
> > >
> >
>
http://codereview.chromium.org/8502033/diff/35005/chrome/browser/profiles/pro...
> > > File chrome/browser/profiles/profile_info_cache.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8502033/diff/35005/chrome/browser/profiles/pro...
> > > chrome/browser/profiles/profile_info_cache.cc:470: if
> > > (ShellUtil::GetChromeShortcutName(dist, false, name, &shortcut))
> > > Should this logic be in this method, or should this be in the
> ShortcutManager?
> > 
> > Yes, I thought about this refactoring, and then it slipped my mind. Agreed
> that
> > it should be in the shortcutmanager -- will change forthwith.
> 
> Ready for review. I think there could be more refactoring of the balancing of
> work between shell_util and the profile_shortcut_manager, but I'll take a
fresh
> look tomorrow morning.

(+Rachel as an FYI, and in case you want to take a look, or split this largish
patch up with Sailesh or Robert...)

[robertshield, 2011-11-18 03:58:57]

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:156: cache->GetDictionary(key,
&info);
check that cache is non null?

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:158: info->GetString(kNameKey,
&name);
check that info is non null? I notice that most of this code doesn't null check,
so maybe it's not needed, but I don't see a documented guarantee that info is
non-null here.

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:242: info->GetString(kNameKey,
&old_name);
same null check grumbling as above, ignore me if need be :)

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:40: return
profile_directory;
base::StringPrintf could make this shorter.

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:49: string16 shortcut,
micro nit: looks like you could wrap this at the ( above.

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:651: if (appended_name.length() > 0) {
!appended_name.empty()

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1007: it++) {
opinions vary, but I find the following formatting cleaner:

std::vector<std::wstring>::iterator it(appended_names.begin());
for (; it != appended_names.end(); ++it) {
...
}

in either case, it++ -> ++it

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:272: // the base links, including appended
names (for example,
Does this mean:
"|appended_names| is a list of shortcut file names as obtained from
ShellUtil::GetChromeShortcutName"? Please clarify a bit if so.

[sail, 2011-11-18 07:14:24]

LGTM
Profile info cache code looks good to me!

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.h:91: static std::vector<string16>
GetProfileNames();
How about just creating a new instance of ProfileInfoCache and using that
instead?

[Robert Sesek, 2011-11-18 14:13:43]

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:460: it != cache->end_keys();
nit: align with b in base

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:217:
profile_info_cache_->RemoveObserver(profile_shortcut_manager_.get());
The ShortcutManager could add and remove itself as an observer in its ctor and
dtor.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:88: } else { // Only one
profile, so create original shortcut.
nit: two spaces before comment, or push to next line

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:21:
~ProfileShortcutManagerWin();
virtual

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:30: // Wrap a ShellUtil
function that returns a bool so it can be posted in a
private:?

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:32: static void
CallShellUtilBoolFunction(
This could go in the anon namespace in the .cc because it's very
implementation-y.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:38: std::vector<string16>
profile_names);
I think these methods can take const-ref arguments. I believe the callback
system will copy into InvokerStorage when it gets passed across threads, so when
your callback runs, a const-ref is OK to take.

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:275: std::vector<std::wstring>
appended_names);
const ref

[grt (UTC plus 2), 2011-11-18 16:02:15]

drive-by

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:150:
ProfileInfoCache::GetProfileNames())))
nit: braces are required for these since the conditionals span more than one
line.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:464: string16 name;
nit: move this out of the loop so it isn't constructed/destructed on each
iteration.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache_observer.h:13:
ProfileInfoCacheObserver() {}
google style says ctor must be protected.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache_observer.h:15: virtual void
OnProfileAdded(string16 profile_name,
can these not be passed as const ref?

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache_observer.h:22: virtual
~ProfileInfoCacheObserver() {}
this should be public unless you're really trying for tricky deny-destruction
stuff.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:217:
profile_info_cache_->RemoveObserver(profile_shortcut_manager_.get());
On 2011/11/18 14:13:43, rsesek wrote:
> The ShortcutManager could add and remove itself as an observer in its ctor and
> dtor.

If you go that route, be sure to either explicitly insert
profile_shortcut_manager_.reset() here to force its destruction, or document in
the .h that the profile_shortcut_manager_ member MUST come after the
profile_info_cache_ member so that its dtor runs first.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:99:
std::vector<string16> shortcuts;
construct the vector with the one shortcut in it:
std::vector<string16> shortcuts(1, shortcut);

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:100:
shortcuts.push_back(shortcut);
remove this line.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:140: for
(std::vector<string16>::iterator it = profile_names.begin();
insert "shortcuts.reserve(profile_names.size());" above this line.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:140: for
(std::vector<string16>::iterator it = profile_names.begin();
const_iterator

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:210:
UTF8ToUTF16(shortcut_path.MaybeAsASCII()),
MaybeAsASCII() is bad since shortcuts are localized.  is shortcut_path.value()
not what you want (in place of UTF8ToUTF16(shortcut_path.MaybeAsASCII()))?

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1005: for
(std::vector<std::wstring>::iterator it = appended_names.begin();
const_iterator

[Miranda Callahan, 2011-11-18 19:00:36]

Addressed concerns, rebased, running through the bots -- please PTAL.

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:156: cache->GetDictionary(key,
&info);
On 2011/11/18 03:58:57, robertshield wrote:
> check that cache is non null?

Done.

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:158: info->GetString(kNameKey,
&name);
On 2011/11/18 03:58:57, robertshield wrote:
> check that info is non null? I notice that most of this code doesn't null
check,
> so maybe it's not needed, but I don't see a documented guarantee that info is
> non-null here.

Done.

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:242: info->GetString(kNameKey,
&old_name);
On 2011/11/18 03:58:57, robertshield wrote:
> same null check grumbling as above, ignore me if need be :)

Added null DCHECK directly to GetInfoForProfileAtIndex.

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:40: return
profile_directory;
On 2011/11/18 03:58:57, robertshield wrote:
> base::StringPrintf could make this shorter.

Ah, didn't know about that. Thanks, done.

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8502033/diff/34007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:49: string16 shortcut,
On 2011/11/18 03:58:57, robertshield wrote:
> micro nit: looks like you could wrap this at the ( above.

I'll do you one better and rewrap this and the two above.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:460: it != cache->end_keys();
On 2011/11/18 14:13:43, rsesek wrote:
> nit: align with b in base

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:464: string16 name;
On 2011/11/18 16:02:15, grt wrote:
> nit: move this out of the loop so it isn't constructed/destructed on each
> iteration.

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.h:91: static std::vector<string16>
GetProfileNames();
On 2011/11/18 07:14:25, sail wrote:
> How about just creating a new instance of ProfileInfoCache and using that
> instead?

I thought about that -- and was kind of torn! Here was my thought process: if I
create a new instance of PIC in here, that means that if someone calls this
method during normal operation (while a PIC already exists in the
ProfileManager) things may be very bad. I could add a check to see whether the
ProfileManager exists before doing this -- but this would mean an extra global
method, as right now the PM is lazily constructed. I decided to go with reading
the data out of the pref because it seemed the least bad option; and because we
never manipulate the data, but only read it in a consty fashion, we don't really
need all the functionality of the PIC, and it doesn't feel *that* evil to skip
it. If we were manipulating the data in any way, my answer would be different.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache_observer.h:13:
ProfileInfoCacheObserver() {}
On 2011/11/18 16:02:15, grt wrote:
> google style says ctor must be protected.

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache_observer.h:15: virtual void
OnProfileAdded(string16 profile_name,
On 2011/11/18 16:02:15, grt wrote:
> can these not be passed as const ref?

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache_observer.h:22: virtual
~ProfileInfoCacheObserver() {}
On 2011/11/18 16:02:15, grt wrote:
> this should be public unless you're really trying for tricky deny-destruction
> stuff.

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:217:
profile_info_cache_->RemoveObserver(profile_shortcut_manager_.get());
On 2011/11/18 14:13:43, rsesek wrote:
> The ShortcutManager could add and remove itself as an observer in its ctor and
> dtor.

Hmm -- I didn't do this because both the PIC and the PSM_win are held as
scoped_ptrs in the ProfileManager. If I manage the deletion in the PSM_win dtor,
then I have to be sure the ordering of these two instance vars is correct within
the ProfileManager header file, or the PSM_win may try to remove itself from an
already-destructed PIC. Keeping the add/delete in the PM means that I'm assured
that both objects are alive and well when I detach one from the other.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:217:
profile_info_cache_->RemoveObserver(profile_shortcut_manager_.get());
On 2011/11/18 16:02:15, grt wrote:
> On 2011/11/18 14:13:43, rsesek wrote:
> > The ShortcutManager could add and remove itself as an observer in its ctor
and
> > dtor.
> 
> If you go that route, be sure to either explicitly insert
> profile_shortcut_manager_.reset() here to force its destruction, or document
in
> the .h that the profile_shortcut_manager_ member MUST come after the
> profile_info_cache_ member so that its dtor runs first.

Yup -- this came in after I had written the above comment. :-) I think it's wise
to avoid trickery in destruction order, if possible...

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:88: } else { // Only one
profile, so create original shortcut.
On 2011/11/18 14:13:43, rsesek wrote:
> nit: two spaces before comment, or push to next line

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:99:
std::vector<string16> shortcuts;
On 2011/11/18 16:02:15, grt wrote:
> construct the vector with the one shortcut in it:
> std::vector<string16> shortcuts(1, shortcut);

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:100:
shortcuts.push_back(shortcut);
On 2011/11/18 16:02:15, grt wrote:
> remove this line.

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:140: for
(std::vector<string16>::iterator it = profile_names.begin();
On 2011/11/18 16:02:15, grt wrote:
> insert "shortcuts.reserve(profile_names.size());" above this line.

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:140: for
(std::vector<string16>::iterator it = profile_names.begin();
On 2011/11/18 16:02:15, grt wrote:
> insert "shortcuts.reserve(profile_names.size());" above this line.

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:210:
UTF8ToUTF16(shortcut_path.MaybeAsASCII()),
On 2011/11/18 16:02:15, grt wrote:
> MaybeAsASCII() is bad since shortcuts are localized.  is shortcut_path.value()
> not what you want (in place of UTF8ToUTF16(shortcut_path.MaybeAsASCII()))?

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:21:
~ProfileShortcutManagerWin();
On 2011/11/18 14:13:43, rsesek wrote:
> virtual

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:30: // Wrap a ShellUtil
function that returns a bool so it can be posted in a
On 2011/11/18 14:13:43, rsesek wrote:
> private:?

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:32: static void
CallShellUtilBoolFunction(
On 2011/11/18 14:13:43, rsesek wrote:
> This could go in the anon namespace in the .cc because it's very
> implementation-y.

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:38: std::vector<string16>
profile_names);
On 2011/11/18 14:13:43, rsesek wrote:
> I think these methods can take const-ref arguments. I believe the callback
> system will copy into InvokerStorage when it gets passed across threads, so
when
> your callback runs, a const-ref is OK to take.

That's right -- I wondered if that was so. That's convenient, but annoying in
that it hides details that we should all be thinking about carefully.
</puritan_coder>

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:651: if (appended_name.length() > 0) {
On 2011/11/18 03:58:57, robertshield wrote:
> !appended_name.empty()

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1005: for
(std::vector<std::wstring>::iterator it = appended_names.begin();
On 2011/11/18 16:02:15, grt wrote:
> const_iterator

Done.

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1007: it++) {
On 2011/11/18 03:58:57, robertshield wrote:
> opinions vary, but I find the following formatting cleaner:
> 
> std::vector<std::wstring>::iterator it(appended_names.begin());
> for (; it != appended_names.end(); ++it) {
> ...
> }
> 
> in either case, it++ -> ++it

Hmm -- I am allergic to the empty ";" at the beginning of the for loop (and this
follows the pattern set in other for loops in this file) so if you don't mind, I
think I'll leave it be -- but definitely changed to prefix.

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:272: // the base links, including appended
names (for example,
On 2011/11/18 03:58:57, robertshield wrote:
> Does this mean:
> "|appended_names| is a list of shortcut file names as obtained from
> ShellUtil::GetChromeShortcutName"? Please clarify a bit if so.

Clarification added.

http://codereview.chromium.org/8502033/diff/39001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:275: std::vector<std::wstring>
appended_names);
On 2011/11/18 14:13:43, rsesek wrote:
> const ref

Done.

[grt (UTC plus 2), 2011-11-18 20:02:54]

my drive-bys lgtm w/ the following changes.  thanks.

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:465: ++it) {
indent

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:38: L"=\"",
base::StringPrintf(L"--%ls=\"%ls\"",
ASCIIToUTF16(switches::kProfileDirectory).c_str(), profile_base_dir.c_str());

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:156: const string16&
profile_name, const string16& directory, bool create) {
break into three lines

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:180: const string16&
old_shortcut, const string16& new_shortcut) {
break into three lines

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:193: const string16&
shortcut, const string16& arguments) {
break into two lines

[robertshield, 2011-11-18 20:30:53]

lgtm

[Miranda Callahan, 2011-11-18 22:41:33]

Addressed concerns, and added a testing method to remove the ShortcutManager
from the ProfileManager for unit tests. Because removing an Observer twice is
fine, this is no problem.

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:465: ++it) {
On 2011/11/18 20:02:54, grt wrote:
> indent

Done.

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:38: L"=\"",
On 2011/11/18 20:02:54, grt wrote:
> base::StringPrintf(L"--%ls=\"%ls\"",
> ASCIIToUTF16(switches::kProfileDirectory).c_str(), profile_base_dir.c_str());

:-) Done.

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:156: const string16&
profile_name, const string16& directory, bool create) {
On 2011/11/18 20:02:54, grt wrote:
> break into three lines

Done.

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:180: const string16&
old_shortcut, const string16& new_shortcut) {
On 2011/11/18 20:02:54, grt wrote:
> break into three lines

would you settle for two? :-)

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:193: const string16&
shortcut, const string16& arguments) {
On 2011/11/18 20:02:54, grt wrote:
> break into two lines

Done.

[grt (UTC plus 2), 2011-11-21 15:21:18]

sticky LGTM w/ nits.  thanks, miranda.

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/39010/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:180: const string16&
old_shortcut, const string16& new_shortcut) {
On 2011/11/18 22:41:33, Miranda Callahan wrote:
> On 2011/11/18 20:02:54, grt wrote:
> > break into three lines
> 
> would you settle for two? :-)

uh, yeah, that's the ticket. :-)

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:152: if (!cache)
if any kind of check is needed here (i'm not sure if one is or not),
DCHECK(cache) is better than if (!cache) NOTREACHED();

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:418: if (!info)
same

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:36:
profile_base_dir.c_str());
align with previous line

[Miranda Callahan, 2011-11-21 15:40:38]

Thanks, grt! rsesek, could you take another look?

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:152: if (!cache)
On 2011/11/21 15:21:18, grt wrote:
> if any kind of check is needed here (i'm not sure if one is or not),
> DCHECK(cache) is better than if (!cache) NOTREACHED();

Yes, this was based on a discussion w/robertshield, but we then ended up
deciding the checks were already done in PrefService initialization. Removed.

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/45002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:36:
profile_base_dir.c_str());
On 2011/11/21 15:21:18, grt wrote:
> align with previous line

Done.

[Robert Sesek, 2011-11-21 16:00:37]

lgtm with nits

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:150:
ProfileInfoCache::GetProfileNames())))
nit: braces around this body

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:151: DictionaryValue* cache =
update.Get();
nit: blank line after this

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:155: DCHECK(info);
Is this DCHECK necessary? A NULL deref below would be pretty clear.

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:167:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
nit: While you're here, indent all this to 4 spaces

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.h:288: // about every profile which has
been created for this instance of Chrome,
über-nit: s/which/that

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:47: } // namespace
nit: two spaces before comments

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:36: std::vector<string16>
profile_names);
Take the vector as a const-ref

[Miranda Callahan, 2011-11-21 18:19:51]

Thank you!

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main_win.cc (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main_win.cc:150:
ProfileInfoCache::GetProfileNames())))
On 2011/11/21 16:00:37, rsesek wrote:
> nit: braces around this body

Ok; will brace the other two in this list from which I copied the original
nonbracerism as well.

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:151: DictionaryValue* cache =
update.Get();
On 2011/11/21 16:00:37, rsesek wrote:
> nit: blank line after this

Done.

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:155: DCHECK(info);
On 2011/11/21 16:00:37, rsesek wrote:
> Is this DCHECK necessary? A NULL deref below would be pretty clear.

Done.

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:167:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
On 2011/11/21 16:00:37, rsesek wrote:
> nit: While you're here, indent all this to 4 spaces

Done. Also fixing all other instances in this file.

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.h:288: // about every profile which has
been created for this instance of Chrome,
On 2011/11/21 16:00:37, rsesek wrote:
> über-nit: s/which/that

From The Chicago Manual of Style Online
(http://www.chicagomanualofstyle.org/CMS_FAQ/Whichvs.That/Whichvs.That01.html):

"Some people use “which” restrictively, which is more or less okay (and popular
among writers of British English) as long as no commas are involved:
Pianos which have a fourth pedal to mute the strings are popular among apartment
owners."

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.cc:47: } // namespace
On 2011/11/21 16:00:37, rsesek wrote:
> nit: two spaces before comments

Done.

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_win.h (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_win.h:36: std::vector<string16>
profile_names);
On 2011/11/21 16:00:37, rsesek wrote:
> Take the vector as a const-ref

Done.

[Robert Sesek, 2011-11-21 18:29:33]

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_info_cache.cc:167:
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
On 2011/11/21 18:19:52, Miranda Callahan wrote:
> On 2011/11/21 16:00:37, rsesek wrote:
> > nit: While you're here, indent all this to 4 spaces
> 
> Done. Also fixing all other instances in this file.

+1 for campsite-ing this CL.

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/8502033/diff/52002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.h:288: // about every profile which has
been created for this instance of Chrome,
On 2011/11/21 18:19:52, Miranda Callahan wrote:
> On 2011/11/21 16:00:37, rsesek wrote:
> > über-nit: s/which/that
> 
> From The Chicago Manual of Style Online
>
(http://www.chicagomanualofstyle.org/CMS_FAQ/Whichvs.That/Whichvs.That01.html%29:
> 
> "Some people use “which” restrictively, which is more or less okay (and
popular
> among writers of British English) as long as no commas are involved:
> Pianos which have a fourth pedal to mute the strings are popular among
apartment
> owners."

I _guess_ I can let it slide, if you really want to be one of those "some
people"…

[commit-bot: I haz the power, 2011-11-21 20:34:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mirandac@chromium.org/8502033/63003

[commit-bot: I haz the power, 2011-11-21 20:34:15]

Presubmit check for 8502033-63003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome/installer/util/shell_util.cc:548
    chrome/installer/util/shell_util.cc:549
    chrome/installer/util/shell_util.cc:647
    chrome/installer/util/shell_util.cc:648
    chrome/installer/util/shell_util.cc:1001
    chrome/installer/util/shell_util.cc:1005
    chrome/installer/util/shell_util.cc:1053
    chrome/installer/util/shell_util.h:112
    chrome/installer/util/shell_util.h:113
    chrome/installer/util/shell_util.h:150
    chrome/installer/util/shell_util.h:151
    chrome/installer/util/shell_util.h:275
    chrome/installer/util/shell_util.h:293

Presubmit checks took 2.9s to calculate.