Profile picture doesn't display correctly after new profile has been created

Fix the profile picture doesn't display correctly after the new profile has been created.

Trigger to download the high resolution avatar immediately, after a new profile has been created.

BUG=391696
TEST=manually tested, see repro steps in the bug.

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

[Malcolm, 2014-07-09 09:36:54]

Please review this patch, thanks!

[noms (inactive), 2014-07-09 13:37:29]

Please add a TEST= line to the CL description.

https://codereview.chromium.org/380643002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/380643002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:241: if
(switches::IsNewAvatarMenu())
I think it might make more sense to move this to right above the
content::Notification line (so that we do everything we need to do before the
notification is sent out)

[Malcolm, 2014-07-10 03:54:04]

https://codereview.chromium.org/380643002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/380643002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:241: if
(switches::IsNewAvatarMenu())
Done, and add TEST=
On 2014/07/09 13:37:29, Monica Dinculescu wrote:
> I think it might make more sense to move this to right above the
> content::Notification line (so that we do everything we need to do before the
> notification is sent out)

[Malcolm, 2014-07-10 14:41:02]

Patch is updated, please review it, thanks!

[noms (inactive), 2014-07-10 14:42:42]

lgtm, thanks!

[Malcolm, 2014-07-10 14:45:25]

The CQ bit was checked by malcolm.2.wang@gmail.com

[commit-bot: I haz the power, 2014-07-10 14:47:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/malcolm.2.wang@gmail.com/380643002/40001

[commit-bot: I haz the power, 2014-07-10 16:44:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-10 17:59:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-10 17:59:47]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Malcolm, 2014-07-11 07:47:55]

Hi Monica,

The cl failed on unit_tests=ProfileInfoCacheTest.DownloadHighResAvatarTest, on
line:

EXPECT_EQ(0U, GetCache()->avatar_images_downloads_in_progress_.size());

Now, a high-res avatar will be triggered to download, during creating a new
profile. Do you have any idea or how to figure it out, it seems contradictory.

Thanks!

[noms (inactive), 2014-07-11 13:44:17]

So, to make that test still useful, it could test changing the avatar for the
profile. However, what worries me is that now all unit tests will automatically
download an avatar. I see ProfileInfoCacheTest.DownloadHighResAvatarTest has
cleanup code at the end, so I'm not sure what will happen for all the tests that
create profiles and don't have that. Can you check whether that actually
happens? Are there avatar files left over, or does the downloader not do
anything because network calls are blocked in unit tests? 

not lgtm until we figure this one out :(

On 2014/07/11 07:47:55, Malcolm wrote:
> Hi Monica,
> 
> The cl failed on unit_tests=ProfileInfoCacheTest.DownloadHighResAvatarTest, on
> line:
> 
> EXPECT_EQ(0U, GetCache()->avatar_images_downloads_in_progress_.size());
> 
> Now, a high-res avatar will be triggered to download, during creating a new
> profile. Do you have any idea or how to figure it out, it seems contradictory.
> 
> Thanks!

[noms (inactive), 2014-07-11 13:47:56]

https://codereview.chromium.org/380643002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/380643002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:236: if
(switches::IsNewAvatarMenu())
I'm sorry, I didn't see this initially because the code was hidden. Could you
move this above the OnProfileAdded notification? Basically, the notifications
should only be sent out once we've done all the work.

[Malcolm, 2014-07-11 14:18:44]

https://codereview.chromium.org/380643002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/380643002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:236: if
(switches::IsNewAvatarMenu())
You are right, this should be done before notification is sent out. Also, I have
think about a solution to make the failed test pass, just declare the method
like:
void ProfileInfoCache::AddProfileToCache(
    const base::FilePath& profile_path,
    const base::string16& name,
    const base::string16& username,
    size_t icon_index,
    const std::string& supervised_user_id,
    bool willDownloadResAvatar = true); 

then in the ProfileInfoCacheTest.DownloadHighResAvatarTest call like below:
  GetCache()->AddProfileToCache(path_1, ASCIIToUTF16("name_1"),
                                base::string16(), 0, std::string(), false);

This will not break the current code and make the test work fine. What's your
suggestion?

On 2014/07/11 13:47:56, Monica Dinculescu wrote:
> I'm sorry, I didn't see this initially because the code was hidden. Could you
> move this above the OnProfileAdded notification? Basically, the notifications
> should only be sent out once we've done all the work.

[noms (inactive), 2014-07-11 15:59:12]

https://codereview.chromium.org/380643002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/380643002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:236: if
(switches::IsNewAvatarMenu())
Default arguments are not allowed in Chromium, unfortunately. I will take a look
at the ProfileInfoCache today and see whether this call can be moved somewhere
else, or mocked out in tests.

On 2014/07/11 14:18:44, Malcolm wrote:
> You are right, this should be done before notification is sent out. Also, I
have
> think about a solution to make the failed test pass, just declare the method
> like:
> void ProfileInfoCache::AddProfileToCache(
>     const base::FilePath& profile_path,
>     const base::string16& name,
>     const base::string16& username,
>     size_t icon_index,
>     const std::string& supervised_user_id,
>     bool willDownloadResAvatar = true); 
> 
> then in the ProfileInfoCacheTest.DownloadHighResAvatarTest call like below:
>   GetCache()->AddProfileToCache(path_1, ASCIIToUTF16("name_1"),
>                                 base::string16(), 0, std::string(), false);
> 
> This will not break the current code and make the test work fine. What's your
> suggestion?
> 
> On 2014/07/11 13:47:56, Monica Dinculescu wrote:
> > I'm sorry, I didn't see this initially because the code was hidden. Could
you
> > move this above the OnProfileAdded notification? Basically, the
notifications
> > should only be sent out once we've done all the work.
>

[noms (inactive), 2014-07-15 15:31:09]

Alright. I finally got around to look at it, and everything is pretty much ok.
There is no problem adding the DownloadHighResAvatar for all the tests, because
unit tests use an URLFetcher that doesn't actually fetch anything. So that part
is still good.

This means that to fix this particular unit test you need to:
- add the kNewAvatarMenu flag to make sure you're actually testing that path.
You can do this with:
"CommandLine::ForCurrentProcess()->AppendSwitch(switches::kNewAvatarMenu);"

- change all the expectations of avatar_images_downloads_in_progress_.size() to
be 1. (Because you've started the download, but the downloader won't ever call
OnFetchComplete in the test. You can add this as a comment, if you want)

Let me know if any of this doesn't make sense :)

[Malcolm, 2014-07-17 04:10:49]

Thank you for your help, I updated the patch, also tested through
ProfileInfoCacheTest.*

[noms (inactive), 2014-07-17 14:15:25]

Thanks! LGTM

[Malcolm, 2014-07-17 14:28:02]

The CQ bit was checked by malcolm.2.wang@gmail.com

[commit-bot: I haz the power, 2014-07-17 14:29:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/malcolm.2.wang@gmail.com/380643002/60001

[commit-bot: I haz the power, 2014-07-17 18:26:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-17 20:27:48]

Change committed as 283856