Make user_manager::UserImage non-copyable

Make user_manager::UserImage non-copyable

The class contains |image_bytes_| member for JPEG data
bytes that can be very large for wallpapers, thus is
expensive to copy. This patch makes the class non-copyable
and eliminates some instances of object copies.

BUG=593251
TEST=user image and wallpaper stuff works as before

Committed: https://crrev.com/40e5476f77c26efab0a63f05e8498ec8968565de
Cr-Commit-Position: refs/heads/master@{#384848}

[satorux1, 2016-03-15 03:26:46]

Patchset #1 (id:1) has been deleted

[satorux1, 2016-03-15 07:56:52]

Patchset #1 (id:20001) has been deleted

[satorux1, 2016-03-15 07:57:07]

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

[hashimoto, 2016-03-15 08:39:28]

It's not necessary to eliminate all data copy in this change, so please feel
free to add TODO instead of fixing all of them now.

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

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:344: //
JPEG data of 512x512 pixels (usually <100KB), thus it's OK.
I don't want to call it OK to copy 100KB of data on the UI thread.

Probably it's better to change image_byte's type to base::RefCountedBytes to
eliminate the need of copy.

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:400: //
See the comment in SetToImageData() for why the copy is created.
This comment is not maintainable, as it's super easy to modify/rename/delete
SetToImageData() without revising this comment.
Please repeat the same thing here, or share the same code between these 2
places.

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1079:
result_out->reset(user_image.release());
This should be *result_out = std::move(user_image);

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/image_source.cc (right):

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/image_source.cc:42: got_data_callback.Run(new
base::RefCountedBytes(user_image->image_bytes()));
This line still copies image_bytes.

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
File components/user_manager/user.h (right):

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
components/user_manager/user.h:132: const UserImage::Bytes& image_bytes() const
{
Probably we should change this method to return scoped_ptr<Bytes>, as almost all
users of this method copies image bytes.

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
File components/user_manager/user_image/user_image.cc (right):

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
components/user_manager/user_image/user_image.cc:46: scoped_ptr<UserImage>
result(new UserImage(image, *image_bytes));
This line still copies image bytes.

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
File components/user_manager/user_image/user_image.h (right):

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
components/user_manager/user_image/user_image.h:46: UserImage(const
gfx::ImageSkia& image, const Bytes& image_bytes);
This ctor still copies image bytes.

https://codereview.chromium.org/1794323003/diff/40001/components/wallpaper/wa...
File components/wallpaper/wallpaper_manager_base.cc (right):

https://codereview.chromium.org/1794323003/diff/40001/components/wallpaper/wa...
components/wallpaper/wallpaper_manager_base.cc:913: scoped_ptr<
user_manager::UserImage> wallpaper) {
nit: "< " -> "<"

https://codereview.chromium.org/1794323003/diff/40001/components/wallpaper/wa...
components/wallpaper/wallpaper_manager_base.cc:923: scoped_ptr<gfx::ImageSkia>
deep_copy(wallpaper->image().DeepCopy());
Do we still need to deep copy the image here?
Can't we just modify the passed image which we have ownership now?

https://codereview.chromium.org/1794323003/diff/40001/components/wallpaper/wa...
components/wallpaper/wallpaper_manager_base.cc:932: base::Passed(&deep_copy),
wallpaper->image_bytes(),
This line still copies image bytes.

[satorux1, 2016-03-16 02:01:30]

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

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:344: //
JPEG data of 512x512 pixels (usually <100KB), thus it's OK.
On 2016/03/15 08:39:26, hashimoto wrote:
> I don't want to call it OK to copy 100KB of data on the UI thread.
> 
> Probably it's better to change image_byte's type to base::RefCountedBytes to
> eliminate the need of copy.

Good point. Let me revisit this in a separate patch in favor of doing less
things in a patch. Updated the comment with a TODO.

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1079:
result_out->reset(user_image.release());
On 2016/03/15 08:39:27, hashimoto wrote:
> This should be *result_out = std::move(user_image);

Done.

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
File components/user_manager/user.h (right):

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
components/user_manager/user.h:132: const UserImage::Bytes& image_bytes() const
{
On 2016/03/15 08:39:27, hashimoto wrote:
> Probably we should change this method to return scoped_ptr<Bytes>, as almost
all
> users of this method copies image bytes.

Maybe we can solve this problem by switching to base::RefCountedBytes? Let me
keep it as-is for now.

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
File components/user_manager/user_image/user_image.cc (right):

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
components/user_manager/user_image/user_image.cc:46: scoped_ptr<UserImage>
result(new UserImage(image, *image_bytes));
On 2016/03/15 08:39:27, hashimoto wrote:
> This line still copies image bytes.

Added a TODO comment.

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
File components/user_manager/user_image/user_image.h (right):

https://codereview.chromium.org/1794323003/diff/40001/components/user_manager...
components/user_manager/user_image/user_image.h:46: UserImage(const
gfx::ImageSkia& image, const Bytes& image_bytes);
On 2016/03/15 08:39:27, hashimoto wrote:
> This ctor still copies image bytes.

Added a TODO comment.

https://codereview.chromium.org/1794323003/diff/40001/components/wallpaper/wa...
File components/wallpaper/wallpaper_manager_base.cc (right):

https://codereview.chromium.org/1794323003/diff/40001/components/wallpaper/wa...
components/wallpaper/wallpaper_manager_base.cc:913: scoped_ptr<
user_manager::UserImage> wallpaper) {
On 2016/03/15 08:39:27, hashimoto wrote:
> nit: "< " -> "<"

Done.

https://codereview.chromium.org/1794323003/diff/40001/components/wallpaper/wa...
components/wallpaper/wallpaper_manager_base.cc:923: scoped_ptr<gfx::ImageSkia>
deep_copy(wallpaper->image().DeepCopy());
On 2016/03/15 08:39:27, hashimoto wrote:
> Do we still need to deep copy the image here?
> Can't we just modify the passed image which we have ownership now?

Good point. Wanted to test but this code path is hard to test. If I understand
correctly, this code is used to handle vendor-specified wallpapers downloaded
from the web via CustomizationDocument:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...


I'll spend a bit more time to figure how to test the code, but for now let me
add a TODO.

https://codereview.chromium.org/1794323003/diff/40001/components/wallpaper/wa...
components/wallpaper/wallpaper_manager_base.cc:932: base::Passed(&deep_copy),
wallpaper->image_bytes(),
On 2016/03/15 08:39:27, hashimoto wrote:
> This line still copies image bytes.

Added a TODO comment.

[hashimoto, 2016-03-17 06:06:51]

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

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:400: //
See the comment in SetToImageData() for why the copy is created.
On 2016/03/15 08:39:26, hashimoto wrote:
> This comment is not maintainable, as it's super easy to modify/rename/delete
> SetToImageData() without revising this comment.
> Please repeat the same thing here, or share the same code between these 2
> places.

ping?

[satorux1, 2016-03-17 06:56:22]

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

https://codereview.chromium.org/1794323003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:400: //
See the comment in SetToImageData() for why the copy is created.
On 2016/03/17 06:06:51, hashimoto wrote:
> On 2016/03/15 08:39:26, hashimoto wrote:
> > This comment is not maintainable, as it's super easy to modify/rename/delete
> > SetToImageData() without revising this comment.
> > Please repeat the same thing here, or share the same code between these 2
> > places.
> 
> ping?

oops. i missed this comment. introduced a function to share the code.

[hashimoto, 2016-03-17 06:58:41]

lgtm

[satorux1, 2016-03-17 06:59:39]

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

https://codereview.chromium.org/1794323003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:235: //
|image_is_safe_format|. Local state will be updated as needed.
updated the comment because the original comment was not correct documenting the
precise condition here seems not so helpful.

[satorux1, 2016-03-17 07:13:18]

satorux@chromium.org changed reviewers:
+ achuith@chromium.org, xdai@chromium.org

[satorux1, 2016-03-17 07:13:18]

+achuith: components/user_manager/...
+xdai: components/wallpaper/...

[satorux1, 2016-03-18 08:39:04]

ping achuith and xdai

[achuithb, 2016-03-18 18:57:47]

lgtm assuming you change the method name below.

https://codereview.chromium.org/1794323003/diff/100001/components/user_manage...
File components/user_manager/user.h (right):

https://codereview.chromium.org/1794323003/diff/100001/components/user_manage...
components/user_manager/user.h:227: const UserImage& user_image() const { return
*user_image_; }
The method name should be GetUserImage since it is no longer a simple getter.

[xdai1, 2016-03-18 19:41:08]

components/wallpaper lgtm

[satorux1, 2016-03-18 22:46:36]

https://codereview.chromium.org/1794323003/diff/100001/components/user_manage...
File components/user_manager/user.h (right):

https://codereview.chromium.org/1794323003/diff/100001/components/user_manage...
components/user_manager/user.h:227: const UserImage& user_image() const { return
*user_image_; }
On 2016/03/18 18:57:47, achuithb wrote:
> The method name should be GetUserImage since it is no longer a simple getter.

I think it's ok to keep the name as is because the function is still simple and
cheap:

https://google.github.io/styleguide/cppguide.html#Function_Names

Functions that are very cheap to call may instead follow the style for variable
names (all lower-case, with underscores between words).

[achuithb, 2016-03-21 19:47:27]

On 2016/03/18 22:46:36, satorux1 wrote:
>
https://codereview.chromium.org/1794323003/diff/100001/components/user_manage...
> File components/user_manager/user.h (right):
> 
>
https://codereview.chromium.org/1794323003/diff/100001/components/user_manage...
> components/user_manager/user.h:227: const UserImage& user_image() const {
return
> *user_image_; }
> On 2016/03/18 18:57:47, achuithb wrote:
> > The method name should be GetUserImage since it is no longer a simple
getter.
> 
> I think it's ok to keep the name as is because the function is still simple
and
> cheap:
> 
> https://google.github.io/styleguide/cppguide.html#Function_Names
> 
> Functions that are very cheap to call may instead follow the style for
variable
> names (all lower-case, with underscores between words).

didn't know this. lgtm

[satorux1, 2016-04-04 05:07:43]

The CQ bit was checked by satorux@chromium.org

[satorux1, 2016-04-04 05:07:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, xdai@chromium.org, hashimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/1794323003/#ps140001
(title: "just rebase")

[commit-bot: I haz the power, 2016-04-04 05:07:51]

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

[commit-bot: I haz the power, 2016-04-04 05:38:29]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-04-04 05:39:29]

Description was changed from

==========
Make user_manager::UserImage non-copyable

The class contains |image_bytes_| member for JPEG data
bytes that can be very large for wallpapers, thus is
expensive to copy. This patch makes the class non-copyable
and eliminates some instances of object copies.

BUG=593251
TEST=user image and wallpaper stuff works as before
==========

to

==========
Make user_manager::UserImage non-copyable

The class contains |image_bytes_| member for JPEG data
bytes that can be very large for wallpapers, thus is
expensive to copy. This patch makes the class non-copyable
and eliminates some instances of object copies.

BUG=593251
TEST=user image and wallpaper stuff works as before

Committed: https://crrev.com/40e5476f77c26efab0a63f05e8498ec8968565de
Cr-Commit-Position: refs/heads/master@{#384848}
==========

[commit-bot: I haz the power, 2016-04-04 05:39:31]

Patchset 6 (id:??) landed as
https://crrev.com/40e5476f77c26efab0a63f05e8498ec8968565de
Cr-Commit-Position: refs/heads/master@{#384848}