Add support for transparent/translucent pixels in the user image

Add support for transparent/translucent pixels in the user image

Previously, the user image was always saved in JPEG
format at /home/chronos/<email-address>.jpg even if the user
chose a PNG file with transparent/translucent pixels.
Since JPEG format does not support transparent/translucent pixels,
these pixels were replaced with black ones.

This patch solves the problem by storing the user image in PNG
format at /home/chronos/<email-address>.png, when
transparent/translucent pixels are present.

BUG=587289
TEST=transparent pixels are honored in the login and lock screens, as well as the settings page

Committed: https://crrev.com/0a1936275b756e3e36de00ecf83058553d9d7217
Cr-Commit-Position: refs/heads/master@{#436230}

[satorux1, 2016-11-29 06:03:08]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2016-11-29 06:24:56]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 06:25:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2016-11-29 06:37:42]

Patchset #1 (id:1) has been deleted

[satorux1, 2016-11-29 06:37:51]

Patchset #1 (id:20001) has been deleted

[satorux1, 2016-11-29 06:38:05]

Patchset #1 (id:30001) has been deleted

[satorux1, 2016-11-29 06:39:37]

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

[satorux1, 2016-11-29 06:39:38]

xiyuan: chrome/browser/ui/webui/settings/chromeos/change_picture_handler.cc

achuith:
everything else

[achuithb, 2016-11-29 09:04:54]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 09:05:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 09:22:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 09:22:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2016-11-30 00:04:22]

change_picture_handler.cc lgtm

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

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:169: if
(base::EndsWith(utf8_path, ".jpg", base::CompareCase::INSENSITIVE_ASCII))
nit: image_path.Extension() == FILE_PATH_LITERAL(".jpg")

[achuithb, 2016-11-30 00:26:48]

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:80: if (encoded)
Does it make sense to reset image_format to FORMAT_UNKNOWN if the encoding
failed?

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:151:
user_manager::UserImage::ImageFormat* image_format =
auto* is appropriate here

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:199:
image_info_.image_codec == ImageDecoder::ROBUST_PNG_CODEC ||
This is a change in behavior right?

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

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:500: //
Remove the old image file if the path is different. This can happen if
Should this code be browser tested?

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

https://codereview.chromium.org/2537713002/diff/50001/components/user_manager...
components/user_manager/user_image/user_image.cc:33: reinterpret_cast<unsigned
char*>(bitmap.getAddr32(0, 0)),
Does it make sense to have a temporary for this pointer?

https://codereview.chromium.org/2537713002/diff/50001/components/user_manager...
components/user_manager/user_image/user_image.cc:77: if
(SkBitmap::ComputeIsOpaque(bitmap))
Ternary operator is more compact here, but up to you:
return SkBitmap::ComputeIsOpaque(bitmap) ? FORMAT_JPG : FORMAT_PNG;

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

https://codereview.chromium.org/2537713002/diff/50001/components/user_manager...
components/user_manager/user_image/user_image.h:41: // TODO(ivankr): remove
eventually.
Do you know what this TODO is about?

[achuithb, 2016-11-30 00:31:17]

Looks like user_image_manager_browsertest.cc needs to be updated as well

[satorux1, 2016-12-01 07:39:42]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 07:39:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2016-12-01 07:42:11]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 07:42:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2016-12-01 07:43:34]

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

[satorux1, 2016-12-01 07:43:36]

+danakj for adding +ui/gfx/geometry to components/user_manager/DEPS

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right):

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:80: if (encoded)
On 2016/11/30 00:26:48, achuithb wrote:
> Does it make sense to reset image_format to FORMAT_UNKNOWN if the encoding
> failed?

No, I think. The function returns nullptr on error, and I think we don't need to
guarantee anything about |bitmap| and |image_format| on error. Updated the
function comment.

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:151:
user_manager::UserImage::ImageFormat* image_format =
On 2016/11/30 00:26:48, achuithb wrote:
> auto* is appropriate here

Done.

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:199:
image_info_.image_codec == ImageDecoder::ROBUST_PNG_CODEC ||
On 2016/11/30 00:26:48, achuithb wrote:
> This is a change in behavior right?

Yes. Added some comment to clarify the intention here.

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

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:169: if
(base::EndsWith(utf8_path, ".jpg", base::CompareCase::INSENSITIVE_ASCII))
On 2016/11/30 00:04:22, xiyuan wrote:
> nit: image_path.Extension() == FILE_PATH_LITERAL(".jpg")

Good idea. Wanted to make it case-insensitive but it's not necessary as the
image is always saved with a lower case extension by the code here.

https://codereview.chromium.org/2537713002/diff/50001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:500: //
Remove the old image file if the path is different. This can happen if
On 2016/11/30 00:26:48, achuithb wrote:
> Should this code be browser tested?

Thank you for pointing this out! I've just added a test. It was my bad that the
original patch didn't have it.

Also revised the logic here to save and delete files in one go.

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

https://codereview.chromium.org/2537713002/diff/50001/components/user_manager...
components/user_manager/user_image/user_image.cc:33: reinterpret_cast<unsigned
char*>(bitmap.getAddr32(0, 0)),
On 2016/11/30 00:26:48, achuithb wrote:
> Does it make sense to have a temporary for this pointer?

good idea. done

https://codereview.chromium.org/2537713002/diff/50001/components/user_manager...
components/user_manager/user_image/user_image.cc:77: if
(SkBitmap::ComputeIsOpaque(bitmap))
On 2016/11/30 00:26:48, achuithb wrote:
> Ternary operator is more compact here, but up to you:
> return SkBitmap::ComputeIsOpaque(bitmap) ? FORMAT_JPG : FORMAT_PNG;

Done.

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

https://codereview.chromium.org/2537713002/diff/50001/components/user_manager...
components/user_manager/user_image/user_image.h:41: // TODO(ivankr): remove
eventually.
On 2016/11/30 00:26:48, achuithb wrote:
> Do you know what this TODO is about?

I don't know. This function is used in many places so it may be difficult to
remove now.

Maybe we should remove the TODO?

[commit-bot: I haz the power, 2016-12-01 09:00:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 09:00:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2016-12-01 09:41:08]

alemate@chromium.org changed reviewers:
+ alemate@chromium.org

[Alexander Alekseev, 2016-12-01 09:41:09]

Saving files identified by user e-mail - not lgtm .

Could you use AccountId::GetAccountIdKey() for this?

[satorux1, 2016-12-01 11:42:00]

alemate: This email-based file name has been used for many years, and this patch
does not change that. If it is an issue, I think we should file a new bug and
fix it separately. What do you think?

[achuithb, 2016-12-01 20:20:32]

lgtm. Thanks for taking care of the comments.

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

https://codereview.chromium.org/2537713002/diff/50001/components/user_manager...
components/user_manager/user_image/user_image.h:41: // TODO(ivankr): remove
eventually.
On 2016/12/01 07:43:36, satorux1 wrote:
> On 2016/11/30 00:26:48, achuithb wrote:
> > Do you know what this TODO is about?
> 
> I don't know. This function is used in many places so it may be difficult to
> remove now.
> 
> Maybe we should remove the TODO?

Doesn't look like ivankr@ has been active in chromium for a while, and there's
no bug, so maybe we should remove the TODO to reduce clutter.

[danakj, 2016-12-01 21:19:47]

https://codereview.chromium.org/2537713002/diff/90001/components/user_manager...
File components/user_manager/DEPS (right):

https://codereview.chromium.org/2537713002/diff/90001/components/user_manager...
components/user_manager/DEPS:8: "+ui/gfx/geometry",
deps LGTM

[Alexander Alekseev, 2016-12-01 23:09:46]

On 2016/12/01 11:42:00, satorux1 wrote:
> alemate: This email-based file name has been used for many years, and this
patch
> does not change that. If it is an issue, I think we should file a new bug and
> fix it separately. What do you think?

Well, ok, let's postpone.
lgtm

[achuithb, 2016-12-01 23:10:24]

On 2016/12/01 23:09:46, Alexander Alekseev wrote:
> On 2016/12/01 11:42:00, satorux1 wrote:
> > alemate: This email-based file name has been used for many years, and this
> patch
> > does not change that. If it is an issue, I think we should file a new bug
and
> > fix it separately. What do you think?
> 
> Well, ok, let's postpone.
> lgtm

Satoru-san, could you please file a bug for this?

[achuithb, 2016-12-01 23:12:17]

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

https://codereview.chromium.org/2537713002/diff/90001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:515:
prefs_images->GetDictionaryWithoutPathExpansion(user_id(),
Please add a TODO to replace this with GetAccountIdKey() here, referencing the
bug.

[Alexander Alekseev, 2016-12-01 23:16:25]

On 2016/12/01 23:10:24, achuithb wrote:
> On 2016/12/01 23:09:46, Alexander Alekseev wrote:
> > On 2016/12/01 11:42:00, satorux1 wrote:
> > > alemate: This email-based file name has been used for many years, and this
> > patch
> > > does not change that. If it is an issue, I think we should file a new bug
> and
> > > fix it separately. What do you think?
> > 
> > Well, ok, let's postpone.
> > lgtm
> 
> Satoru-san, could you please file a bug for this?

Yes. You can assign it to me, as there is more work to do during migration.
Thank you!

[satorux1, 2016-12-02 04:59:36]

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

https://codereview.chromium.org/2537713002/diff/90001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:156: bool
should_remove_old_image) {
removed should_remove_old_image parameter to simplify the code.

https://codereview.chromium.org/2537713002/diff/90001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:515:
prefs_images->GetDictionaryWithoutPathExpansion(user_id(),
On 2016/12/01 23:12:16, achuithb wrote:
> Please add a TODO to replace this with GetAccountIdKey() here, referencing the
> bug.

Filed a bug and added a TODO comment in SaveImageAndUpdateLocalState() where the
file name is generated using user_id().

[Alexander Alekseev, 2016-12-02 05:00:53]

lgtm

[satorux1, 2016-12-02 06:25:43]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 06:26:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2016-12-02 06:42:58]

achuith: please have another look at user_image_manager_impl.cc. I made some
minor changes.

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

https://codereview.chromium.org/2537713002/diff/130001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:483:
return;
Added this to fix the following NOTREACHED() failure in
ChooseExtensionFromImageFormat():

[17019:17019:1201/005029.191350:FATAL:user_image_manager_impl.cc(198)] Check
failed: false. Invalid format: 2

This happened because the image format is unknown if a stub profile image is
used. This code path is exercised in the following two tests:

UserImageManagerTest.ProfileImageDownloadDoesNotClobber
UserImageManagerTest.SaveUserImageFromProfileImage

https://codereview.chromium.org/2537713002/diff/130001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:507:
user_id() + ChooseExtensionFromImageFormat(image_format));
Moved this code here, as there is no need to generate the image path earlier.

[achuithb, 2016-12-02 08:28:18]

Thanks, and great work! lgtm

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

https://codereview.chromium.org/2537713002/diff/130001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:480: //
image hasn't been downloaded yet)
nit: period

https://codereview.chromium.org/2537713002/diff/130001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:483:
return;
On 2016/12/02 06:42:58, satorux1 wrote:
> Added this to fix the following NOTREACHED() failure in
> ChooseExtensionFromImageFormat():
> 
> [17019:17019:1201/005029.191350:FATAL:user_image_manager_impl.cc(198)] Check
> failed: false. Invalid format: 2
> 
> This happened because the image format is unknown if a stub profile image is
> used. This code path is exercised in the following two tests:
> 
> UserImageManagerTest.ProfileImageDownloadDoesNotClobber
> UserImageManagerTest.SaveUserImageFromProfileImage

Acknowledged.

https://codereview.chromium.org/2537713002/diff/130001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:507:
user_id() + ChooseExtensionFromImageFormat(image_format));
On 2016/12/02 06:42:58, satorux1 wrote:
> Moved this code here, as there is no need to generate the image path earlier.

Acknowledged.

[commit-bot: I haz the power, 2016-12-02 08:30:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 08:30:47]

Dry run: This issue passed the CQ dry run.

[satorux1, 2016-12-05 01:53:44]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 01:53:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2016-12-05 01:57:40]

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

https://codereview.chromium.org/2537713002/diff/130001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:480: //
image hasn't been downloaded yet)
On 2016/12/02 08:28:18, achuithb wrote:
> nit: period

Done.

[commit-bot: I haz the power, 2016-12-05 03:36:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 03:36:11]

Dry run: This issue passed the CQ dry run.

[satorux1, 2016-12-05 04:10:35]

The CQ bit was checked by satorux@chromium.org

[satorux1, 2016-12-05 04:10:36]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
danakj@chromium.org, alemate@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2537713002/#ps170001
(title: "rebased")

[commit-bot: I haz the power, 2016-12-05 04:10:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 04:15:59]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1480911035537650,
"parent_rev": "10baa789d60e9a4f9c078faaa9154c4724041533", "commit_rev":
"8457cc32645354c6e91e1aaa22a7ff4bf7ba5ad0"}

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

Committed patchset #7 (id:170001)

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

Description was changed from

==========
Add support for transparent/translucent pixels in the user image

Previously, the user image was always saved in JPEG
format at /home/chronos/<email-address>.jpg even if the user
chose a PNG file with transparent/translucent pixels.
Since JPEG format does not support transparent/translucent pixels,
these pixels were replaced with black ones.

This patch solves the problem by storing the user image in PNG
format at /home/chronos/<email-address>.png, when
transparent/translucent pixels are present.

BUG=587289
TEST=transparent pixels are honored in the login and lock screens, as well as
the settings page
==========

to

==========
Add support for transparent/translucent pixels in the user image

Previously, the user image was always saved in JPEG
format at /home/chronos/<email-address>.jpg even if the user
chose a PNG file with transparent/translucent pixels.
Since JPEG format does not support transparent/translucent pixels,
these pixels were replaced with black ones.

This patch solves the problem by storing the user image in PNG
format at /home/chronos/<email-address>.png, when
transparent/translucent pixels are present.

BUG=587289
TEST=transparent pixels are honored in the login and lock screens, as well as
the settings page

Committed: https://crrev.com/0a1936275b756e3e36de00ecf83058553d9d7217
Cr-Commit-Position: refs/heads/master@{#436230}
==========

[commit-bot: I haz the power, 2016-12-05 04:19:41]

Patchset 7 (id:??) landed as
https://crrev.com/0a1936275b756e3e36de00ecf83058553d9d7217
Cr-Commit-Position: refs/heads/master@{#436230}