Don't try to download 'NothingToDownload' placeholder avatar.

Don't try to download 'NothingToDownload' placeholder avatar.

Fix ProfileInfoCache behaviour when it tried to download the
non-existing "NothingToDownload" placeholder profile avatar from the
Web, instead of just loading it from the resources.

BUG=486854
Committed: https://crrev.com/0cb195713ef380397be9ff4d13080cdd10d0d10f
Cr-Commit-Position: refs/heads/master@{#330397}

[emaxx, 2015-05-12 11:47:14]

emaxx@chromium.org changed reviewers:
+ mlerman@chromium.org

[emaxx, 2015-05-12 11:47:20]

Hi Michael, please have a look at this review.

Apart from fixing the issue with downloading through non-existing URI, I suggest
completely removing this placeholder's file name from the constants, and
DCHECK'ing on trying to access it.

[Mike Lerman, 2015-05-12 19:59:12]

There are a few other places that need to do the necessary checks, such as the
ProfileAvatarDownloader. All will need to check the avatar_index, and handle
appropriately.

This will be much cleaner all around, which I'm looking forward to :)

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

https://codereview.chromium.org/1127113003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:349: DCHECK(index !=
kPlaceholderAvatarIcon);
this method is also called from the ProfileAvatarDownloader. This will have
impacts on that class, unless that class codes around it.

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

https://codereview.chromium.org/1127113003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:1073: const std::string file_name
=
What happens if icon_index is the non-downloading icon? This isn't handled here.

[emaxx, 2015-05-13 12:08:16]

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

https://codereview.chromium.org/1127113003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:349: DCHECK(index !=
kPlaceholderAvatarIcon);
On 2015/05/12 19:59:12, Mike Lerman wrote:
> this method is also called from the ProfileAvatarDownloader. This will have
> impacts on that class, unless that class codes around it.
Yes, that class uses this method, but the point is that the object of that class
should be never created now.

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

https://codereview.chromium.org/1127113003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:1073: const std::string file_name
=
On 2015/05/12 19:59:12, Mike Lerman wrote:
> What happens if icon_index is the non-downloading icon? This isn't handled
here.
It would DCHECK inside GetDefaultAvatarIconFileNameAtIndex().
But, anyway, the DownloadHighResAvatar() is a private function that is
referenced only by DownloadHighResAvatarIfNeed() function, whose responsibility
is exactly caring about existing files and placeholder.
Do you think that we should nevertheless handle it here too?

[Mike Lerman, 2015-05-13 20:54:00]

Yes - thanks for helping me think through this.

Just add a couple of extra elements to help future people be confident and aware
of what's going on.

LGTM. Thanks!

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

https://codereview.chromium.org/1127113003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:909: // resources, so it doesn't
need to be downloaded.
Can you append:
The avatar picture for this index will never be requested from disk by
GetHighResAvatarOfProfileAtIndex.

That makes a clear link between this condition and the check at line 1048.

https://codereview.chromium.org/1127113003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:1073: const std::string file_name
=
On 2015/05/13 12:08:16, emaxx wrote:
> On 2015/05/12 19:59:12, Mike Lerman wrote:
> > What happens if icon_index is the non-downloading icon? This isn't handled
> here.
> It would DCHECK inside GetDefaultAvatarIconFileNameAtIndex().
> But, anyway, the DownloadHighResAvatar() is a private function that is
> referenced only by DownloadHighResAvatarIfNeed() function, whose
responsibility
> is exactly caring about existing files and placeholder.
> Do you think that we should nevertheless handle it here too?

Let's add a DCHECK(!file_name.empty()), to help protect against misuse of this
method. Doesn't hurt.

[emaxx, 2015-05-18 17:06:18]

The CQ bit was checked by emaxx@chromium.org

[emaxx, 2015-05-18 17:06:19]

The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org
Link to the patchset: https://codereview.chromium.org/1127113003/#ps40001
(title: "Clarify the comments a bit.")

[commit-bot: I haz the power, 2015-05-18 17:07:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1127113003/40001

[commit-bot: I haz the power, 2015-05-18 20:23:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-18 22:41:55]

Patchset 3 (id:??) landed as
https://crrev.com/0cb195713ef380397be9ff4d13080cdd10d0d10f
Cr-Commit-Position: refs/heads/master@{#330397}