[Profiles] Send out less profile avatar related notifications

[Profiles] Send out less profile avatar related notifications

With the new profiles UI and the high res avatar downloading code, we are sending out
waaaay too many OnProfileAvatarChanged notifications. This is particularly terrible on
Windows, where basically on every Chrome startup we refresh all the icons.

Most importantly, we need to stop downloading the avatar files every time profiles get
loaded in the ProfileInfoCache. We did this originally because the avatar files were pretty
volatile, but it's safe to assume they've stopped changing.

Secondly, the only things that care about the high res avatar files are the UserManager
and the profile switcher, so they should get a special notification that this particular
file is ready, rather that annoy all the other listeners who only care about the tiny avatars.

BUG=449569
Committed: https://crrev.com/28693ab216bb923b2e3dd1a849ecfa59afe75b18
Cr-Commit-Position: refs/heads/master@{#313593}

[noms (inactive), 2015-01-26 23:48:16]

Patchset #1 (id:1) has been deleted

[noms (inactive), 2015-01-26 23:51:35]

Patchset #1 (id:20001) has been deleted

[noms (inactive), 2015-01-26 23:51:47]

Patchset #1 (id:20001) has been deleted

[noms (inactive), 2015-01-27 14:36:31]

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

[noms (inactive), 2015-01-27 14:36:31]

Hiya Mike!

I'm trying to clean up the notification mess around avatars. How does it look?

[Mike Lerman, 2015-01-27 16:39:16]

Good idea - this should help quite a bit! First thoughts:

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

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:138: void CheckIfFileExists(const
base::FilePath& file_path,
Not a huge fan of the name (sorry!). I feel like a method named "Check..."
should always return a boolean. This is "ExecuteCallbackIfFileExists", really.

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:140: if
(!base::PathExists(file_path))
Do a CHECK that you're on FILE thread.

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:871:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
Rather that posting to FILE, consider using a BlockingPool:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:1014: const std::string file_name,
For simplicity, and to have this signature match DownloadHighResAvatarIfNeeded,
why not remove |file_name| and have this method call
GetDefaultAvatarIconFileNameAtIndex()?

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

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.h:189: void
DownloadHighResAvatar(const std::string file_name,
nit: mention |file_name| in the comment, for completeness.

Also, file_name should be const std::string&, not merely const std::string.
(Note: &)

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache_observer.h:29: const base::FilePath&
profile_path) {}
Should the ProfileChooserView/Controller also listed to this? Just as the
UserManager does? Especially if we're going to make it Modal.

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (left):

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:155: if
(browser_->profile()->GetPath() == profile_path)
Why remove this? Don't you want to do the same as on Mac... only execute this if
!IsNewAvatarMenu()?

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

https://codereview.chromium.org/861053004/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:863: const base::FilePath
file_path =
To Simplify the ProfileInfoCache, should all this logic be in the
ProfileAvatarDownloader? It could have a method called "StartIfNecessary" and
then all the logic goes in there.

https://codereview.chromium.org/861053004/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:887: LOG(ERROR) << "Failed to PNG
encode the image.";
In this case we don't delete the ProfileAvatarDownloader. We should! In fact
there, are several flows from creating the ProfileAvatarDownloader that don't
lead to OnAvatarPictureSaved, which all mean the ProfileAvatarDownloader sits in
memory until the end of Chrome.

[noms (inactive), 2015-01-27 17:02:20]

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_cache_observer.h (right):

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache_observer.h:29: const base::FilePath&
profile_path) {}
On 2015/01/27 16:39:16, Mike Lerman wrote:
> Should the ProfileChooserView/Controller also listed to this? Just as the
> UserManager does? Especially if we're going to make it Modal.

The ProfileChooserView/Controller listen to OnAvatarMenu changes, which in turn
listen to the old style notifications. I agree one or both should change, but
maybe not quite in this CL :)

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (left):

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:155: if
(browser_->profile()->GetPath() == profile_path)
On 2015/01/27 16:39:16, Mike Lerman wrote:
> Why remove this? Don't you want to do the same as on Mac... only execute this
if
> !IsNewAvatarMenu()?

NewAvatarButton only exists for the new profiles UI (the other one is
AvatarMenuButton), so it doesn't care about avatar changes :)

[noms (inactive), 2015-01-28 02:18:36]

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

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:138: void CheckIfFileExists(const
base::FilePath& file_path,
On 2015/01/27 16:39:15, Mike Lerman wrote:
> Not a huge fan of the name (sorry!). I feel like a method named "Check..."
> should always return a boolean. This is "ExecuteCallbackIfFileExists", really.

Done.

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:140: if
(!base::PathExists(file_path))
On 2015/01/27 16:39:15, Mike Lerman wrote:
> Do a CHECK that you're on FILE thread.

Done.

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:871:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
On 2015/01/27 16:39:15, Mike Lerman wrote:
> Rather that posting to FILE, consider using a BlockingPool:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

I'd rather have all the posts in this file look the same (so all posts, or all
BlockingPool), and I'd rather not change them now. So I'm going to punt this
one, if that's ok :)

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:1014: const std::string file_name,
On 2015/01/27 16:39:15, Mike Lerman wrote:
> For simplicity, and to have this signature match
DownloadHighResAvatarIfNeeded,
> why not remove |file_name| and have this method call
> GetDefaultAvatarIconFileNameAtIndex()?
Done.

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

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.h:189: void
DownloadHighResAvatar(const std::string file_name,
On 2015/01/27 16:39:15, Mike Lerman wrote:
> nit: mention |file_name| in the comment, for completeness.
> 
> Also, file_name should be const std::string&, not merely const std::string.
> (Note: &)

Done.

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

https://codereview.chromium.org/861053004/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:863: const base::FilePath
file_path =
On 2015/01/27 16:39:16, Mike Lerman wrote:
> To Simplify the ProfileInfoCache, should all this logic be in the
> ProfileAvatarDownloader? It could have a method called "StartIfNecessary" and
> then all the logic goes in there.

Hmm, I don't think so. That would mean we'd create a ProfileAvatarDownloader
every time we look at an avatar and wonder if we should download it (i.e. every
time Chrome starts), which is a little much.

https://codereview.chromium.org/861053004/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:887: LOG(ERROR) << "Failed to PNG
encode the image.";
On 2015/01/27 16:39:16, Mike Lerman wrote:
> In this case we don't delete the ProfileAvatarDownloader. We should! In fact
> there, are several flows from creating the ProfileAvatarDownloader that don't
> lead to OnAvatarPictureSaved, which all mean the ProfileAvatarDownloader sits
in
> memory until the end of Chrome.

I think it makes more sense to delete the downloader before the file is actually
saved (it's not like the download is restarted) so now the dangling pointers
should be taken care of.

[Mike Lerman, 2015-01-28 18:32:56]

lgtm

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

https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:871:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
On 2015/01/28 02:18:35, Monica Dinculescu wrote:
> On 2015/01/27 16:39:15, Mike Lerman wrote:
> > Rather that posting to FILE, consider using a BlockingPool:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...
> 
> I'd rather have all the posts in this file look the same (so all posts, or all
> BlockingPool), and I'd rather not change them now. So I'm going to punt this
> one, if that's ok :)

that's ok :)

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

https://codereview.chromium.org/861053004/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:864: const base::FilePath
file_path =
Reference missing (&)

[noms (inactive), 2015-01-28 18:40:11]

noms@chromium.org changed reviewers:
+ asvitkine@chromium.org, sky@chromium.org

[noms (inactive), 2015-01-28 18:40:11]

Thanks Mike!

Adding some owners :)

+ alexei: c/b/ui/cocoa
+ sky: c/b/ui/views

[noms (inactive), 2015-01-28 18:43:10]

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

https://codereview.chromium.org/861053004/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_cache.cc:864: const base::FilePath
file_path =
On 2015/01/28 18:32:56, Mike Lerman wrote:
> Reference missing (&)

Done.

[Alexei Svitkine (slow), 2015-01-28 18:43:43]

lgtm

[sky, 2015-01-28 21:19:33]

LGTM

[noms (inactive), 2015-01-28 21:19:57]

Thanks everyone!

[noms (inactive), 2015-01-28 21:20:00]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-01-28 21:20:40]

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

[commit-bot: I haz the power, 2015-01-28 21:45:23]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-01-28 21:46:21]

Patchset 4 (id:??) landed as
https://crrev.com/28693ab216bb923b2e3dd1a849ecfa59afe75b18
Cr-Commit-Position: refs/heads/master@{#313593}