Get rid of UserImageManagerImpl::Job::user_image_

Get rid of UserImageManagerImpl::Job::user_image_

There is no need to copy UserImage to the member variable.

BUG=593251
TEST=the user image stuff works as before

Committed: https://crrev.com/bc660067f6322d4ad1383336cb5afaae7d70fc91
Cr-Commit-Position: refs/heads/master@{#380935}

[satorux1, 2016-03-14 03:45:55]

Patchset #1 (id:1) has been deleted

[satorux1, 2016-03-14 03:46:02]

Patchset #1 (id:20001) has been deleted

[satorux1, 2016-03-14 03:54:39]

satorux@chromium.org changed reviewers:
+ hashimoto@chromium.org

[satorux1, 2016-03-14 03:54:40]

https://codereview.chromium.org/1778413004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc
(right):

https://codereview.chromium.org/1778413004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:545:
job_.reset(new Job(this));
BTW, the Job class looks rather awkward to me. I think we can get rid of the
class as follows:

1) replace the member variables image_(image|url|path)_ in the class to local
variables and pass them to other functions as needed
2) make some of the functions in the class free functions in anonymous namespace
3) make the rest of the functions in the class part of UserImageManagerImpl
class

If you think it's a good, I'll give it a try.

[hashimoto, 2016-03-14 05:59:16]

lgtm

https://codereview.chromium.org/1778413004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc
(right):

https://codereview.chromium.org/1778413004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:545:
job_.reset(new Job(this));
On 2016/03/14 03:54:40, satorux1 wrote:
> BTW, the Job class looks rather awkward to me. I think we can get rid of the
> class as follows:
> 
> 1) replace the member variables image_(image|url|path)_ in the class to local
> variables and pass them to other functions as needed
> 2) make some of the functions in the class free functions in anonymous
namespace
> 3) make the rest of the functions in the class part of UserImageManagerImpl
> class
> 
> If you think it's a good, I'll give it a try.

I agree the design of the class is not pretty, but you have to figure out how to
implement the same functionality served by WeakPtrFactory which allows you to
ignore the result of the previous job, by calling job_.reset().
If you think this can be done easily, then I think it might be something OK to
do.

[satorux1, 2016-03-14 06:04:25]

The CQ bit was checked by satorux@chromium.org

[commit-bot: I haz the power, 2016-03-14 06:04:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778413004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778413004/40001

[commit-bot: I haz the power, 2016-03-14 06:09:25]

Committed patchset #1 (id:40001)

[satorux1, 2016-03-14 06:10:26]

https://codereview.chromium.org/1778413004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc
(right):

https://codereview.chromium.org/1778413004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:545:
job_.reset(new Job(this));
On 2016/03/14 05:59:16, hashimoto wrote:
> On 2016/03/14 03:54:40, satorux1 wrote:
> > BTW, the Job class looks rather awkward to me. I think we can get rid of the
> > class as follows:
> > 
> > 1) replace the member variables image_(image|url|path)_ in the class to
local
> > variables and pass them to other functions as needed
> > 2) make some of the functions in the class free functions in anonymous
> namespace
> > 3) make the rest of the functions in the class part of UserImageManagerImpl
> > class
> > 
> > If you think it's a good, I'll give it a try.
> 
> I agree the design of the class is not pretty, but you have to figure out how
to
> implement the same functionality served by WeakPtrFactory which allows you to
> ignore the result of the previous job, by calling job_.reset().
> If you think this can be done easily, then I think it might be something OK to
> do.

That's a good point. I'll keep the code as is for now

[commit-bot: I haz the power, 2016-03-14 06:11:14]

Description was changed from

==========
Get rid of UserImageManagerImpl::Job::user_image_

There is no need to copy UserImage to the member variable.

BUG=593251
TEST=the user image stuff works as before
==========

to

==========
Get rid of UserImageManagerImpl::Job::user_image_

There is no need to copy UserImage to the member variable.

BUG=593251
TEST=the user image stuff works as before

Committed: https://crrev.com/bc660067f6322d4ad1383336cb5afaae7d70fc91
Cr-Commit-Position: refs/heads/master@{#380935}
==========

[commit-bot: I haz the power, 2016-03-14 06:11:16]

Patchset 1 (id:??) landed as
https://crrev.com/bc660067f6322d4ad1383336cb5afaae7d70fc91
Cr-Commit-Position: refs/heads/master@{#380935}