[cros] Profile image downloader is always reset after a download.

[cros] Profile image downloader is always reset after a download.


Additionally, downloaded profile image is no more saved to file every time it is downloaded after login.

BUG=chromium-os:23684
TEST=Manual: see bug description

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

[Ivan Korotkov, 2011-12-06 16:51:24]

PTAL

[whywhat, 2011-12-06 17:51:31]

lgtm

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user.cc (right):

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.cc:43: image_index_ = image_index;
What values |image_index| can take here?

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:57: bool image_is_stub() const { return
image_is_stub_; }
optional: rename method to is_image_downloading()?

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:75: void SetStubImage(int image_index);
Document what index is.

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:95: // This is true after a |SetStubImage|
call and until a |SetImage| call.
nit: sounds kind of hacky, no? Could it be "True if we're in the process of
downloading profile picture and have set a white ghost avatar as user picture
for the time being."

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:730:
user->SetStubImage(image_index);
Why not check for empty image in User::SetImage and call SetStubImage from
there?

[Ivan Korotkov, 2011-12-08 10:52:20]

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user.cc (right):

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.cc:43: image_index_ = image_index;
On 2011/12/06 17:51:31, whywhat wrote:
> What values |image_index| can take here?

Commented.

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:57: bool image_is_stub() const { return
image_is_stub_; }
On 2011/12/06 17:51:31, whywhat wrote:
> optional: rename method to is_image_downloading()?

This isn't actually User's knowledge if image is downloading or not, just that
we've set a stub image previously.

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:75: void SetStubImage(int image_index);
On 2011/12/06 17:51:31, whywhat wrote:
> Document what index is.

Done.

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:95: // This is true after a |SetStubImage|
call and until a |SetImage| call.
On 2011/12/06 17:51:31, whywhat wrote:
> nit: sounds kind of hacky, no? Could it be "True if we're in the process of
> downloading profile picture and have set a white ghost avatar as user picture
> for the time being."

Rephrased.

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/8822015/diff/1005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:730:
user->SetStubImage(image_index);
On 2011/12/06 17:51:31, whywhat wrote:
> Why not check for empty image in User::SetImage and call SetStubImage from
> there?

I'd like the User's API to be more explicit.