Ensure that use of GAIA names are considered when handling desktop shortcut changes.

Ensure that use of GAIA names are considered when handling desktop shortcut changes. Beefed up ProfileInfoCacheTest with a verifier class to ensure this.

BUG=106261
TEST=On Windows, create a new user and sign him into sync with a GAIA account. Ensure that the desktop shortcut changes with the synced GAIA name. Ensure that changing the profile's name or avatar updates the shortcut. Ensure that deleting the profile deletes the shortcut.

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

[SteveT, 2011-12-09 16:54:00]

Kind of a work in progress still, but here is the patch so far. PTAL.

Basically we were forgetting to use the GAIA name for Notifications if the GAIA
name was in fact being used. I'm actually not a big fan of having to do all
these checks, and wish there was a universal way of just saying "give me the
profile name used today. I don't care if it's the GAIA name or set by the user."
but it seems like there is a good reason why we avoid that.

I have one more thing to fix (so expect another addition to this patch). Right
now, when the user signs in, the setting of the GAIA avatar and the GAIA name
seem to create a semi-busted shortcut file on the desktop. It doesn't really
work if you double click it, and Win7 shows that it has some sort of
"compatibility" issue. If you refresh the desktop, however, the shortcut is
there and works fine.

[sail, 2011-12-09 17:47:59]

http://codereview.chromium.org/8894005/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8894005/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:240: if
(IsUsingGAIANameOfProfileAtIndex(index))
In all these places you should be able to just call GetNameOfProfileAtIndex()
which will do the check for you.

[SteveT, 2011-12-09 18:20:20]

Thanks! Back to you.

http://codereview.chromium.org/8894005/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8894005/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_info_cache.cc:240: if
(IsUsingGAIANameOfProfileAtIndex(index))
Just what I wanted. Done here and below where appropriate.

[sail, 2011-12-09 18:24:22]

hey steve, can chance we could add a quick 3 link unit test for this?

http://codereview.chromium.org/8894005/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8894005/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:232: DictionaryPrefUpdate
update(prefs_, prefs::kProfileInfoCache);
can the next three lines be moved after the FOR_EACH_OBSERVER call?

http://codereview.chromium.org/8894005/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:236: DictionaryValue* info = NULL;
do you still need this?

[SteveT, 2011-12-09 18:37:56]

Hey Sail - addressed your comments.

I'd be more than happy to write some unit tests for this, but what did you mean
by "3 link unit tests"?


Steve

http://codereview.chromium.org/8894005/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_info_cache.cc (right):

http://codereview.chromium.org/8894005/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:232: DictionaryPrefUpdate
update(prefs_, prefs::kProfileInfoCache);
Rearranged the code here to be grouped more sensibly, and removed unnecessary
bits.

http://codereview.chromium.org/8894005/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_info_cache.cc:236: DictionaryValue* info = NULL;
No, nor do I need |index| below. That was a bit careless of me. Removed the
unnecessary stuff.

[sail, 2011-12-09 18:41:28]

> I'd be more than happy to write some unit tests for this, but what did you
mean
> by "3 link unit tests"?

"3 line unit test".
Argh, need some coffee.

[SteveT, 2011-12-09 19:50:10]

I see your request for a 3 line unit test and I'll raise you a helper class that
verifies this fix across all ProfileInfoCacheTests.

At first I thought you wanted a UT that checked that GetNameOfProfileAtIndex
returns the right thing given different GAIA Name states... but then I realized
you had tested that already in ProfileInfoCacheTest.GAIAName. So instead I came
up with this to ensure we're never passing bogus names to
ProfileInfoCacheObservers... which was what this patch fixes.

Let me know if this sounds good or if I've misinterpreted :)

[SteveT, 2011-12-09 19:58:31]

Oh, let me know if you prefer the definition of the helper class in the .cc
rather than the header.

[sail, 2011-12-09 20:48:46]

thanks for adding the test! LGTM!

[SteveT, 2011-12-09 21:36:32]

Cool, thanks. Again, I need to work out that bug with the glitchy .lnk file
before I commit this. Plus it seems like the trybots want me to separate the
definition from the header, so I'll do that too.

[SteveT, 2011-12-13 16:19:06]

Sail - PTAL at gaia_info_update_service.cc.

I figured out the issue with the corrupted shortcut. Turns out matter orders
when you rename and update the badge on the desktop shortcut
(ProfileShortcutManagerWin::OnProfileAdded came across the same issue when it
tries to call RenameChromeDesktopShortcutForProfile and
UpdateChromeDesktopShortcutForProfile).

Let me know what you think of this change.

[sail, 2011-12-13 19:36:18]

lgtm

http://codereview.chromium.org/8894005/diff/6003/chrome/browser/profiles/gaia...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

http://codereview.chromium.org/8894005/diff/6003/chrome/browser/profiles/gaia...
chrome/browser/profiles/gaia_info_update_service.cc:134: // Order matters here
like in ProfileShortcutManagerWin::OnProfileAdded. We
Can you explain in the comment why order matters? It sounds like a bug.

[SteveT, 2011-12-13 20:20:47]

Thanks. Committing once tests and stuff look good. ONWARDS!

http://codereview.chromium.org/8894005/diff/6003/chrome/browser/profiles/gaia...
File chrome/browser/profiles/gaia_info_update_service.cc (right):

http://codereview.chromium.org/8894005/diff/6003/chrome/browser/profiles/gaia...
chrome/browser/profiles/gaia_info_update_service.cc:134: // Order matters here
like in ProfileShortcutManagerWin::OnProfileAdded. We
Did my best to beef up the comments. Unfortunately it seems to be caused by some
race at the Windows level (when I debug it, the issue does not appear - I
suppose the windows-level file writing ops get enough time to finish what
they're doing).

[commit-bot: I haz the power, 2011-12-13 20:25:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/8894005/25001

[commit-bot: I haz the power, 2011-12-13 22:07:17]

Change committed as 114282