Cache GAIA profile picture URL

Cache GAIA profile picture URL

With this CL we now cache the GAIA profile picture URL to avoid downloading the picture multiple times.

BUG=91241
, http://code.google.com/p/chromium-os/issues/detail?id=23579
TEST=

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

[Ivan Korotkov, 2011-11-30 14:35:44]

LGTM with nits:

http://codereview.chromium.org/8742009/diff/1/chrome/browser/chromeos/login/u...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/8742009/diff/1/chrome/browser/chromeos/login/u...
chrome/browser/chromeos/login/user_manager.cc:844: // Currently the profile
picture URL is not cached on ChromeOS.
Please file a bug to me as soon as you submit this.

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

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader.cc:256: picture_result_ =
PICTURE_DEFAULT;
Please add VLOG here and in the next if ()

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader.cc:260: if (image_url ==
delegate_->GetCachedPictureURL()) {
Since GetCachedPictureURL() has to return something to indicate that the cached
URL is missing (presumably "") please check explicitly that !image_url_.empty()
or smth like that (so we don't end up returning PICTURE_CACHED if image_url
happens to be "" for whatever reason).

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_downloader.h (right):

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader.h:29: enum PictureResult {
[here and in other names] "picture result" sounds somewhat unsemantical. What
about smth like PictureDownloadResult or PictureStatus?

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader.h:56: // Gets the URL for the profile
picture.
Please document when (for which values of PictureResult) it makes sense to call
this one.

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_downloader_delegate.h (right):

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader_delegate.h:28: // the image will not
be downloaded.
Please document what to return when there is no cached URL (empty string?)

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader_delegate.h:29: virtual std::string
GetCachedPictureURL() = 0;
I think that this and the previous getter should be "const". For
GetBrowserProfile it probably makes sense to not be const since it returns
non-const Profile*.

http://codereview.chromium.org/8742009/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8742009/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:1473: // The URL where the GAIA profile picture was
downloaded. This is cached to
where/from which/

[sail, 2011-11-30 19:00:22]

http://codereview.chromium.org/8742009/diff/1/chrome/browser/chromeos/login/u...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/8742009/diff/1/chrome/browser/chromeos/login/u...
chrome/browser/chromeos/login/user_manager.cc:844: // Currently the profile
picture URL is not cached on ChromeOS.
On 2011/11/30 14:35:44, Ivan Korotkov wrote:
> Please file a bug to me as soon as you submit this.

Done.
Looks like nkostylev has filed bug 23579 to track this.

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

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader.cc:256: picture_result_ =
PICTURE_DEFAULT;
On 2011/11/30 14:35:44, Ivan Korotkov wrote:
> Please add VLOG here and in the next if ()

Done.

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader.cc:260: if (image_url ==
delegate_->GetCachedPictureURL()) {
On 2011/11/30 14:35:44, Ivan Korotkov wrote:
> Since GetCachedPictureURL() has to return something to indicate that the
cached
> URL is missing (presumably "") please check explicitly that
!image_url_.empty()
> or smth like that (so we don't end up returning PICTURE_CACHED if image_url
> happens to be "" for whatever reason).

Done.

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_downloader.h (right):

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader.h:29: enum PictureResult {
On 2011/11/30 14:35:44, Ivan Korotkov wrote:
> [here and in other names] "picture result" sounds somewhat unsemantical. What
> about smth like PictureDownloadResult or PictureStatus?

Done. Changed to PictureStatus

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader.h:56: // Gets the URL for the profile
picture.
On 2011/11/30 14:35:44, Ivan Korotkov wrote:
> Please document when (for which values of PictureResult) it makes sense to
call
> this one.

Done.

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_downloader_delegate.h (right):

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader_delegate.h:28: // the image will not
be downloaded.
On 2011/11/30 14:35:44, Ivan Korotkov wrote:
> Please document what to return when there is no cached URL (empty string?)

Done.

http://codereview.chromium.org/8742009/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_downloader_delegate.h:29: virtual std::string
GetCachedPictureURL() = 0;
On 2011/11/30 14:35:44, Ivan Korotkov wrote:
> I think that this and the previous getter should be "const". For
> GetBrowserProfile it probably makes sense to not be const since it returns
> non-const Profile*.

Done.

http://codereview.chromium.org/8742009/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8742009/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:1473: // The URL where the GAIA profile picture was
downloaded. This is cached to
On 2011/11/30 14:35:44, Ivan Korotkov wrote:
> where/from which/

Done.

[commit-bot: I haz the power, 2011-11-30 22:01:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8742009/4005

[commit-bot: I haz the power, 2011-11-30 23:32:42]

Change committed as 112332