Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 861053004: [Profiles] Send out less profile avatar related notifications (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 6 months ago by noms (inactive)
Modified:
2 years, 5 months ago
CC:
chromium-reviews, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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}

Patch Set 1 : #

Total comments: 15

Patch Set 2 : fix test #

Total comments: 4

Patch Set 3 : mike comments #

Total comments: 2

Patch Set 4 : mike nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -52 lines) Patch
M chrome/browser/profiles/profile_info_cache.h View 1 2 2 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/profiles/profile_info_cache.cc View 1 2 3 11 chunks +55 lines, -39 lines 0 comments Download
M chrome/browser/profiles/profile_info_cache_observer.h View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_info_cache_unittest.cc View 1 2 chunks +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/profiles/new_avatar_button.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/ui/views/profiles/new_avatar_button.cc View 1 chunk +0 lines, -6 lines 0 comments Download
M chrome/browser/ui/webui/signin/user_manager_screen_handler.cc View 1 chunk +5 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 19 (6 generated)
noms (inactive)
Hiya Mike! I'm trying to clean up the notification mess around avatars. How does it ...
2 years, 6 months ago (2015-01-27 14:36:31 UTC) #5
Mike Lerman
Good idea - this should help quite a bit! First thoughts: https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/profile_info_cache.cc File chrome/browser/profiles/profile_info_cache.cc (right): ...
2 years, 6 months ago (2015-01-27 16:39:16 UTC) #6
noms (inactive)
https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/profile_info_cache_observer.h File chrome/browser/profiles/profile_info_cache_observer.h (right): https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/profile_info_cache_observer.h#newcode29 chrome/browser/profiles/profile_info_cache_observer.h:29: const base::FilePath& profile_path) {} On 2015/01/27 16:39:16, Mike Lerman ...
2 years, 6 months ago (2015-01-27 17:02:20 UTC) #7
noms (inactive)
https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/profile_info_cache.cc File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/profile_info_cache.cc#newcode138 chrome/browser/profiles/profile_info_cache.cc:138: void CheckIfFileExists(const base::FilePath& file_path, On 2015/01/27 16:39:15, Mike Lerman ...
2 years, 6 months ago (2015-01-28 02:18:36 UTC) #8
Mike Lerman
lgtm https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/profile_info_cache.cc File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/861053004/diff/40001/chrome/browser/profiles/profile_info_cache.cc#newcode871 chrome/browser/profiles/profile_info_cache.cc:871: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, On 2015/01/28 02:18:35, Monica Dinculescu wrote: ...
2 years, 5 months ago (2015-01-28 18:32:56 UTC) #9
noms (inactive)
Thanks Mike! Adding some owners :) + alexei: c/b/ui/cocoa + sky: c/b/ui/views
2 years, 5 months ago (2015-01-28 18:40:11 UTC) #11
noms (inactive)
https://codereview.chromium.org/861053004/diff/80001/chrome/browser/profiles/profile_info_cache.cc File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/861053004/diff/80001/chrome/browser/profiles/profile_info_cache.cc#newcode864 chrome/browser/profiles/profile_info_cache.cc:864: const base::FilePath file_path = On 2015/01/28 18:32:56, Mike Lerman ...
2 years, 5 months ago (2015-01-28 18:43:10 UTC) #12
Alexei Svitkine (slow)
lgtm
2 years, 5 months ago (2015-01-28 18:43:43 UTC) #13
sky
LGTM
2 years, 5 months ago (2015-01-28 21:19:33 UTC) #14
noms (inactive)
Thanks everyone!
2 years, 5 months ago (2015-01-28 21:19:57 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/861053004/100001
2 years, 5 months ago (2015-01-28 21:20:40 UTC) #17
commit-bot: I haz the power
Committed patchset #4 (id:100001)
2 years, 5 months ago (2015-01-28 21:45:23 UTC) #18
commit-bot: I haz the power
2 years, 5 months ago (2015-01-28 21:46:21 UTC) #19
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/28693ab216bb923b2e3dd1a849ecfa59afe75b18
Cr-Commit-Position: refs/heads/master@{#313593}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973