Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1748423005:
Crop the user-specified profile image for WebUI (Closed)
|
Created:
4 years, 9 months ago by satorux1 Modified:
4 years, 9 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCrop the user-specified profile image for WebUI
This patch fixes a bug where the user-specified profile image
wasn't cropped for WebUI thus the image was displayed weirdly
in the lock screen and the settings.
The root cause of the problem was that the original image data
bytes (not cropped, and can be big) were used for WebUI.
Along the way, get rid of UserImage.RecodedJpegSize UMA
because it's not defined in tools/metrics/histograms/histograms.xml
BUG=590630
TEST=follow steps in the bug
Committed: https://crrev.com/5820ae933247dc363ba3ae51220acd64ba3488b0
Cr-Commit-Position: refs/heads/master@{#380512}
Patch Set 1 #Patch Set 2 : #
Total comments: 14
Patch Set 3 : address comments #Patch Set 4 : just rebase #
Total comments: 4
Patch Set 5 : just rebase #Patch Set 6 : just rebase #Patch Set 7 : address comments #
Total comments: 4
Patch Set 8 : address comments #Patch Set 9 : address comments #
Total comments: 4
Patch Set 10 : just rebase #Patch Set 11 : address comments #
Messages
Total messages: 25 (8 generated)
satorux@chromium.org changed reviewers: + hashimoto@chromium.org
https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right): https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:130: base::Bind(&user_manager::UserImage::Encode, image), Why don't you call Encode() from CropImage()? IIUC there is no need to encode the image when pixels_per_side <= target_size. https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc (right): https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:175: DCHECK(user_image.is_safe_format()); Instead of DCHECK, shouldn't we have LOG(ERROR) when this condition doesn't hold? https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:183: if (!image_bytes.size() || nit: image_bytes.empty()? https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:185: reinterpret_cast<const char*>(&image_bytes[0]), &image_bytes[0] -> image_bytes.data()? nit: I'm not sure why UserImage::Bytes' element type is unsigned char while most code in base and net uses char as the byte type. Can't we just change UserImage::Bytes' type to std::vector<char> and eliminate this reinterpret_cast? https://codereview.chromium.org/1748423005/diff/20001/components/user_manager... File components/user_manager/user_image/user_image.cc (right): https://codereview.chromium.org/1748423005/diff/20001/components/user_manager... components/user_manager/user_image/user_image.cc:22: TRACE_EVENT2("oobe", "Encode", nit: s/Encode/UserImage::Encode/ might be more helpful? https://codereview.chromium.org/1748423005/diff/20001/components/user_manager... components/user_manager/user_image/user_image.cc:33: return output; This line may enjoy NRVO or implicit move so there might be no data copy, but I'm not so sure about the current status of our codebase. Are you sure this doesn't result in copying a lot of data? https://codereview.chromium.org/1748423005/diff/20001/components/user_manager... components/user_manager/user_image/user_image.cc:42: return UserImage(image); Just UserImage()?
https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right): https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:130: base::Bind(&user_manager::UserImage::Encode, image), On 2016/03/02 07:33:58, hashimoto wrote: > Why don't you call Encode() from CropImage()? > IIUC there is no need to encode the image when pixels_per_side <= target_size. That's a great point. Fixed! https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc (right): https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:175: DCHECK(user_image.is_safe_format()); On 2016/03/02 07:33:58, hashimoto wrote: > Instead of DCHECK, shouldn't we have LOG(ERROR) when this condition doesn't > hold? Done. https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:183: if (!image_bytes.size() || On 2016/03/02 07:33:58, hashimoto wrote: > nit: image_bytes.empty()? Done. https://codereview.chromium.org/1748423005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:185: reinterpret_cast<const char*>(&image_bytes[0]), On 2016/03/02 07:33:58, hashimoto wrote: > &image_bytes[0] -> image_bytes.data()? Done > nit: I'm not sure why UserImage::Bytes' element type is unsigned char while most > code in base and net uses char as the byte type. > Can't we just change UserImage::Bytes' type to std::vector<char> and eliminate > this reinterpret_cast? I also had the same question. My guess is that it's because JPEGCodec::Encode() takes vector<unsigned char>* for the output parameter. https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/codec/jpeg_... https://codereview.chromium.org/1748423005/diff/20001/components/user_manager... File components/user_manager/user_image/user_image.cc (right): https://codereview.chromium.org/1748423005/diff/20001/components/user_manager... components/user_manager/user_image/user_image.cc:22: TRACE_EVENT2("oobe", "Encode", On 2016/03/02 07:33:58, hashimoto wrote: > nit: s/Encode/UserImage::Encode/ might be more helpful? Done. https://codereview.chromium.org/1748423005/diff/20001/components/user_manager... components/user_manager/user_image/user_image.cc:33: return output; On 2016/03/02 07:33:58, hashimoto wrote: > This line may enjoy NRVO or implicit move so there might be no data copy, but > I'm not so sure about the current status of our codebase. > Are you sure this doesn't result in copying a lot of data? Good point. Changed it to scoped_ptr<Bytes> https://codereview.chromium.org/1748423005/diff/20001/components/user_manager... components/user_manager/user_image/user_image.cc:42: return UserImage(image); On 2016/03/02 07:33:58, hashimoto wrote: > Just UserImage()? Done.
https://codereview.chromium.org/1748423005/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right): https://codereview.chromium.org/1748423005/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:133: if (!pair.second) { // Failed to encode to bytes representation. Checking pair.second looks too subtle. Could you make CropImage return bool? https://codereview.chromium.org/1748423005/diff/60001/components/user_manager... File components/user_manager/user_image/user_image.cc (right): https://codereview.chromium.org/1748423005/diff/60001/components/user_manager... components/user_manager/user_image/user_image.cc:65: const Bytes& image_bytes) Not necessarily needs to be done in this CL, but could you rewrite all code which copies image_bytes while you are here? Also, UserImage should have DISALLOW_COPY_AND_ASSIGN.
https://codereview.chromium.org/1748423005/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right): https://codereview.chromium.org/1748423005/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:133: if (!pair.second) { // Failed to encode to bytes representation. On 2016/03/04 06:39:50, hashimoto wrote: > Checking pair.second looks too subtle. > Could you make CropImage return bool? Done. https://codereview.chromium.org/1748423005/diff/60001/components/user_manager... File components/user_manager/user_image/user_image.cc (right): https://codereview.chromium.org/1748423005/diff/60001/components/user_manager... components/user_manager/user_image/user_image.cc:65: const Bytes& image_bytes) On 2016/03/04 06:39:50, hashimoto wrote: > Not necessarily needs to be done in this CL, but could you rewrite all code > which copies image_bytes while you are here? > > Also, UserImage should have DISALLOW_COPY_AND_ASSIGN. That's a good idea. Filed a bug: crbug.com/593251
https://codereview.chromium.org/1748423005/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right): https://codereview.chromium.org/1748423005/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:79: bytes->swap(*encoded.release()); This line leaks. No one will delete released std::vector.
https://codereview.chromium.org/1748423005/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right): https://codereview.chromium.org/1748423005/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:79: bytes->swap(*encoded.release()); On 2016/03/09 09:30:50, hashimoto wrote: > This line leaks. > No one will delete released std::vector. Good catch! Changed to get().
lgtm with a nit. https://codereview.chromium.org/1748423005/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right): https://codereview.chromium.org/1748423005/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:79: bytes->swap(*encoded.release()); On 2016/03/09 09:39:34, satorux1 wrote: > On 2016/03/09 09:30:50, hashimoto wrote: > > This line leaks. > > No one will delete released std::vector. > > Good catch! Changed to get(). This should be just bytes->swap(*encoded);
https://codereview.chromium.org/1748423005/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/avatar/user_image_loader.cc (right): https://codereview.chromium.org/1748423005/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_loader.cc:79: bytes->swap(*encoded.release()); On 2016/03/09 09:42:02, hashimoto wrote: > On 2016/03/09 09:39:34, satorux1 wrote: > > On 2016/03/09 09:30:50, hashimoto wrote: > > > This line leaks. > > > No one will delete released std::vector. > > > > Good catch! Changed to get(). > > This should be just bytes->swap(*encoded); My bad. Fixed!
The CQ bit was checked by satorux@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/1748423005/#ps160001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748423005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748423005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
satorux@chromium.org changed reviewers: + achuith@chromium.org
achuith@ please take a look at components/user_manager
lgtm https://codereview.chromium.org/1748423005/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc (right): https://codereview.chromium.org/1748423005/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:165: LOG(ERROR) << "User images is not in safe format"; User image is not in a safe format https://codereview.chromium.org/1748423005/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:172: reinterpret_cast<const char*>(image_bytes.data()), is reinterpret_cast still necessary? Or does unsigned char not get implicitly converted to char?
https://codereview.chromium.org/1748423005/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc (right): https://codereview.chromium.org/1748423005/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:165: LOG(ERROR) << "User images is not in safe format"; On 2016/03/10 23:02:10, achuithb wrote: > User image is not in a safe format Good catch. Done. https://codereview.chromium.org/1748423005/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:172: reinterpret_cast<const char*>(image_bytes.data()), On 2016/03/10 23:02:10, achuithb wrote: > is reinterpret_cast still necessary? Or does unsigned char not get implicitly > converted to char? It's needed. Otherwise we'll get ../../chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:172:23: error: cannot initialize a parameter of type 'const char *' with an rvalue of type 'const unsigned char *'
The CQ bit was checked by satorux@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/1748423005/#ps200001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748423005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748423005/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Crop the user-specified profile image for WebUI This patch fixes a bug where the user-specified profile image wasn't cropped for WebUI thus the image was displayed weirdly in the lock screen and the settings. The root cause of the problem was that the original image data bytes (not cropped, and can be big) were used for WebUI. Along the way, get rid of UserImage.RecodedJpegSize UMA because it's not defined in tools/metrics/histograms/histograms.xml BUG=590630 TEST=follow steps in the bug ========== to ========== Crop the user-specified profile image for WebUI This patch fixes a bug where the user-specified profile image wasn't cropped for WebUI thus the image was displayed weirdly in the lock screen and the settings. The root cause of the problem was that the original image data bytes (not cropped, and can be big) were used for WebUI. Along the way, get rid of UserImage.RecodedJpegSize UMA because it's not defined in tools/metrics/histograms/histograms.xml BUG=590630 TEST=follow steps in the bug Committed: https://crrev.com/5820ae933247dc363ba3ae51220acd64ba3488b0 Cr-Commit-Position: refs/heads/master@{#380512} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5820ae933247dc363ba3ae51220acd64ba3488b0 Cr-Commit-Position: refs/heads/master@{#380512} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
