Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2264743002:
cheets: implement cros side of WallpaperManagerService. (Closed)
|
Created:
4 years, 4 months ago by Muyuan Modified:
4 years, 3 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, oshima+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org, davemoore+watch_chromium.org, qsr+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncheets: implement cros side of WallpaperManagerService.
Rename ArcWallpaperHandler to ArcWallpaperService and implement it as an ArcService, which handles WallpaperManager API from Android.
Test=Manually open PickMyWallpaper app and set wallpaper
Test=run cts android.content.cts.ContextWrapperTest
Bug=642465
Committed: https://crrev.com/533be7ef1063d285008913c9784d8d852055c8bd
Cr-Commit-Position: refs/heads/master@{#417428}
Patch Set 1 #Patch Set 2 : cheets: implement cros side of WallpaperManagerService. #Patch Set 3 : cheets: implement cros side of WallpaperManagerService. #
Total comments: 31
Patch Set 4 : cheets: implement cros side of WallpaperManagerService. #
Total comments: 30
Patch Set 5 : cheets: implement cros side of WallpaperManagerService. #
Total comments: 10
Patch Set 6 : cheets: implement cros side of WallpaperManagerService. #
Total comments: 5
Patch Set 7 : cheets: implement cros side of WallpaperManagerService. #
Total comments: 2
Patch Set 8 : cheets: implement cros side of WallpaperManagerService. #Patch Set 9 : cheets: implement cros side of WallpaperManagerService. #
Total comments: 5
Patch Set 10 : cheets: implement cros side of WallpaperManagerService. #
Total comments: 18
Patch Set 11 : Add a static instance to SetWallpaperDelegate and null check to ArcIntentHelper. #
Total comments: 12
Patch Set 12 : add DCHECK with static instance and fixed other minor issues. #
Total comments: 9
Patch Set 13 : moved SetWallpaperDelegate::instance_ to ArcIntentHelper; addressed minor edits. #
Total comments: 1
Patch Set 14 : Make instance_ in SetWallpaperDelegate private. #Patch Set 15 : rebase #
Total comments: 6
Patch Set 16 : change instance getter / setter in SetWallpaperDelegate to snake_case. #Messages
Total messages: 63 (15 generated)
muyuanli@chromium.org changed reviewers: + nya@chromium.org, xiaohuic@chromium.org, xiyuan@chromium.org
Description was changed from ========== cheets: implement cros side of WallpaperManagerService. - Make ArcWallpaperHandler singleton. - Add GetWallpaper API to ArcWallpaperHandler. - Implement arc_wallpaper_bridge to handle WallpaperService APIs from Android side. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest BUG=29589509 ========== to ========== cheets: implement cros side of WallpaperManagerService. - Make ArcWallpaperHandler singleton. - Add GetWallpaper API to ArcWallpaperHandler. - Implement arc_wallpaper_bridge to handle WallpaperService APIs from Android side. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest BUG=b/29589509 ==========
https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:77: static std::shared_ptr<ArcWallpaperHandler> instance(new ArcWallpaperHandler); We might want to use CR_DEFINE_STATIC_LOCAL or a LazyInstance instead of this. https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:95: gfx::JPEG1xEncodedDataFromImage(wallpaper, 100, &result); What happens if there is no wallpaper and JPEG1xEncodedDataFromImage returns false? https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:9: #include "base/threading/thread_checker.h" This should be moved to header file since we declare it as a member there. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:16: std::shared_ptr<SetWallpaperDelegate>& nit: The format looks wrong. Can you run "git cl format"? https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:25: DCHECK(thread_checker_.CalledOnValidThread()); #include "base/logging.h" https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.h (right): https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:11: #include "components/arc/arc_bridge_service.h" We can move this into cc file and forward declare ArcBridgeService. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:15: #include "components/arc/set_wallpaper_delegate.h" Move to cc file and forward declare SetWallpaperDelegate. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:25: explicit ArcWallpaperBridge(ArcBridgeService* bridge_service, "explicit" is only needed for single arg ctor, so not needed here.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:77: static std::shared_ptr<ArcWallpaperHandler> instance(new ArcWallpaperHandler); DCHECK_CURRENTLY_ON(content::BrowserThread::UI); https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:90: std::vector<uint8_t> ArcWallpaperHandler::GetWallpaper() const { DCHECK_CURRENTLY_ON(...); Note that we should not be encoding stuff on the UI thread. https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:96: return std::move(result); Can you run trybots? The compiler will complain about this pessimization and will request you to omit the std::move when returning something. https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... components/arc/common/wallpaper.mojom:24: Init@1(WallpaperHost host_ptr); nit: Init@0
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Drive-by. https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:7: #include <cstdlib> Clarification: for what? https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:77: static std::shared_ptr<ArcWallpaperHandler> instance(new ArcWallpaperHandler); Could you get rid of Singleton? Instead, ArcLauncher can keep the instance, and it can pass the ptr to necessary services. https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... components/arc/common/wallpaper.mojom:13: [MinVersion=1] GetWallpaper@2() => (array<uint8> wallpaper); @1 Could you explicitly document the encoding of the |wallpaper|'s content? https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... components/arc/common/wallpaper.mojom:17: [MinVersion=1] SetWallpaper@1(array<uint8> jpeg_data); @0 https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... components/arc/common/wallpaper.mojom:23: // Establishes communication with the container side. nit: 2-indent please.
https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:77: static std::shared_ptr<ArcWallpaperHandler> instance(new ArcWallpaperHandler); On 2016/08/22 02:30:10, hidehiko wrote: > Could you get rid of Singleton? > Instead, ArcLauncher can keep the instance, and it can pass the ptr to necessary > services. It sounds better :) thx. https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:90: std::vector<uint8_t> ArcWallpaperHandler::GetWallpaper() const { On 2016/08/19 23:22:56, Luis Héctor Chávez wrote: > DCHECK_CURRENTLY_ON(...); > > Note that we should not be encoding stuff on the UI thread. Done. https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:95: gfx::JPEG1xEncodedDataFromImage(wallpaper, 100, &result); On 2016/08/19 23:04:07, xiyuan wrote: > What happens if there is no wallpaper and JPEG1xEncodedDataFromImage returns > false? When there is no wallpaper, JPEG1xEncodedDataFromImage returns an empty vectory, causing decoder in Android side to fail and WallpaperManager will handle it by returning default Android wallpaper or null based on user's choice. https://codereview.chromium.org/2264743002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:96: return std::move(result); On 2016/08/19 23:22:56, Luis Héctor Chávez wrote: > Can you run trybots? The compiler will complain about this pessimization and > will request you to omit the std::move when returning something. Acknowledged. https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... components/arc/common/wallpaper.mojom:13: [MinVersion=1] GetWallpaper@2() => (array<uint8> wallpaper); On 2016/08/22 02:30:11, hidehiko wrote: > @1 > > Could you explicitly document the encoding of the |wallpaper|'s content? Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... components/arc/common/wallpaper.mojom:17: [MinVersion=1] SetWallpaper@1(array<uint8> jpeg_data); On 2016/08/22 02:30:10, hidehiko wrote: > @0 Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... components/arc/common/wallpaper.mojom:23: // Establishes communication with the container side. On 2016/08/22 02:30:11, hidehiko wrote: > nit: 2-indent please. Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/common/w... components/arc/common/wallpaper.mojom:24: Init@1(WallpaperHost host_ptr); On 2016/08/19 23:22:56, Luis Héctor Chávez wrote: > nit: Init@0 Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:9: #include "base/threading/thread_checker.h" On 2016/08/19 23:04:07, xiyuan wrote: > This should be moved to header file since we declare it as a member there. Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:16: std::shared_ptr<SetWallpaperDelegate>& On 2016/08/19 23:04:07, xiyuan wrote: > nit: The format looks wrong. Can you run "git cl format"? Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:25: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/08/19 23:04:07, xiyuan wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.h (right): https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:11: #include "components/arc/arc_bridge_service.h" On 2016/08/19 23:04:07, xiyuan wrote: > We can move this into cc file and forward declare ArcBridgeService. Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:15: #include "components/arc/set_wallpaper_delegate.h" On 2016/08/19 23:04:07, xiyuan wrote: > Move to cc file and forward declare SetWallpaperDelegate. Done. https://codereview.chromium.org/2264743002/diff/40001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:25: explicit ArcWallpaperBridge(ArcBridgeService* bridge_service, On 2016/08/19 23:04:07, xiyuan wrote: > "explicit" is only needed for single arg ctor, so not needed here. Done.
https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.h (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.h:28: std::shared_ptr<ArcWallpaperHandler> arc_wallpaper_handler_; Do we really need a shared_ptr? Can some class own ArcWallpaperHandler and provide access to its instance? https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:99: content::BrowserThread::IO, FROM_HERE, IO thread is for network IO etc. Maybe do the job in a blocking pool thread? https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:23: } nit: insert an empty line before and add "// namespace"
https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.h (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.h:28: std::shared_ptr<ArcWallpaperHandler> arc_wallpaper_handler_; On 2016/08/22 22:49:41, xiyuan wrote: > Do we really need a shared_ptr? Can some class own ArcWallpaperHandler and > provide access to its instance? OK.. So I wonder if ArcServiceManager should own it? https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:99: content::BrowserThread::IO, FROM_HERE, On 2016/08/22 22:49:41, xiyuan wrote: > IO thread is for network IO etc. Maybe do the job in a blocking pool thread? Acknowledged. https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:23: } On 2016/08/22 22:49:41, xiyuan wrote: > nit: insert an empty line before and add "// namespace" Done.
https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.h (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.h:28: std::shared_ptr<ArcWallpaperHandler> arc_wallpaper_handler_; On 2016/08/22 23:48:19, muyuanli wrote: > On 2016/08/22 22:49:41, xiyuan wrote: > > Do we really need a shared_ptr? Can some class own ArcWallpaperHandler and > > provide access to its instance? > > OK.. So I wonder if ArcServiceManager should own it? Could it still be owned by ArcIntentHelperBridge as before? Where else do you need to access it?
https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.h (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.h:28: std::shared_ptr<ArcWallpaperHandler> arc_wallpaper_handler_; On 2016/08/23 00:41:04, xiyuan wrote: > On 2016/08/22 23:48:19, muyuanli wrote: > > On 2016/08/22 22:49:41, xiyuan wrote: > > > Do we really need a shared_ptr? Can some class own ArcWallpaperHandler and > > > provide access to its instance? > > > > OK.. So I wonder if ArcServiceManager should own it? > > Could it still be owned by ArcIntentHelperBridge as before? Where else do you > need to access it? ArcWallpaperHandler is accessed from ArcIntentHelperBridge and ArcWallpaperBridge so it can't be owned by ArcIntentHelperBridge. That said, please make this std::unique_ptr and pass a raw (borrowed) pointer to ArcIntentHelperBridge/ArcWallpaperBridge. It is safe because ArcServiceLauncher is guaranteed to outlive ArcServices. Please avoid std::shared_ptr as far as possible since it makes it harder to reason about the timing and the thread of destruction. https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:69: callback.Run(result); We must invoke |callback| on UI thread. Please use PostTaskAndReply() to bounce back to UI thread. Also please make sure to use std::move() where possible to reduce copies. Note that, as lhchavez@ pointed out in earlier comment, you should not use std::move() when returning a local variable nevertheless. https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:97: gfx::Image wallpaper(dbc->GetWallpaper()); I know gfx::JPEG1xEncodedDataFromImage() actually checks if a given image is null, but could you check it by yourself explicitly? Documentation of gfx::JPEG1xEncodedDataFromImage() does not mention about the behavior when a null image is given: https://cs.chromium.org/chromium/src/ui/gfx/image/image_util.h?sq=package:chr... OTOH, documentation of gfx::Image warns that use of a null image will cause a crash: https://cs.chromium.org/chromium/src/ui/gfx/image/image.h?q=gfx::image&sq=pac... https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:99: content::BrowserThread::IO, FROM_HERE, On 2016/08/22 23:48:19, muyuanli wrote: > On 2016/08/22 22:49:41, xiyuan wrote: > > IO thread is for network IO etc. Maybe do the job in a blocking pool thread? > > Acknowledged. As xiyuan said, please avoid doing CPU-intensive task on IO thread. For example message loop of IPC between browser and renderer is performed on this thread, so during JPEG decoding, IPC will be choked. https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/a... components/arc/common/arc_bridge.mojom:106: [MinVersion=19] OnWallpaperInstanceReady@124(WallpaperInstance instance_ptr); Did you mean: MinVersion=18 https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/w... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/w... components/arc/common/wallpaper.mojom:13: [MinVersion=1] GetWallpaper@1() => (array<uint8> wallpaper); I'm not sure about security implication of allowing ARC to read the current wallpaper. Did you talk with security teams? https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/w... components/arc/common/wallpaper.mojom:17: [MinVersion=1] SetWallpaper@0(array<uint8> jpeg_data); Optional: You can just skip MinVersion since those APIs exist from the first version. https://codereview.chromium.org/2264743002/diff/60001/components/arc/set_wall... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/set_wall... components/arc/set_wallpaper_delegate.h:11: #include "base/macros.h" I guess you meant base/callback.h ? https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:23: } On 2016/08/22 23:48:19, muyuanli wrote: > On 2016/08/22 22:49:41, xiyuan wrote: > > nit: insert an empty line before and add "// namespace" > > Done. Could you upload a new patch please? https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:40: mojom::WallpaperInstance* wallpaper_instance = Please call DCHECK(thread_checker_.CalledOnValidThread()); here too. https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.h (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:34: // mojo::WallpaperHost overrides. nit: mojo[m]::
https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.h (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.h:28: std::shared_ptr<ArcWallpaperHandler> arc_wallpaper_handler_; On 2016/08/23 00:41:04, xiyuan wrote: > On 2016/08/22 23:48:19, muyuanli wrote: > > On 2016/08/22 22:49:41, xiyuan wrote: > > > Do we really need a shared_ptr? Can some class own ArcWallpaperHandler and > > > provide access to its instance? > > > > OK.. So I wonder if ArcServiceManager should own it? > > Could it still be owned by ArcIntentHelperBridge as before? Where else do you > need to access it? ArcWallpaperBridge https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.h:28: std::shared_ptr<ArcWallpaperHandler> arc_wallpaper_handler_; On 2016/08/23 09:34:21, Shuhei Takahashi wrote: > On 2016/08/23 00:41:04, xiyuan wrote: > > On 2016/08/22 23:48:19, muyuanli wrote: > > > On 2016/08/22 22:49:41, xiyuan wrote: > > > > Do we really need a shared_ptr? Can some class own ArcWallpaperHandler and > > > > provide access to its instance? > > > > > > OK.. So I wonder if ArcServiceManager should own it? > > > > Could it still be owned by ArcIntentHelperBridge as before? Where else do you > > need to access it? > > ArcWallpaperHandler is accessed from ArcIntentHelperBridge and > ArcWallpaperBridge so it can't be owned by ArcIntentHelperBridge. > > That said, please make this std::unique_ptr and pass a raw (borrowed) pointer to > ArcIntentHelperBridge/ArcWallpaperBridge. It is safe because ArcServiceLauncher > is guaranteed to outlive ArcServices. Please avoid std::shared_ptr as far as > possible since it makes it harder to reason about the timing and the thread of > destruction. Done. https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:69: callback.Run(result); On 2016/08/23 09:34:21, Shuhei Takahashi wrote: > We must invoke |callback| on UI thread. Please use PostTaskAndReply() to bounce > back to UI thread. > > Also please make sure to use std::move() where possible to reduce copies. Note > that, as lhchavez@ pointed out in earlier comment, you should not use > std::move() when returning a local variable nevertheless. Acknowledged. https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:97: gfx::Image wallpaper(dbc->GetWallpaper()); On 2016/08/23 09:34:21, Shuhei Takahashi wrote: > I know gfx::JPEG1xEncodedDataFromImage() actually checks if a given image is > null, but could you check it by yourself explicitly? > > Documentation of gfx::JPEG1xEncodedDataFromImage() does not mention about the > behavior when a null image is given: > https://cs.chromium.org/chromium/src/ui/gfx/image/image_util.h?sq=package:chr... > > OTOH, documentation of gfx::Image warns that use of a null image will cause a > crash: > https://cs.chromium.org/chromium/src/ui/gfx/image/image.h?q=gfx::image&sq=pac... DesktopBackgroundController::GetWallpaper() will return an empty image when the background is not available so it should not be null. https://codereview.chromium.org/2264743002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:99: content::BrowserThread::IO, FROM_HERE, On 2016/08/23 09:34:21, Shuhei Takahashi wrote: > On 2016/08/22 23:48:19, muyuanli wrote: > > On 2016/08/22 22:49:41, xiyuan wrote: > > > IO thread is for network IO etc. Maybe do the job in a blocking pool thread? > > > > Acknowledged. > > As xiyuan said, please avoid doing CPU-intensive task on IO thread. For example > message loop of IPC between browser and renderer is performed on this thread, so > during JPEG decoding, IPC will be choked. Acknowledged. https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/a... components/arc/common/arc_bridge.mojom:106: [MinVersion=19] OnWallpaperInstanceReady@124(WallpaperInstance instance_ptr); On 2016/08/23 09:34:21, Shuhei Takahashi wrote: > Did you mean: MinVersion=18 Done. https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/w... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/w... components/arc/common/wallpaper.mojom:13: [MinVersion=1] GetWallpaper@1() => (array<uint8> wallpaper); On 2016/08/23 09:34:21, Shuhei Takahashi wrote: > I'm not sure about security implication of allowing ARC to read the current > wallpaper. Did you talk with security teams? Acknowledged. https://codereview.chromium.org/2264743002/diff/60001/components/arc/common/w... components/arc/common/wallpaper.mojom:17: [MinVersion=1] SetWallpaper@0(array<uint8> jpeg_data); On 2016/08/23 09:34:21, Shuhei Takahashi wrote: > Optional: You can just skip MinVersion since those APIs exist from the first > version. Thx. Is it ok to keep it just to look consistent? https://codereview.chromium.org/2264743002/diff/60001/components/arc/set_wall... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/set_wall... components/arc/set_wallpaper_delegate.h:11: #include "base/macros.h" On 2016/08/23 09:34:22, Shuhei Takahashi wrote: > I guess you meant base/callback.h ? Done. https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:23: } On 2016/08/23 09:34:22, Shuhei Takahashi wrote: > On 2016/08/22 23:48:19, muyuanli wrote: > > On 2016/08/22 22:49:41, xiyuan wrote: > > > nit: insert an empty line before and add "// namespace" > > > > Done. > > Could you upload a new patch please? Done. https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.cc:40: mojom::WallpaperInstance* wallpaper_instance = On 2016/08/23 09:34:22, Shuhei Takahashi wrote: > Please call DCHECK(thread_checker_.CalledOnValidThread()); here too. Done. https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.h (right): https://codereview.chromium.org/2264743002/diff/60001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:34: // mojo::WallpaperHost overrides. On 2016/08/23 09:34:22, Shuhei Takahashi wrote: > nit: mojo[m]:: Done.
https://codereview.chromium.org/2264743002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:101: std::shared_ptr<std::vector<uint8_t>> buffer(new std::vector<uint8_t>()); I would avoid shared_ptr. Either use PostTaskAndReplyWithResult, or use base::Owned() for |buffer|. e.g. // Use PostTaskAndReplyWithResult // std::vector<uint8_t> EncodeImageJPEG(const gfx::Image image)... // void OnImageEncoded( // const base::Callback<void(std::vector<uint8_t> image)>& callback, // const std::vector<uint8_t>& bytes); base::PostTaskAndReplyWithResult( BrowserThread::GetBlockingPool(), FROM_HERE, base::Bind(&EncodeImageJPEG, wallpaper), base::Bind(&OnImageEncoded, callback)); // Use base::Owned std::vector<uint8_t>* buffer = new std::vector<uint8_t>(); content::BrowserThread::GetBlockingPool()->PostTaskAndReply( FROM_HERE, base::Bind(&EncodeImageJPEG, wallpaper, buffer), base::Bind(&OnImageEncoded, base::Owned(buffer), callback)); https://codereview.chromium.org/2264743002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:103: FROM_HERE, base::Bind(&EncodeImageJPEG, std::move(wallpaper), buffer), nit: std::move is not necessary, gfx::Image assignment is cheap. https://codereview.chromium.org/2264743002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/arc_intent_helper_bridge.h (right): https://codereview.chromium.org/2264743002/diff/80001/components/arc/intent_h... components/arc/intent_helper/arc_intent_helper_bridge.h:74: SetWallpaperDelegate* set_wallpaper_delegate_; nit: SetWallpaperDelegate* const set_wallpaper_delegate_; if |set_wallpaper_delegate_| never changes during lifetime of this class. https://codereview.chromium.org/2264743002/diff/80001/components/arc/set_wall... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/80001/components/arc/set_wall... components/arc/set_wallpaper_delegate.h:11: #include "base/callback.h" nit: callback_forward.h https://codereview.chromium.org/2264743002/diff/80001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.h (right): https://codereview.chromium.org/2264743002/diff/80001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:40: SetWallpaperDelegate* wallpaper_delegate_; nit: SetWallpaperDelegate* const
https://codereview.chromium.org/2264743002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:101: std::shared_ptr<std::vector<uint8_t>> buffer(new std::vector<uint8_t>()); On 2016/08/23 20:43:47, xiyuan wrote: > I would avoid shared_ptr. Either use PostTaskAndReplyWithResult, or use > base::Owned() for |buffer|. > > e.g. > // Use PostTaskAndReplyWithResult > // std::vector<uint8_t> EncodeImageJPEG(const gfx::Image image)... > // void OnImageEncoded( > // const base::Callback<void(std::vector<uint8_t> image)>& callback, > // const std::vector<uint8_t>& bytes); > base::PostTaskAndReplyWithResult( > BrowserThread::GetBlockingPool(), > FROM_HERE, > base::Bind(&EncodeImageJPEG, wallpaper), > base::Bind(&OnImageEncoded, callback)); > > // Use base::Owned > std::vector<uint8_t>* buffer = new std::vector<uint8_t>(); > content::BrowserThread::GetBlockingPool()->PostTaskAndReply( > FROM_HERE, > base::Bind(&EncodeImageJPEG, wallpaper, buffer), > base::Bind(&OnImageEncoded, base::Owned(buffer), callback)); Thx https://codereview.chromium.org/2264743002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:103: FROM_HERE, base::Bind(&EncodeImageJPEG, std::move(wallpaper), buffer), On 2016/08/23 20:43:47, xiyuan wrote: > nit: std::move is not necessary, gfx::Image assignment is cheap. Done. https://codereview.chromium.org/2264743002/diff/80001/components/arc/intent_h... File components/arc/intent_helper/arc_intent_helper_bridge.h (right): https://codereview.chromium.org/2264743002/diff/80001/components/arc/intent_h... components/arc/intent_helper/arc_intent_helper_bridge.h:74: SetWallpaperDelegate* set_wallpaper_delegate_; On 2016/08/23 20:43:47, xiyuan wrote: > nit: SetWallpaperDelegate* const set_wallpaper_delegate_; > > if |set_wallpaper_delegate_| never changes during lifetime of this class. Done. https://codereview.chromium.org/2264743002/diff/80001/components/arc/set_wall... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/80001/components/arc/set_wall... components/arc/set_wallpaper_delegate.h:11: #include "base/callback.h" On 2016/08/23 20:43:48, xiyuan wrote: > nit: callback_forward.h Done. https://codereview.chromium.org/2264743002/diff/80001/components/arc/wallpape... File components/arc/wallpaper/arc_wallpaper_bridge.h (right): https://codereview.chromium.org/2264743002/diff/80001/components/arc/wallpape... components/arc/wallpaper/arc_wallpaper_bridge.h:40: SetWallpaperDelegate* wallpaper_delegate_; On 2016/08/23 20:43:48, xiyuan wrote: > nit: SetWallpaperDelegate* const Done.
lgtm
muyuanli@chromium.org changed reviewers: + dcheng@chromium.org
lgtm
https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:65: std::vector<uint8_t> EncodeImageJPEG(const gfx::Image image) { const ref to avoid the copy? https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:99: base::Bind(&EncodeImageJPEG, wallpaper), callback); std::move is needed to avoid the copy?
https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.cc (right): https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.cc:99: base::Bind(&EncodeImageJPEG, wallpaper), callback); On 2016/08/24 16:22:18, hidehiko wrote: > std::move is needed to avoid the copy? gfx::Image is cheap to copy since it only increment the storage's ref count. [1] IMHO, std::move does not make a real difference. [1]: https://cs.chromium.org/chromium/src/ui/gfx/image/image.h?rcl=0&l=82-86
https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.h (right): https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.h:32: void GetWallpaper(const base::Callback<void(std::vector<uint8_t> image)>& This (and the previous function) should probably pass std::vector by const ref.
https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_handler.h (right): https://codereview.chromium.org/2264743002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_handler.h:32: void GetWallpaper(const base::Callback<void(std::vector<uint8_t> image)>& On 2016/08/25 06:23:48, dcheng wrote: > This (and the previous function) should probably pass std::vector by const ref. Done. Also updated the previous function.
lgtm https://codereview.chromium.org/2264743002/diff/120001/components/arc/wallpap... File components/arc/wallpaper/arc_wallpaper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/120001/components/arc/wallpap... components/arc/wallpaper/arc_wallpaper_bridge.cc:60: base::Bind(&HandleWallpaperResult, callback)); FWIW, mojo::Array and stuff are deprecated. If we get rid of use_new_wrapper_types = false from the GN file, then we wouldn't have to create an intermediate callback here. We should probably consider doing this sooner rather than later.
https://codereview.chromium.org/2264743002/diff/120001/components/arc/wallpap... File components/arc/wallpaper/arc_wallpaper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/120001/components/arc/wallpap... components/arc/wallpaper/arc_wallpaper_bridge.cc:60: base::Bind(&HandleWallpaperResult, callback)); On 2016/08/29 20:33:19, dcheng wrote: > FWIW, mojo::Array and stuff are deprecated. If we get rid of > use_new_wrapper_types = false from the GN file, then we wouldn't have to create > an intermediate callback here. We should probably consider doing this sooner > rather than later. Added a TODO in the source. I'll fix that in the future when the whole module is upgraded to new interface.
Description was changed from ========== cheets: implement cros side of WallpaperManagerService. - Make ArcWallpaperHandler singleton. - Add GetWallpaper API to ArcWallpaperHandler. - Implement arc_wallpaper_bridge to handle WallpaperService APIs from Android side. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest BUG=b/29589509 ========== to ========== cheets: implement cros side of WallpaperManagerService. - Make ArcWallpaperHandler singleton. - Add GetWallpaper API to ArcWallpaperHandler. - Implement arc_wallpaper_bridge to handle WallpaperService APIs from Android side. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest BUG=642465 ==========
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, nya@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2264743002/#ps160001 (title: "cheets: implement cros side of WallpaperManagerService.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
muyuanli@chromium.org changed reviewers: + yusukes@chromium.org
https://codereview.chromium.org/2264743002/diff/160001/components/arc/common/... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/160001/components/arc/common/... components/arc/common/wallpaper.mojom:13: [MinVersion=1] GetWallpaper@1() => (array<uint8> wallpaper); [MinVersion] is not needed until after the version has been bumped (and also change L5 to say that next MinVersion is 1). Also, can you change the ordinal in GetWallpaper to @0 and SetWallpaper to @1? This is the first version, so might as well have them in order. https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:16: class SetWallpaperDelegate { Can you rename it to WallpaperDelegate?
https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:16: class SetWallpaperDelegate { On 2016/08/30 22:47:15, Luis Héctor Chávez wrote: > Can you rename it to WallpaperDelegate? After an offline discussion, this interface should eventually go away. For now and for backwards compatibility it should stay as-is, but the intention is that the responsibilities of the WallpaperHost should be assumed by what now is called ArcWallpaperHandler (the only implementation of SetWallpaperDelegate), and eventually ArcIntentHelper will stop having wallpaper-related responsibilities, at which point this class should disappear.
FYI https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:16: class SetWallpaperDelegate { On 2016/08/31 04:25:14, Luis Héctor Chávez wrote: > On 2016/08/30 22:47:15, Luis Héctor Chávez wrote: > > Can you rename it to WallpaperDelegate? > > After an offline discussion, this interface should eventually go away. For now > and for backwards compatibility it should stay as-is, but the intention is that > the responsibilities of the WallpaperHost should be assumed by what now is > called ArcWallpaperHandler (the only implementation of SetWallpaperDelegate), > and eventually ArcIntentHelper will stop having wallpaper-related > responsibilities, at which point this class should disappear. Clarification: So the plan is; - Deprecate IntentHelper::SetWallpaper() - Switch to use WallpaperHost for SetWallpaper in ARC. - Remove IntentHelper::SetWallpaper() - Migrate SetWallpaperDelegate into WalpaperHost implementation. Am I correct? If so, can we move ArcWallpaperBridge c/b/c/arc and migrate with ArcWallpaperHandler in this CL, because there is no reason to keep the two classes separated with the plan, I think? The migrated class can inherit SetWallpaperDelegate with TODO to remove it at the moment, so that IntentHelper can proxy from its SetWallpaper.
On 2016/08/31 04:59:53, hidehiko wrote: > FYI > > https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... > File components/arc/set_wallpaper_delegate.h (right): > > https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... > components/arc/set_wallpaper_delegate.h:16: class SetWallpaperDelegate { > On 2016/08/31 04:25:14, Luis Héctor Chávez wrote: > > On 2016/08/30 22:47:15, Luis Héctor Chávez wrote: > > > Can you rename it to WallpaperDelegate? > > > > After an offline discussion, this interface should eventually go away. For now > > and for backwards compatibility it should stay as-is, but the intention is > that > > the responsibilities of the WallpaperHost should be assumed by what now is > > called ArcWallpaperHandler (the only implementation of SetWallpaperDelegate), > > and eventually ArcIntentHelper will stop having wallpaper-related > > responsibilities, at which point this class should disappear. > > Clarification: So the plan is; > - Deprecate IntentHelper::SetWallpaper() > - Switch to use WallpaperHost for SetWallpaper in ARC. > - Remove IntentHelper::SetWallpaper() > - Migrate SetWallpaperDelegate into WalpaperHost implementation. > > Am I correct? If so, can we move ArcWallpaperBridge c/b/c/arc and migrate with > ArcWallpaperHandler in this CL, because there is no reason to keep the two > classes separated with the plan, I think? > The migrated class can inherit SetWallpaperDelegate with TODO to remove it at > the moment, so that IntentHelper can proxy from its SetWallpaper. yes, that's the plan (i now realized that the summary i wrote here of the offline discussion is a bit short on details).
Description was changed from ========== cheets: implement cros side of WallpaperManagerService. - Make ArcWallpaperHandler singleton. - Add GetWallpaper API to ArcWallpaperHandler. - Implement arc_wallpaper_bridge to handle WallpaperService APIs from Android side. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest BUG=642465 ========== to ========== cheets: implement cros side of WallpaperManagerService. Rename ArcWallpaperHandler to ArcWallpaperService and implement it as an ArcService, which handles WallpaperManager API from Android. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest Bug=642465 ==========
https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/160001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:16: class SetWallpaperDelegate { On 2016/08/31 04:59:53, hidehiko wrote: > On 2016/08/31 04:25:14, Luis Héctor Chávez wrote: > > On 2016/08/30 22:47:15, Luis Héctor Chávez wrote: > > > Can you rename it to WallpaperDelegate? > > > > After an offline discussion, this interface should eventually go away. For now > > and for backwards compatibility it should stay as-is, but the intention is > that > > the responsibilities of the WallpaperHost should be assumed by what now is > > called ArcWallpaperHandler (the only implementation of SetWallpaperDelegate), > > and eventually ArcIntentHelper will stop having wallpaper-related > > responsibilities, at which point this class should disappear. > > Clarification: So the plan is; > - Deprecate IntentHelper::SetWallpaper() > - Switch to use WallpaperHost for SetWallpaper in ARC. > - Remove IntentHelper::SetWallpaper() > - Migrate SetWallpaperDelegate into WalpaperHost implementation. > > Am I correct? If so, can we move ArcWallpaperBridge c/b/c/arc and migrate with > ArcWallpaperHandler in this CL, because there is no reason to keep the two > classes separated with the plan, I think? > The migrated class can inherit SetWallpaperDelegate with TODO to remove it at > the moment, so that IntentHelper can proxy from its SetWallpaper. Done.
Looks much nicer. My biggest worry is use-after-free (please see my comment in arc_service_launcher.cc). Others are just minor comments. https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:34: std::unique_ptr<ArcService> arc_wallpaper_handler_ = nit: Please remove trailing underscore. https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:49: (ArcWallpaperService*)arc_wallpaper_handler_.get(), This can be use-after-free. IIUC, the element destruction order is not fixed in the C++ spec. So, you'd need WeakPtr until the clean up, instead. https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.h (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.h:28: std::unique_ptr<ArcWallpaperService> arc_wallpaper_handler_; unused. https://codereview.chromium.org/2264743002/diff/180001/components/arc/common/... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/180001/components/arc/common/... components/arc/common/wallpaper.mojom:17: SetWallpaper@1(array<uint8> png_data); Clarification: I'd like to know the clean-up plan. So the plan is merging IntentHelper's SetWallpaper to here. IntentHelper's one supports JPEG and this supports PNG. Who'll convert the image? I thought there could be several approaches; 1) Convert image format in ARC container (JPEG -> PNG). Then I'm fine with this API. 2) Pass through the image as is via Mojo, and in ArcWallpaperService, decode images based on the format. Then, it's better to add file format to the API. Or, alternatively you may be able to detect the file format by, e.g., magic. 3) Decode image in ARC container (JPEG -> Bitmap, PNG -> Bitmap). Then the file format should be bitmap, here. The pros is simplicity, but the cons is that the data will be bigger. WDYT? https://codereview.chromium.org/2264743002/diff/180001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/180001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:11: #include "base/callback_forward.h" No longer needed. https://codereview.chromium.org/2264743002/diff/180001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:16: class SetWallpaperDelegate { Could you add TODO to remove this, with the migrate SetWallpaper plan?
https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:49: (ArcWallpaperService*)arc_wallpaper_handler_.get(), On 2016/09/02 05:07:13, hidehiko wrote: > This can be use-after-free. IIUC, the element destruction order is not fixed in > the C++ spec. So, you'd need WeakPtr until the clean up, instead. Another approach (since this will be removed very soon) is to have a global static getter on the ArcWallpaperService and have the ArcIntentHelperBridge consume *and* null-check it every time it wants to set the wallpaper. If you go through this route, please add TODOs to remove all the static getter-related code. https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:102: ImageDecoder::StartWithOptions(this, std::move(png_data.PassStorage()), |png_data.PassStorage()| is already an rvalue, there's no need for std::move(). https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.h (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.h:45: const base::Callback<void(mojo::Array<uint8_t>)>& callback) override; nit: const mojom::WallpaperHost::GetWallpaperCallback& https://codereview.chromium.org/2264743002/diff/180001/components/arc/common/... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/180001/components/arc/common/... components/arc/common/wallpaper.mojom:17: SetWallpaper@1(array<uint8> png_data); On 2016/09/02 05:07:13, hidehiko wrote: > Clarification: I'd like to know the clean-up plan. > > So the plan is merging IntentHelper's SetWallpaper to here. IntentHelper's one > supports JPEG and this supports PNG. Who'll convert the image? I thought there > could be several approaches; > 1) Convert image format in ARC container (JPEG -> PNG). Then I'm fine with this > API. > 2) Pass through the image as is via Mojo, and in ArcWallpaperService, decode > images based on the format. Then, it's better to add file format to the API. Or, > alternatively you may be able to detect the file format by, e.g., magic. > 3) Decode image in ARC container (JPEG -> Bitmap, PNG -> Bitmap). Then the file > format should be bitmap, here. The pros is simplicity, but the cons is that the > data will be bigger. > > WDYT? The intention is to drop the IntentHelper's SetWallpaper altogether, so nobody will have to do JPEG encoding/decoding in the future :) Android's implementation of the IntentHelper will call the WallpaperManager directly instead of having a second way of setting the wallpaper.
Also, overall comment: whenever git cl upload asks you to provide a summary of the patch set, please describe the delta between the current iteration and the previous one instead of repeating the patch title. That helps us reviewers identify what changed.
https://codereview.chromium.org/2264743002/diff/180001/components/arc/common/... File components/arc/common/wallpaper.mojom (right): https://codereview.chromium.org/2264743002/diff/180001/components/arc/common/... components/arc/common/wallpaper.mojom:17: SetWallpaper@1(array<uint8> png_data); On 2016/09/02 16:23:48, Luis Héctor Chávez wrote: > On 2016/09/02 05:07:13, hidehiko wrote: > > Clarification: I'd like to know the clean-up plan. > > > > So the plan is merging IntentHelper's SetWallpaper to here. IntentHelper's one > > supports JPEG and this supports PNG. Who'll convert the image? I thought there > > could be several approaches; > > 1) Convert image format in ARC container (JPEG -> PNG). Then I'm fine with > this > > API. > > 2) Pass through the image as is via Mojo, and in ArcWallpaperService, decode > > images based on the format. Then, it's better to add file format to the API. > Or, > > alternatively you may be able to detect the file format by, e.g., magic. > > 3) Decode image in ARC container (JPEG -> Bitmap, PNG -> Bitmap). Then the > file > > format should be bitmap, here. The pros is simplicity, but the cons is that > the > > data will be bigger. > > > > WDYT? > > The intention is to drop the IntentHelper's SetWallpaper altogether, so nobody > will have to do JPEG encoding/decoding in the future :) Android's implementation > of the IntentHelper will call the WallpaperManager directly instead of having a > second way of setting the wallpaper. Makes sense. Thank you for clarification.
https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:34: std::unique_ptr<ArcService> arc_wallpaper_handler_ = On 2016/09/02 05:07:13, hidehiko wrote: > nit: Please remove trailing underscore. Acknowledged. https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:49: (ArcWallpaperService*)arc_wallpaper_handler_.get(), On 2016/09/02 16:23:47, Luis Héctor Chávez wrote: > On 2016/09/02 05:07:13, hidehiko wrote: > > This can be use-after-free. IIUC, the element destruction order is not fixed > in > > the C++ spec. So, you'd need WeakPtr until the clean up, instead. > > Another approach (since this will be removed very soon) is to have a global > static getter on the ArcWallpaperService and have the ArcIntentHelperBridge > consume *and* null-check it every time it wants to set the wallpaper. If you go > through this route, please add TODOs to remove all the static getter-related > code. Done. https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.h (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.h:28: std::unique_ptr<ArcWallpaperService> arc_wallpaper_handler_; On 2016/09/02 05:07:13, hidehiko wrote: > unused. Done. https://codereview.chromium.org/2264743002/diff/180001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/180001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:11: #include "base/callback_forward.h" On 2016/09/02 05:07:13, hidehiko wrote: > No longer needed. Done. https://codereview.chromium.org/2264743002/diff/180001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:16: class SetWallpaperDelegate { On 2016/09/02 05:07:13, hidehiko wrote: > Could you add TODO to remove this, with the migrate SetWallpaper plan? Done.
https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:102: ImageDecoder::StartWithOptions(this, std::move(png_data.PassStorage()), On 2016/09/02 16:23:47, Luis Héctor Chávez wrote: > |png_data.PassStorage()| is already an rvalue, there's no need for std::move(). ping on the removal of this std::move(). https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:59: arc_service_manager_->AddService(base::MakeUnique<ArcWallpaperService>( nit: keep this list sorted lexicographically. https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:76: SetWallpaperDelegate* SetWallpaperDelegate::instance_; nit: initialize to nullptr. https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:81: instance_ = (SetWallpaperDelegate*)this; nit: use C++-style casts. (actually, is it needed?) you also need to add a DCHECK(!instance_); https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:90: instance_ = nullptr; nit: DCHECK(instance_ == this); https://codereview.chromium.org/2264743002/diff/200001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (left): https://codereview.chromium.org/2264743002/diff/200001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:9: keep this empty line: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2264743002/diff/200001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/200001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:13: class ArcBridgeService; unused?
https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:102: ImageDecoder::StartWithOptions(this, std::move(png_data.PassStorage()), On 2016/09/06 20:05:46, Luis Héctor Chávez wrote: > On 2016/09/02 16:23:47, Luis Héctor Chávez wrote: > > |png_data.PassStorage()| is already an rvalue, there's no need for > std::move(). > > ping on the removal of this std::move(). done. sry for overlooking it. https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:59: arc_service_manager_->AddService(base::MakeUnique<ArcWallpaperService>( On 2016/09/06 20:05:46, Luis Héctor Chávez wrote: > nit: keep this list sorted lexicographically. Done. https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:76: SetWallpaperDelegate* SetWallpaperDelegate::instance_; On 2016/09/06 20:05:46, Luis Héctor Chávez wrote: > nit: initialize to nullptr. Done. https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:81: instance_ = (SetWallpaperDelegate*)this; On 2016/09/06 20:05:46, Luis Héctor Chávez wrote: > nit: use C++-style casts. (actually, is it needed?) > > you also need to add a DCHECK(!instance_); Done. https://codereview.chromium.org/2264743002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:90: instance_ = nullptr; On 2016/09/06 20:05:46, Luis Héctor Chávez wrote: > nit: DCHECK(instance_ == this); Done. https://codereview.chromium.org/2264743002/diff/200001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (left): https://codereview.chromium.org/2264743002/diff/200001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:9: On 2016/09/06 20:05:46, Luis Héctor Chávez wrote: > keep this empty line: > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.chromium.org/2264743002/diff/200001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/200001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:13: class ArcBridgeService; done.
Only minor comments, except L76 of https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo.... https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:66: std::vector<uint8_t> EncodeImagePNG(const gfx::ImageSkia image) { nit: EncodeImagePng. cf) https://google.github.io/styleguide/cppguide.html#Function_Names "Prefer to capitalize acronyms as single words (i.e. StartRpc(), not StartRPC())." https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:76: SetWallpaperDelegate* SetWallpaperDelegate::instance_ = nullptr; Could you define this as a part of components/arc/libarc.a? Even for the very short-term solution, I'd like to keep the consistency of the lib definition. IMHO, define the var somewhere in components/arc/libarc.a, and introducing static SetWallpaperDelegate::SetInstance(...) is fine. https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:100: LOG(ERROR) << "OnWallpaperInstanceReady called, " Could you use LOG(DFATAL) instead? This is unexpected situation. So, on Debug build, crashing will be better. https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:125: void ArcWallpaperService::SetWallpaperJPEG( Ditto. "SetWallpaperJpeg" https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.h (right): https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.h:45: const base::Callback<void(mojo::Array<uint8_t>)>& callback) override; could you use "const GetWallpaperCallback&"?
https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:66: std::vector<uint8_t> EncodeImagePNG(const gfx::ImageSkia image) { On 2016/09/07 01:16:25, hidehiko wrote: > nit: EncodeImagePng. > > cf) https://google.github.io/styleguide/cppguide.html#Function_Names > "Prefer to capitalize acronyms as single words (i.e. StartRpc(), not > StartRPC())." Acknowledged. https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:76: SetWallpaperDelegate* SetWallpaperDelegate::instance_ = nullptr; On 2016/09/07 01:16:25, hidehiko wrote: > Could you define this as a part of components/arc/libarc.a? > Even for the very short-term solution, I'd like to keep the consistency of the > lib definition. > > IMHO, define the var somewhere in components/arc/libarc.a, and introducing > static SetWallpaperDelegate::SetInstance(...) is fine. OK. Made it part of ArcIntentHelper temporarily. https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:100: LOG(ERROR) << "OnWallpaperInstanceReady called, " On 2016/09/07 01:16:25, hidehiko wrote: > Could you use LOG(DFATAL) instead? > This is unexpected situation. So, on Debug build, crashing will be better. Acknowledged. https://codereview.chromium.org/2264743002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:125: void ArcWallpaperService::SetWallpaperJPEG( On 2016/09/07 01:16:25, hidehiko wrote: > Ditto. "SetWallpaperJpeg" Acknowledged.
LGTM with one more request. Defer to Luis. https://codereview.chromium.org/2264743002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:77: DCHECK(!instance_); nit: Could you follow the style guide by avoiding protected static field? https://google.github.io/styleguide/cppguide.html#Access_Control
On 2016/09/07 03:26:49, hidehiko wrote: > LGTM with one more request. Defer to Luis. > > https://codereview.chromium.org/2264743002/diff/240001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): > > https://codereview.chromium.org/2264743002/diff/240001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_wallpaper_service.cc:77: DCHECK(!instance_); > nit: Could you follow the style guide by avoiding protected static field? > https://google.github.io/styleguide/cppguide.html#Access_Control So I made the instance_ field private by adding a public method SetSetWallpaperDelegate。
lgtm with nits. Thanks for being patient with us :) https://codereview.chromium.org/2264743002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:77: DCHECK(!GetInstance()); nit: Since these are all static functions, can you qualify them with the class name? WallpaperDelegate::instance() WallpaperDelegate::set_instance() https://codereview.chromium.org/2264743002/diff/280001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/280001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:33: SetWallpaperDelegate* SetWallpaperDelegate::instance_ = nullptr; for future reference: you should have added a set_wallpaper_delegate.cc just to put this, but since the plan is to deprecate it almost immediately afterwards, this is fine. https://codereview.chromium.org/2264743002/diff/280001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/280001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:21: static SetWallpaperDelegate* GetInstance() { return instance_; } nit: Chromium style guide prefers having these inline functions use lowercase snake_case names. instance() { return instance_; } set_instance(SetWallpaperDelegate* instance) { instance_ = instance; }
https://codereview.chromium.org/2264743002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_wallpaper_service.cc (right): https://codereview.chromium.org/2264743002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_wallpaper_service.cc:77: DCHECK(!GetInstance()); On 2016/09/08 19:14:24, Luis Héctor Chávez wrote: > nit: Since these are all static functions, can you qualify them with the class > name? > > WallpaperDelegate::instance() > WallpaperDelegate::set_instance() Done. https://codereview.chromium.org/2264743002/diff/280001/components/arc/intent_... File components/arc/intent_helper/arc_intent_helper_bridge.cc (right): https://codereview.chromium.org/2264743002/diff/280001/components/arc/intent_... components/arc/intent_helper/arc_intent_helper_bridge.cc:33: SetWallpaperDelegate* SetWallpaperDelegate::instance_ = nullptr; On 2016/09/08 19:14:24, Luis Héctor Chávez wrote: > for future reference: you should have added a set_wallpaper_delegate.cc just to > put this, but since the plan is to deprecate it almost immediately afterwards, > this is fine. Acknowledged. https://codereview.chromium.org/2264743002/diff/280001/components/arc/set_wal... File components/arc/set_wallpaper_delegate.h (right): https://codereview.chromium.org/2264743002/diff/280001/components/arc/set_wal... components/arc/set_wallpaper_delegate.h:21: static SetWallpaperDelegate* GetInstance() { return instance_; } On 2016/09/08 19:14:24, Luis Héctor Chávez wrote: > nit: Chromium style guide prefers having these inline functions use lowercase > snake_case names. > > instance() { return instance_; } > set_instance(SetWallpaperDelegate* instance) { instance_ = instance; } Done.
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, dcheng@chromium.org, nya@chromium.org, hidehiko@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2264743002/#ps300001 (title: "change instance getter / setter in SetWallpaperDelegate to snake_case.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== cheets: implement cros side of WallpaperManagerService. Rename ArcWallpaperHandler to ArcWallpaperService and implement it as an ArcService, which handles WallpaperManager API from Android. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest Bug=642465 ========== to ========== cheets: implement cros side of WallpaperManagerService. Rename ArcWallpaperHandler to ArcWallpaperService and implement it as an ArcService, which handles WallpaperManager API from Android. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest Bug=642465 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== cheets: implement cros side of WallpaperManagerService. Rename ArcWallpaperHandler to ArcWallpaperService and implement it as an ArcService, which handles WallpaperManager API from Android. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest Bug=642465 ========== to ========== cheets: implement cros side of WallpaperManagerService. Rename ArcWallpaperHandler to ArcWallpaperService and implement it as an ArcService, which handles WallpaperManager API from Android. Test=Manually open PickMyWallpaper app and set wallpaper Test=run cts android.content.cts.ContextWrapperTest Bug=642465 Committed: https://crrev.com/533be7ef1063d285008913c9784d8d852055c8bd Cr-Commit-Position: refs/heads/master@{#417428} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/533be7ef1063d285008913c9784d8d852055c8bd Cr-Commit-Position: refs/heads/master@{#417428}
Message was sent while issue was closed.
FYI: Findit try jobs (rerunning failed compile or tests) identified this CL at revision 417428 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/2320323002/ by kbr@chromium.org. The reason for reverting is: Broke the build. Not sure how this made it past the CQ: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... .
Message was sent while issue was closed.
There was a move from ash/wallpaper/wallpaper_controller.h to ash/common/wallpaper_controller.h when my CL is in CQ. On Thu, Sep 8, 2016 at 4:27 PM <kbr@chromium.org> wrote: > A revert of this CL (patchset #16 id:300001) has been created in > https://codereview.chromium.org/2320323002/ by kbr@chromium.org. > > The reason for reverting is: Broke the build. Not sure how this made it > past the > CQ: > > https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... > . > > https://codereview.chromium.org/2264743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
