[cros] Encode user images when set from WebUI (regression fix).

[cros] Encode user images when set from WebUI (regression fix).

This only fixes the avatar-related part of the bug.
UserImageLoader now always passes raw data to the callback.


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

[Ivan Korotkov, 2012-08-01 01:59:01]

PTAL
nkostylev: whole CL
jhawkins: options2
phajdan.jr: automation

[Nikita (slow), 2012-08-01 03:56:18]

lgtm

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image.cc (right):

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.cc:27: UserImage
UserImage::CreateAndEncode(const gfx::ImageSkia& image) {
const UserImage&

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image.h (right):

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:25: static UserImage
CreateAndEncode(const gfx::ImageSkia& image);
const UserImage&

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:30: // creates a new instance from a
given still frame without any raw data.
nit: capitalize

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image_loader.cc (right):

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image_loader.cc:96: UserImage
user_image(final_image, decoder->get_image_data());
Move ctor call to Bind?

[Ivan Korotkov, 2012-08-01 21:51:28]

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image.cc (right):

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.cc:27: UserImage
UserImage::CreateAndEncode(const gfx::ImageSkia& image) {
On 2012/08/01 03:56:18, Nikita Kostylev wrote:
> const UserImage&

Uh, you can't return a const reference to a stack-allocated object. This one is
subject to return value optimization, though. Also, I'm going to make RawImage
be RefCountedMemory, anyway.

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image.h (right):

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image.h:30: // creates a new instance from a
given still frame without any raw data.
On 2012/08/01 03:56:18, Nikita Kostylev wrote:
> nit: capitalize

Done.

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_image_loader.cc (right):

http://codereview.chromium.org/10830111/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_image_loader.cc:96: UserImage
user_image(final_image, decoder->get_image_data());
On 2012/08/01 03:56:18, Nikita Kostylev wrote:
> Move ctor call to Bind?

Done.

[James Hawkins, 2012-08-02 20:54:22]

options2 LGTM

[Ivan Korotkov, 2012-08-02 21:45:24]

Darin, could you take a look at automation/?

[darin (slow to review), 2012-08-02 22:15:30]

LGTM for automation changes.

http://codereview.chromium.org/10830111/diff/8001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_image.h (right):

http://codereview.chromium.org/10830111/diff/8001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_image.h:25: static UserImage
CreateAndEncode(const gfx::ImageSkia& image);
maybe it would be helpful to invent factory style methods and make the
constructors private?  it is confusing to have two different ways of
constructing a class.

one downside of doing that however is that the copy-by-value is expensive.  you
could use RefCountedMemory as you say above or you could just wrap the entire
guts on this class with a RefCounted inner class.

or you could always heap allocate the class and return scoped_ptr<UserImage>.

[Ivan Korotkov, 2012-08-02 22:18:48]

Right, I'm intending to make UserImage::RawImage to be ref-counted instead of a
simple std::vector because right now it's copied in a plenty of places.

As for the factory method, it will no longer be needed and can be cleaned up
after we remove the "old" user images UI.

[commit-bot: I haz the power, 2012-08-02 22:19:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/10830111/8001

[commit-bot: I haz the power, 2012-08-02 23:46:08]

Change committed as 149744

[darin (slow to review), 2012-08-03 03:40:18]

On Thu, Aug 2, 2012 at 3:18 PM, <ivankr@chromium.org> wrote:

> Right, I'm intending to make UserImage::RawImage to be ref-counted instead
> of a
> simple std::vector because right now it's copied in a plenty of places.
>
> As for the factory method, it will no longer be needed and can be cleaned
> up
> after we remove the "old" user images UI.
>

Please add a TODO for this.

Thanks,
-Darin



>
>
http://codereview.chromium.**org/10830111/<http://codereview.chromium.org/108...
>

[Ivan Korotkov, 2012-08-03 16:40:27]

OK