Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2061183002:
arc: IPC method to set custom wallpaper. (Closed)
|
Created:
4 years, 6 months ago by Shuhei Takahashi Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, darin (slow to review), davemoore+watch_chromium.org, droger+watchlist_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, qsr+mojo_chromium.org, sdefresne+watchlist_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: IPC method to set custom wallpaper.
A new IPC method SetWallpaper() is added to allow apps to
set custom wallpaper.
Notes:
- The IPC method accepts JPEG data only for now because
Chrome OS Wallpaper Manager re-encodes any custom wallpaper
image to JPEG anyway.
- New wallpaper image does not show up in Chrome OS wallpaper
picker, just similarly as when the wallpaper is set by
enterprise policy. This is good enough for first version
and it is a TODO to register the new wallpaper to picker.
- STRETCH layout is always used to render the wallpaper for
now. We need UI discussion when we allow the user to select
the layout later.
BUG=chromium:618922
TEST=Manually tested with Android change that SetWallpaper()
IPC call changes the wallpaper successfully.
Committed: https://crrev.com/fc1bf374333bb1d9bd0d64971e64eb6f3e604292
Cr-Commit-Position: refs/heads/master@{#400399}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Review ready. #
Total comments: 8
Patch Set 4 : Address hidehiko's comment. #
Total comments: 4
Patch Set 5 : Address hidehiko's comment. #Patch Set 6 : Add next IDs. #
Total comments: 12
Patch Set 7 : Address comments. #Patch Set 8 : Tiny style fix. #
Total comments: 10
Patch Set 9 : Address yusukes' comments. #
Total comments: 12
Patch Set 10 : Addressed hidehiko's comment. #Patch Set 11 : Small amend. #Patch Set 12 : Small amend. #
Total comments: 10
Patch Set 13 : Rebased. #Patch Set 14 : Address hidehiko's comments. #
Total comments: 2
Patch Set 15 : Remove redundant crot. #Patch Set 16 : Style fix. #
Total comments: 2
Messages
Total messages: 43 (13 generated)
Description was changed from
==========
WIP: arc: Set custom wallpaper.
*** WIP ***
BUG=chromium:618922
TEST=TBD
==========
to
==========
arc: Allow apps to set custom wallpaper.
A new IPC method SetWallpaper() is added to allow apps to
set custom wallpaper.
The IPC method accepts JPEG data only for now because
Chrome OS Wallpaper Manager eventually encodes any custom
wallpaper image into JPEG anyway.
BUG=chromium:618922
TEST=Manually tested with Android change that SetWallpaper()
IPC call changes the wallpaper successfully.
==========
Description was changed from
==========
arc: Allow apps to set custom wallpaper.
A new IPC method SetWallpaper() is added to allow apps to
set custom wallpaper.
The IPC method accepts JPEG data only for now because
Chrome OS Wallpaper Manager eventually encodes any custom
wallpaper image into JPEG anyway.
BUG=chromium:618922
TEST=Manually tested with Android change that SetWallpaper()
IPC call changes the wallpaper successfully.
==========
to
==========
arc: IPC method to set custom wallpaper.
A new IPC method SetWallpaper() is added to allow apps to
set custom wallpaper.
Notes:
- The IPC method accepts JPEG data only for now because
Chrome OS Wallpaper Manager re-encodes any custom wallpaper
image to JPEG anyway.
- New wallpaper image does not show up in Chrome OS wallpaper
picker, just similarly as when the wallpaper is set by
enterprise policy. This is good enough for first version
and it is a TODO to register the new wallpaper to picker.
- STRETCH layout is always used to render the wallpaper for
now. We need UI discussion when we allow the user to select
the layout later.
BUG=chromium:618922
TEST=Manually tested with Android change that SetWallpaper()
IPC call changes the wallpaper successfully.
==========
nya@chromium.org changed reviewers: + dcheng@chromium.org, hidehiko@chromium.org, kinuko@chromium.org, xdai@chromium.org
hidehiko: PTAL for all xdai: PTAL for arc_wallpaper_handler.cc which calls WallpaperManager. dcheng: PTAL for .mojom changes kinuko: PTAL for DEPS to content/public
FYI, WIP corresponding changes are ag/1135701, ag/1135563.
Quick walk through to shorten the total review period. Will take a look in details. https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.cc:35: content::BrowserThread::GetBlockingPool(), Why not sharing the pool with arc_service_manager? https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:35: // TODO: Allow specifying layout. TODO(nya) or TODO(bug-id) https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.h (right): https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.h:15: ArcWallpaperHandler() {} "= default" ? https://codereview.chromium.org/2061183002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2061183002/diff/40001/components/arc/intent_h... components/arc/intent_helper/arc_intent_helper_bridge.cc:58: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Could you use ThreadChecker instead? Ditto for below.
nya@chromium.org changed reviewers: - kinuko@chromium.org
hidehiko, thanks for quick review! BTW, I removed dependency to //content/public so approval for DEPS is not required now. Sorry kinuko@ for spamming :( https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.cc:35: content::BrowserThread::GetBlockingPool(), On 2016/06/14 13:44:16, hidehiko wrote: > Why not sharing the pool with arc_service_manager? It is a shared pool so every call to GetBlockingPool() will return the same instance: https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?s... https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:35: // TODO: Allow specifying layout. On 2016/06/14 13:44:16, hidehiko wrote: > TODO(nya) or TODO(bug-id) Done. https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.h (right): https://codereview.chromium.org/2061183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.h:15: ArcWallpaperHandler() {} On 2016/06/14 13:44:16, hidehiko wrote: > "= default" ? Done. https://codereview.chromium.org/2061183002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2061183002/diff/40001/components/arc/intent_h... components/arc/intent_helper/arc_intent_helper_bridge.cc:58: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2016/06/14 13:44:16, hidehiko wrote: > Could you use ThreadChecker instead? Ditto for below. Done.
components/arc LGTM. Defer to other reviewers.
Oops, two small things overlooked. https://codereview.chromium.org/2061183002/diff/60001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2061183002/diff/60001/components/arc/BUILD.gn... components/arc/BUILD.gn:10: "arc_bridge_bootstrap.cc", Please add set_wallpaper_delegate.h here. Ditto for components/arc.gypi. https://codereview.chromium.org/2061183002/diff/60001/components/arc/BUILD.gn... components/arc/BUILD.gn:75: "//content/public/browser", No longer needed?
https://codereview.chromium.org/2061183002/diff/60001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2061183002/diff/60001/components/arc/BUILD.gn... components/arc/BUILD.gn:10: "arc_bridge_bootstrap.cc", On 2016/06/15 09:04:16, hidehiko wrote: > Please add set_wallpaper_delegate.h here. > Ditto for components/arc.gypi. Oops, thanks for catching. https://codereview.chromium.org/2061183002/diff/60001/components/arc/BUILD.gn... components/arc/BUILD.gn:75: "//content/public/browser", On 2016/06/15 09:04:16, hidehiko wrote: > No longer needed? Done.
hidehiko: PTAL for additional mojom comment xdai: PTAL for arc_wallpaper_handler.cc which calls WallpaperManager dcheng: PTAL for .mojom changes I've added next ID comments to intent_helper.mojom to avoid silent conflict with crrev.com/2064263002.
yusukes@chromium.org changed reviewers: + yusukes@chromium.org
drive-by https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:30: std::unique_ptr<SkBitmap> bitmap = gfx::JPEGCodec::Decode( qq: Is it safe/allowed to decode an arbitrary jpeg file from ARC in the browser process? app.mojom's host implementation uses chrome/browser/image_decoder.h to decode pngs in the utility process.
arc_wallpaper_handler.cc ＆ arc/arc_wallpaper_handler.h lgtm https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:45: // it might be quite a lot of work. That won't work. What you need to do is save the wallpaper and the thumbnail to the Wallpaper Picker's sandboxed filesystem. The way to do that basically looks like: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/wallp... and https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/wallpa...
https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:36: base::WrapUnique(new ArcWallpaperHandler())))); MakeUnique? (here and elsewhere). base/memory/ptr_util.h: // MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)): bare // calls to `new` should be treated with scrutiny. https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:18: const char kAndroidWallpaperFilename[] = "android.jpg"; nit: constexpr seems to be the new preferred way. https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:110: std::unique_ptr<LinkHandlerModelImpl> impl( You can add the same DCHECK here if you want. It's called only on the UI thread. https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.h (right): https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.h:66: void SetImageAsWallpaper(gfx::ImageSkia image); const reference did you mean? (although it's ref-counted and the copy is not that expensive.)
hidehiko: PTAL yusukes: PTAL dcheng: PTAL for mojom changes xdai, thanks for reviewing! New patch has no WallpaperManager-related diff from the last patch (except for comments). https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:36: base::WrapUnique(new ArcWallpaperHandler())))); On 2016/06/15 16:45:01, Yusuke Sato wrote: > MakeUnique? (here and elsewhere). > > base/memory/ptr_util.h: > // MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)): bare > // calls to `new` should be treated with scrutiny. Sure, that's better. https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:18: const char kAndroidWallpaperFilename[] = "android.jpg"; On 2016/06/15 16:45:01, Yusuke Sato wrote: > nit: constexpr seems to be the new preferred way. Done. https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:45: // it might be quite a lot of work. On 2016/06/15 16:30:14, xdai1 wrote: > That won't work. What you need to do is save the wallpaper and the thumbnail to > the Wallpaper Picker's sandboxed filesystem. The way to do that basically looks > like: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/wallp... > > and > https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/wallpa... Thanks, updated the comment accordingly. https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:30: std::unique_ptr<SkBitmap> bitmap = gfx::JPEGCodec::Decode( On 2016/06/15 16:12:49, Yusuke Sato wrote: > qq: Is it safe/allowed to decode an arbitrary jpeg file from ARC in the browser > process? app.mojom's host implementation uses chrome/browser/image_decoder.h to > decode pngs in the utility process. Good point, maybe we must use ImageDecoder instead. Since components/arc can not depend on chrome/browser/image_decoder, I moved all decoding logic into arc_wallpaper_handler. https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:110: std::unique_ptr<LinkHandlerModelImpl> impl( On 2016/06/15 16:45:01, Yusuke Sato wrote: > You can add the same DCHECK here if you want. It's called only on the UI thread. Done. https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.h (right): https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.h:66: void SetImageAsWallpaper(gfx::ImageSkia image); On 2016/06/15 16:45:01, Yusuke Sato wrote: > const reference did you mean? (although it's ref-counted and the copy is not > that expensive.) Done.
hidehiko: PTAL yusukes: PTAL dcheng: PTAL for mojom changes xdai, thanks for reviewing! New patch has no WallpaperManager-related diff from the last patch (except for comments). https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:36: base::WrapUnique(new ArcWallpaperHandler())))); On 2016/06/15 16:45:01, Yusuke Sato wrote: > MakeUnique? (here and elsewhere). > > base/memory/ptr_util.h: > // MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)): bare > // calls to `new` should be treated with scrutiny. Sure, that's better. https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:18: const char kAndroidWallpaperFilename[] = "android.jpg"; On 2016/06/15 16:45:01, Yusuke Sato wrote: > nit: constexpr seems to be the new preferred way. Done. https://codereview.chromium.org/2061183002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:45: // it might be quite a lot of work. On 2016/06/15 16:30:14, xdai1 wrote: > That won't work. What you need to do is save the wallpaper and the thumbnail to > the Wallpaper Picker's sandboxed filesystem. The way to do that basically looks > like: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/wallp... > > and > https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/wallpa... Thanks, updated the comment accordingly. https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:30: std::unique_ptr<SkBitmap> bitmap = gfx::JPEGCodec::Decode( On 2016/06/15 16:12:49, Yusuke Sato wrote: > qq: Is it safe/allowed to decode an arbitrary jpeg file from ARC in the browser > process? app.mojom's host implementation uses chrome/browser/image_decoder.h to > decode pngs in the utility process. Good point, maybe we must use ImageDecoder instead. Since components/arc can not depend on chrome/browser/image_decoder, I moved all decoding logic into arc_wallpaper_handler. https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:110: std::unique_ptr<LinkHandlerModelImpl> impl( On 2016/06/15 16:45:01, Yusuke Sato wrote: > You can add the same DCHECK here if you want. It's called only on the UI thread. Done. https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.h (right): https://codereview.chromium.org/2061183002/diff/100001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.h:66: void SetImageAsWallpaper(gfx::ImageSkia image); On 2016/06/15 16:45:01, Yusuke Sato wrote: > const reference did you mean? (although it's ref-counted and the copy is not > that expensive.) Done.
Thanks for the fix. LGTM with nits, comments and questions. Defer to dcheng@. https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:43: ~UnsafeJpegImageDecoder() {} override = default; https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:50: void UnsafeJpegImageDecoder::Start(const std::string& jpeg_data, nit: What about adding '// static' here? I was a little bit confused about the code and the comment. https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:60: SkBitmap immutable(decoded_image); Can you add a short comment on why setImmutable() and MakeThreadSafe() is necessary here? (sorry - I'm not very familiar with skia/image classes.) https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:82: if (image.isNull()) { Does this catch the callback_.Run(gfx::ImageSkia()); case at line 69? Just wanted to make sure. https://codereview.chromium.org/2061183002/diff/140001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2061183002/diff/140001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:82: set_wallpaper_delegate_->SetWallpaper(jpeg_data_as_string); nit: SetWallpaper(jpeg_data.PassStorage()); or something like that might be better here? Is there any reason to prefer const std::string& over std::vector<uint8_t> by value (or uint8_t[] with size_t)? It's probably not a big deal, but with std::vector or a raw array, you can probably avoid one copy. ...ah okay image_decoder.h only takes a std::string object. Even so, I'd prefer uint8_t (and to convert it to std::string right before calling into image_decoder.h) since the data is definitely not a string. We could fix image_decoder later then.
hidehiko: PTAL dcheng: PTAL for mojom changes yusukes, thanks for reviewing! https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:43: ~UnsafeJpegImageDecoder() {} On 2016/06/15 19:44:32, Yusuke Sato wrote: > override = default; Done. https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:50: void UnsafeJpegImageDecoder::Start(const std::string& jpeg_data, On 2016/06/15 19:44:32, Yusuke Sato wrote: > nit: What about adding '// static' here? I was a little bit confused about the > code and the comment. Done. https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:60: SkBitmap immutable(decoded_image); On 2016/06/15 19:44:32, Yusuke Sato wrote: > Can you add a short comment on why setImmutable() and MakeThreadSafe() is > necessary here? (sorry - I'm not very familiar with skia/image classes.) Hmm, actually I just followed the way chrome/browser/chromeos/extensions/wallpaper_function_base.cc does, but forgot to copy comments. https://codereview.chromium.org/2061183002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:82: if (image.isNull()) { On 2016/06/15 19:44:32, Yusuke Sato wrote: > Does this catch the callback_.Run(gfx::ImageSkia()); case at line 69? Just > wanted to make sure. Yes. https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.cc?sq=package:ch... ImageSkia::ImageSkia() : storage_(NULL) { https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.h?dr=CSs&q=image... bool isNull() const { return storage_.get() == NULL; } https://codereview.chromium.org/2061183002/diff/140001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2061183002/diff/140001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:82: set_wallpaper_delegate_->SetWallpaper(jpeg_data_as_string); On 2016/06/15 19:44:32, Yusuke Sato wrote: > nit: > > SetWallpaper(jpeg_data.PassStorage()); or something like that might be better > here? > > Is there any reason to prefer const std::string& over std::vector<uint8_t> by > value (or uint8_t[] with size_t)? It's probably not a big deal, but with > std::vector or a raw array, you can probably avoid one copy. > > ...ah okay image_decoder.h only takes a std::string object. Even so, I'd prefer > uint8_t (and to convert it to std::string right before calling into > image_decoder.h) since the data is definitely not a string. We could fix > image_decoder later then. That sounds better, I've updated the patch to use std::vector<uint8_t> everywhere except the final step passing it to ImageDecoder.
nya@chromium.org changed reviewers: + rickyz@chromium.org
mojom lgtm, thanks for all the cleanups! https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:31: // ImageSkia instance can be null value if decoding failed. Heh, didn't realize this was referring to ImageSkia::isNull, and got confused for a bit https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:82: std::string jpeg_data_as_string(reinterpret_cast<const char*>(jpeg_data[0]), Blech, I'm reading through ImageDecoder::StartWithOptions, and the number of times the image data is copied is disappointing. Would be nice to send a change to ImageDecoder to get rid of this first copy at least. The current sequence of copies happening is: std::vector<uint8_t> -> std::string -> std::vector<unsigned char> -> mojo::Array<uint8_t> Which is kind of sad. https://codereview.chromium.org/2061183002/diff/160001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2061183002/diff/160001/components/arc/arc_ser... components/arc/arc_service_manager.cc:63: AddService(base::WrapUnique( Was it intentional to remove the adding of this service?
https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:28: class UnsafeJpegImageDecoder : public ImageDecoder::ImageRequest { This looks just an adapter of from Callback to ImageRequest. Could you make it so? class ...Request : public ... ImageRequest { public: ...Request(const Callback& callback) : callback_(callback) { } : private: Callback callback_; : }; https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:59: new UnsafeJpegImageDecoder(jpeg_data, callback); We should cancel the operation, on the destruction of the ArcWallpaperHandler. Thus: class AWHandler : ... { public: ... private: std::vector<std::unique_ptr<DecodeImageRequest> > inflight_requests_; }; AWHandler::~AWHandler() { // On UI thread. for (DecodeImageRequest* request : inflight_requests_) { ImageDecoder::Cancel(request); } } void SetWallpaper(... data) { DecodeImageRequest* request = new Request(base::bind(...)); inflight_requests_.push_back(request); ImageDecoder::StartWithOption(request, ...); } ... and so on... Nothen, the life time should be managed by the AWHandler. https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:82: std::string jpeg_data_as_string(reinterpret_cast<const char*>(jpeg_data[0]), On 2016/06/16 01:38:48, rickyz wrote: > Blech, I'm reading through ImageDecoder::StartWithOptions, and the number of > times the image data is copied is disappointing. > > Would be nice to send a change to ImageDecoder to get rid of this first copy at > least. The current sequence of copies happening is: > > std::vector<uint8_t> -> std::string -> std::vector<unsigned char> -> > mojo::Array<uint8_t> > > Which is kind of sad. Ah, +1 for :-(. Maybe, we want to fix it separately. https://codereview.chromium.org/2061183002/diff/160001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2061183002/diff/160001/components/arc/arc_ser... components/arc/arc_service_manager.cc:63: AddService(base::WrapUnique( On 2016/06/16 01:38:48, rickyz wrote: > Was it intentional to remove the adding of this service? FYI; Moved to arc service launcher, because it starts to depend on the CrOS code.
hidehiko: PTAL https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:28: class UnsafeJpegImageDecoder : public ImageDecoder::ImageRequest { On 2016/06/16 05:49:24, hidehiko wrote: > This looks just an adapter of from Callback to ImageRequest. Could you make it > so? > > class ...Request : public ... ImageRequest { > public: > ...Request(const Callback& callback) : callback_(callback) { > } > : > private: > Callback callback_; > : > }; With ImageDecoder::Cancel(), I can do this, thanks! https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:31: // ImageSkia instance can be null value if decoding failed. On 2016/06/16 01:38:48, rickyz wrote: > Heh, didn't realize this was referring to ImageSkia::isNull, and got confused > for a bit That's why I wrote null "value" but it might not be very clear yet... Anyway, this line has been deleted in latest patch. https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:59: new UnsafeJpegImageDecoder(jpeg_data, callback); On 2016/06/16 05:49:24, hidehiko wrote: > We should cancel the operation, on the destruction of the ArcWallpaperHandler. > > Thus: > > class AWHandler : ... { > public: > ... > private: > std::vector<std::unique_ptr<DecodeImageRequest> > inflight_requests_; > }; > > AWHandler::~AWHandler() { > // On UI thread. > for (DecodeImageRequest* request : inflight_requests_) { > ImageDecoder::Cancel(request); > } > } > > void SetWallpaper(... data) { > DecodeImageRequest* request = new Request(base::bind(...)); > inflight_requests_.push_back(request); > ImageDecoder::StartWithOption(request, ...); > } > > ... and so on... > Nothen, the life time should be managed by the AWHandler. Ah I did not notice there is ImageDecoder::Cancel(). Then I can make sure callbacks are not called after destruction without weak pointers. I've updated the code to use cancel. It looks much better, thanks! BTW, I could not use set<unique_ptr> in inflight_requests_ because I can not execute set::erase() on it. If you know any better way, please let me know. https://codereview.chromium.org/2061183002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:82: std::string jpeg_data_as_string(reinterpret_cast<const char*>(jpeg_data[0]), On 2016/06/16 05:49:24, hidehiko wrote: > On 2016/06/16 01:38:48, rickyz wrote: > > Blech, I'm reading through ImageDecoder::StartWithOptions, and the number of > > times the image data is copied is disappointing. > > > > Would be nice to send a change to ImageDecoder to get rid of this first copy > at > > least. The current sequence of copies happening is: > > > > std::vector<uint8_t> -> std::string -> std::vector<unsigned char> -> > > mojo::Array<uint8_t> > > > > Which is kind of sad. > > Ah, +1 for :-(. Maybe, we want to fix it separately. Agreed. I'll leave a TODO here. https://codereview.chromium.org/2061183002/diff/160001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2061183002/diff/160001/components/arc/arc_ser... components/arc/arc_service_manager.cc:63: AddService(base::WrapUnique( On 2016/06/16 05:49:24, hidehiko wrote: > On 2016/06/16 01:38:48, rickyz wrote: > > Was it intentional to remove the adding of this service? > > FYI; Moved to arc service launcher, because it starts to depend on the CrOS > code. Yup, hidehiko is right.
https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:78: ArcWallpaperHandler::~ArcWallpaperHandler() { Optional: can we have ThreadChecker in this class, too? https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:100: inflight_requests_.erase(request); delete request? https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:105: inflight_requests_.erase(request); ditto. https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.h (right): https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.h:10: #include <memory> Unnecessary? https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.h:12: #include <vector> is still needed (SetWallpaper's arg).
hidehiko: PTAL Rebased to rebase mojom conflict. https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:78: ArcWallpaperHandler::~ArcWallpaperHandler() { On 2016/06/16 20:31:53, hidehiko wrote: > Optional: can we have ThreadChecker in this class, too? Sure, done. https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:100: inflight_requests_.erase(request); On 2016/06/16 20:31:53, hidehiko wrote: > delete request? Grrr, thanks for catching. Done. https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:105: inflight_requests_.erase(request); On 2016/06/16 20:31:53, hidehiko wrote: > ditto. Done. https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.h (right): https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.h:10: #include <memory> On 2016/06/16 20:31:53, hidehiko wrote: > Unnecessary? Done. https://codereview.chromium.org/2061183002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.h:12: On 2016/06/16 20:31:53, hidehiko wrote: > #include <vector> > > is still needed (SetWallpaper's arg). Done.
LGTM! https://codereview.chromium.org/2061183002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:79: DCHECK(thread_checker_.CalledOnValidThread()); Redundant. (ThreadChecker checks if it is called on the created thread, so this should always pass).
Thanks for reviewing! https://codereview.chromium.org/2061183002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:79: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/06/17 02:16:56, hidehiko wrote: > Redundant. (ThreadChecker checks if it is called on the created thread, so this > should always pass). Thanks, good to know.
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, yusukes@chromium.org, rickyz@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2061183002/#ps280001 (title: "Remove redundant crot.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2061183002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Updated the patch with style fix.
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, rickyz@chromium.org, yusukes@chromium.org, xdai@chromium.org Link to the patchset: https://codereview.chromium.org/2061183002/#ps300001 (title: "Style fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2061183002/300001
Message was sent while issue was closed.
Description was changed from
==========
arc: IPC method to set custom wallpaper.
A new IPC method SetWallpaper() is added to allow apps to
set custom wallpaper.
Notes:
- The IPC method accepts JPEG data only for now because
Chrome OS Wallpaper Manager re-encodes any custom wallpaper
image to JPEG anyway.
- New wallpaper image does not show up in Chrome OS wallpaper
picker, just similarly as when the wallpaper is set by
enterprise policy. This is good enough for first version
and it is a TODO to register the new wallpaper to picker.
- STRETCH layout is always used to render the wallpaper for
now. We need UI discussion when we allow the user to select
the layout later.
BUG=chromium:618922
TEST=Manually tested with Android change that SetWallpaper()
IPC call changes the wallpaper successfully.
==========
to
==========
arc: IPC method to set custom wallpaper.
A new IPC method SetWallpaper() is added to allow apps to
set custom wallpaper.
Notes:
- The IPC method accepts JPEG data only for now because
Chrome OS Wallpaper Manager re-encodes any custom wallpaper
image to JPEG anyway.
- New wallpaper image does not show up in Chrome OS wallpaper
picker, just similarly as when the wallpaper is set by
enterprise policy. This is good enough for first version
and it is a TODO to register the new wallpaper to picker.
- STRETCH layout is always used to render the wallpaper for
now. We need UI discussion when we allow the user to select
the layout later.
BUG=chromium:618922
TEST=Manually tested with Android change that SetWallpaper()
IPC call changes the wallpaper successfully.
==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from
==========
arc: IPC method to set custom wallpaper.
A new IPC method SetWallpaper() is added to allow apps to
set custom wallpaper.
Notes:
- The IPC method accepts JPEG data only for now because
Chrome OS Wallpaper Manager re-encodes any custom wallpaper
image to JPEG anyway.
- New wallpaper image does not show up in Chrome OS wallpaper
picker, just similarly as when the wallpaper is set by
enterprise policy. This is good enough for first version
and it is a TODO to register the new wallpaper to picker.
- STRETCH layout is always used to render the wallpaper for
now. We need UI discussion when we allow the user to select
the layout later.
BUG=chromium:618922
TEST=Manually tested with Android change that SetWallpaper()
IPC call changes the wallpaper successfully.
==========
to
==========
arc: IPC method to set custom wallpaper.
A new IPC method SetWallpaper() is added to allow apps to
set custom wallpaper.
Notes:
- The IPC method accepts JPEG data only for now because
Chrome OS Wallpaper Manager re-encodes any custom wallpaper
image to JPEG anyway.
- New wallpaper image does not show up in Chrome OS wallpaper
picker, just similarly as when the wallpaper is set by
enterprise policy. This is good enough for first version
and it is a TODO to register the new wallpaper to picker.
- STRETCH layout is always used to render the wallpaper for
now. We need UI discussion when we allow the user to select
the layout later.
BUG=chromium:618922
TEST=Manually tested with Android change that SetWallpaper()
IPC call changes the wallpaper successfully.
Committed: https://crrev.com/fc1bf374333bb1d9bd0d64971e64eb6f3e604292
Cr-Commit-Position: refs/heads/master@{#400399}
==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/fc1bf374333bb1d9bd0d64971e64eb6f3e604292 Cr-Commit-Position: refs/heads/master@{#400399}
Message was sent while issue was closed.
https://codereview.chromium.org/2061183002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:106: delete request; Just curious, but do we really need to handle multiple in-flight requests? I think we can just remember one and Cancel() the previous? Then we don't need the container at all.
Message was sent while issue was closed.
https://codereview.chromium.org/2061183002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2061183002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:106: delete request; On 2016/06/17 13:23:27, dcheng wrote: > Just curious, but do we really need to handle multiple in-flight requests? I > think we can just remember one and Cancel() the previous? Then we don't need the > container at all. You're right. That sounds better. Let me address it in another patch. |
