Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778053002:
Fetch ARC Kiosk app name and icon from Android side. (Closed)
|
Created:
3 years, 9 months ago by Sergey Poromov Modified:
3 years, 8 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, hidehiko+watch_chromium.org, achuith+watch_chromium.org, poromov+watch_chromium.org, lhchavez+watch_chromium.org, ios-reviews_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFetch ARC Kiosk app name and icon from Android side.
To provide appropriate and up-to-date app name and icon
for ARC Kiosk apps on sign in screen, this data is updated from
ARC side when Android container contains information about them,
i.e.apps are installed in ARC Kiosk session.
Before that name is provided by DMServer and default icon is used.
ArcKioskAppData is extended from simple structure to a class,
which caches the data from above to be used on sign in screen.
IconLoader from KioskAppData is extracted as general
KioskAppIconLoader for both Chrome and ARC apps.
BUG=679928
TEST=Manual
Review-Url: https://codereview.chromium.org/2778053002
Cr-Commit-Position: refs/heads/master@{#463607}
Committed: https://chromium.googlesource.com/chromium/src/+/a5709293688a27bb196e33b9488193012f653f63
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase #
Total comments: 7
Patch Set 3 : address comments #
Total comments: 6
Patch Set 4 : cleanup KioskAppIconLoader #
Total comments: 8
Patch Set 5 : extract common code into KioskAppDataBase #
Total comments: 51
Patch Set 6 : address comments #
Total comments: 8
Patch Set 7 : remove CHECK #Patch Set 8 : move weak_ptr from KioskAppDataBase to KioskAppIconLoader #
Total comments: 17
Patch Set 9 : khmel@ comments #
Total comments: 2
Patch Set 10 : xiyuan@ comments #
Total comments: 7
Patch Set 11 : lhchavez@ comments. #Patch Set 12 : fix tests. #
Total comments: 6
Patch Set 13 : some nits #
Total comments: 2
Messages
Total messages: 94 (60 generated)
poromov@chromium.org changed reviewers: + khmel@chromium.org
Yury, could you please first review how icon is requested, retrieved and stored? I'll add owners to review after that.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: 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...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Hi Sergey, Please find my few comments. Plus I have top level question. I see code that duplicates (or looks similar) functionality to store/keep/cache icon data. Is any chance we can reuse existing ARC App functionality? https://codereview.chromium.org/2778053002/diff/1/chrome/browser/chromeos/app... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2778053002/diff/1/chrome/browser/chromeos/app... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:30: constexpr int kArcAppIconSizeInDp = 48; nit: Probably it is better to use kGridIconDimension. It seems we need one place to define size of ARC icon. It is not safe to define size of icon in each file (this can be done as separate CL) https://codereview.chromium.org/2778053002/diff/1/chrome/browser/chromeos/app... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:148: if (icon == app_icon_.get()) { nit: Do you expect OnIconUpdated is called for not app_icon? May be DCHECK_EQ()? https://codereview.chromium.org/2778053002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc (right): https://codereview.chromium.org/2778053002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc:132: ArcKioskAppManager::Apps arc_apps; nit: arc_kiosk_apps;? to reduce confusion. https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:49: base::Unretained(this))); We might need weak pointer binding instead of Unretained or add comment why it is safe. https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:73: base::Unretained(this))); Use weak pointer instead of Unretained? https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:80: if (load_result_ == SUCCESS) You set and read load_results_ from different threads IIUC https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:50: class IconImageRequest : public ImageDecoder::ImageRequest { nit: Move to cc file and use here only forward declaration?
poromov@chromium.org changed reviewers: + xiyuan@chromium.org
Yes, there is lots of code duplicated from KioskAppData into ArcKioskAppData. I'll try to reduce it, most likely introducing general KioskAppDataBase. For now, addressing your other comments. +Xiyuan to provide more feedback on KioskIconLoader implementation. https://codereview.chromium.org/2778053002/diff/1/chrome/browser/chromeos/app... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2778053002/diff/1/chrome/browser/chromeos/app... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:30: constexpr int kArcAppIconSizeInDp = 48; On 2017/03/27 22:56:32, khmel wrote: > nit: Probably it is better to use kGridIconDimension. It seems we need one place > to define size of ARC icon. It is not safe to define size of icon in each file > (this can be done as separate CL) Done. https://codereview.chromium.org/2778053002/diff/1/chrome/browser/chromeos/app... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:148: if (icon == app_icon_.get()) { On 2017/03/27 22:56:32, khmel wrote: > nit: Do you expect OnIconUpdated is called for not app_icon? May be DCHECK_EQ()? Yes, now it's requested mostly once at any time. Initially it was requested every time RequestNameAndIconUpdate() is called, i.e.probably several times in a row, with new ArcAppIcon every time. https://codereview.chromium.org/2778053002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc (right): https://codereview.chromium.org/2778053002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc:132: ArcKioskAppManager::Apps arc_apps; On 2017/03/27 22:56:32, khmel wrote: > nit: arc_kiosk_apps;? to reduce confusion. This class is only about kiosks and Chrome kiosk apps are named just "apps" above, so "arc_apps" is compliant with them. I think no need for one more "kiosk" word in the class. https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:49: base::Unretained(this))); On 2017/03/27 22:56:34, khmel wrote: > We might need weak pointer binding instead of Unretained or add comment why it > is safe. I just refactored to extract this class from KioskAppData, not going to change any internal behavior. I suspect that here in below it was written this way because the class is self-owned and deleted after NotifyClient(). +xiyuan@ for more comments as he implemented the class 4 years ago: http://crrev.com/12223018 https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:50: class IconImageRequest : public ImageDecoder::ImageRequest { On 2017/03/27 22:56:34, khmel wrote: > nit: Move to cc file and use here only forward declaration? Done. Thanks, missed the opportunity.
https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:49: base::Unretained(this))); On 2017/03/28 15:25:55, Sergey Poromov wrote: > On 2017/03/27 22:56:34, khmel wrote: > > We might need weak pointer binding instead of Unretained or add comment why it > > is safe. > > I just refactored to extract this class from KioskAppData, not going to change > any internal behavior. > I suspect that here in below it was written this way because the class is > self-owned and deleted after NotifyClient(). > > +xiyuan@ for more comments as he implemented the class 4 years ago: > http://crrev.com/12223018 The reading is correct. IconLoader manages its own lifetime and no one keeps a reference to it. The class must be created then followed by a Start(). There is no way to cancel the load request after Start(). Its |client_| is a WeakPtr and can go away before the loader. But loader has to finish the full cycle until NotifyClient is called. Adding a comment to explain would be good to make code easier to read. https://codereview.chromium.org/2778053002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:57: base::WeakPtr<Delegate> client_; nit: client_ -> delegate_ or Delegate -> Client to make it consistent. https://codereview.chromium.org/2778053002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:58: base::FilePath icon_path_; nit: Make LoadOnBlockingPool take a const base::FilePath& and get rid of this. https://codereview.chromium.org/2778053002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:60: LoadResult load_result_; nit: Make NotifyClient take a LoadResult and get rid of this.
PTAL. https://codereview.chromium.org/2778053002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:57: base::WeakPtr<Delegate> client_; On 2017/03/28 16:50:09, xiyuan wrote: > nit: client_ -> delegate_ or Delegate -> Client to make it consistent. Done. https://codereview.chromium.org/2778053002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:58: base::FilePath icon_path_; On 2017/03/28 16:50:09, xiyuan wrote: > nit: Make LoadOnBlockingPool take a const base::FilePath& and get rid of this. Done. https://codereview.chromium.org/2778053002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:60: LoadResult load_result_; On 2017/03/28 16:50:10, xiyuan wrote: > nit: Make NotifyClient take a LoadResult and get rid of this. Done.
Thanks for the clean up. Let's wait for khmel@. Think his high-level comment is whether it is possible to share the icon caching code with ArcAppListPrefs, e.g. move relevant code out of ArcAppListPrefs into some class that can be shared. How feasible is that?
https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:167: app->SetCache(name, icon); What I see from the code (IIUC) UpdateNameAndIcon is called from OnIconUpdated. That means we decoded it as ARC icon. As ARC icon it is already stored in ARC app cache. ArcKioskAppData::SetCache implements own logic to encode image to png and store it on file. This is clear duplicate logic in 2 places. At ArcAppListPrefs we decode/encode/save icon. And here we do the same. My idea if possible not to store ARC app icon here and rely on what ArcAppListPrefs provides. WDYT?
https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:167: app->SetCache(name, icon); On 2017/03/29 17:31:43, khmel wrote: > What I see from the code (IIUC) UpdateNameAndIcon is called from OnIconUpdated. > That means we decoded it as ARC icon. As ARC icon it is already stored in ARC > app cache. ArcKioskAppData::SetCache implements own logic to encode image to > png and store it on file. This is clear duplicate logic in 2 places. At > ArcAppListPrefs we decode/encode/save icon. And here we do the same. My idea if > possible not to store ARC app icon here and rely on what ArcAppListPrefs > provides. > > WDYT? Kiosk could not use ArcAppListPrefs data directly because the icon could be used on the splash screen (part of the login screen) before mounting the app's cryptohome and loading its profile.
lgtm https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc (right): https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc:37: base::FilePath dir = icon_path.DirName(); nit: const https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.h (right): https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.h:49: AccountId account_id_; nit: app_id_ and account_id_ are const. https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:167: app->SetCache(name, icon); On 2017/03/29 17:33:44, xiyuan wrote: > On 2017/03/29 17:31:43, khmel wrote: > > What I see from the code (IIUC) UpdateNameAndIcon is called from > OnIconUpdated. > > That means we decoded it as ARC icon. As ARC icon it is already stored in ARC > > app cache. ArcKioskAppData::SetCache implements own logic to encode image to > > png and store it on file. This is clear duplicate logic in 2 places. At > > ArcAppListPrefs we decode/encode/save icon. And here we do the same. My idea > if > > possible not to store ARC app icon here and rely on what ArcAppListPrefs > > provides. > > > > WDYT? > > Kiosk could not use ArcAppListPrefs data directly because the icon could be used > on the splash screen (part of the login screen) before mounting the app's > cryptohome and loading its profile. I see. Thanks for explanation. We currently have project to enable ARC on sign-in screen. I think this would make it possible to reuse this code. However it out of scope of this CL.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. I've extracted significant part of common code from ArcKioskAppData and KioskAppData into KioskAppDataBase. Though it doesn't resolve all ugly code, it reduces the duplication. https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc (right): https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc:37: base::FilePath dir = icon_path.DirName(); On 2017/03/29 17:51:22, khmel wrote: > nit: const Done. https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.h (right): https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.h:49: AccountId account_id_; On 2017/03/29 17:51:22, khmel wrote: > nit: app_id_ and account_id_ are const. Done. https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2778053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:167: app->SetCache(name, icon); On 2017/03/29 17:51:22, khmel wrote: > On 2017/03/29 17:33:44, xiyuan wrote: > > On 2017/03/29 17:31:43, khmel wrote: > > > What I see from the code (IIUC) UpdateNameAndIcon is called from > > OnIconUpdated. > > > That means we decoded it as ARC icon. As ARC icon it is already stored in > ARC > > > app cache. ArcKioskAppData::SetCache implements own logic to encode image > to > > > png and store it on file. This is clear duplicate logic in 2 places. At > > > ArcAppListPrefs we decode/encode/save icon. And here we do the same. My idea > > if > > > possible not to store ARC app icon here and rely on what ArcAppListPrefs > > > provides. > > > > > > WDYT? > > > > Kiosk could not use ArcAppListPrefs data directly because the icon could be > used > > on the splash screen (part of the login screen) before mounting the app's > > cryptohome and loading its profile. > > I see. Thanks for explanation. > We currently have project to enable ARC on sign-in screen. I think this would > make it possible to reuse this code. However it out of scope of this CL. Yes, the data should be cached also for the case when ARC is not yet started. Probably after ARC on sign-in it will be resolved, but not for now.
https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data.h (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data.h:42: public base::SupportsWeakPtr<KioskAppData>, Can we remove this inheritance since KioskAppDataBase derives from it already ? Or even better, drop base::SupportsWeakPtr and use WeakPtrFactory instead for this and KioskAppDataBase https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.h (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:35: virtual const char* kiosk_dictionary_name() const = 0; nit: Slightly prefer to pass this in as an arg of constructor and store it in a const member. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:38: void OnIconLoadSuccess(const gfx::ImageSkia& icon) override = 0; It is a bit confusing to inherit from an interface but leaving the implementation to derived class. Can we let the derived class to directly derive from KioskAppIconLoader::Delegate instead? https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:50: // Helper to load name and icon from provided dictionary. nit: insert a blank line between each function. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:60: base::FilePath icon_path_; Can this and above members be private since we have accessors for them already? https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:63: class IconLoader; remove since it is now KioskAppIconLoader ?
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:218: // Apps are keys by package name. http://crbug.com/665904 nit: keyed https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:170: app_icon_.reset( nit: base::MakeUnique https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data.cc:47: const char kKeyRequiredPlatformVersion[] = "required_platform_version"; nit: constexpr https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data.h (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data.h:17: #include "ui/gfx/image/image_skia.h" unused? https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:27: const char kIconFileExtension[] = ".png"; nit: constexpr? https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:37: CHECK(base::CreateDirectory(dir)); don't use CHECK: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:55: void KioskAppDataBase::SaveToDictionary(DictionaryPrefUpdate& dict_update) { Can you add DCHECK_CURRENTLY_ON to all methods? https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:56: const std::string app_key = std::string(kKeyApps) + '.' + app_id_; It's probably better to use base::StringPrintf instead. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:78: (new KioskAppIconLoader(AsWeakPtr()))->Start(icon_path_); Is this the only caller? (Seems like it) If so, can you instead make this NOT delete itself? Instead make this class own a std::unique_ptr<KioskAppIconLoader>. That way you don't need to have this class support WeakPtr. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:86: scoped_refptr<base::RefCountedString> raw_icon(new base::RefCountedString); nit: Avoid parenthesisless ctors https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... (or document that you're doing it as an optimization. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:93: base::Bind(&SaveIconToLocalOnBlockingPool, icon_path, raw_icon)); Why are you wrapping image_data in a scoped_refptr? Can't you just use base::Passed(std::move(image_data)) to avoid the copy? https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:104: dict_update->Remove(app_key, NULL); nit: nullptr https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.h (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:55: std::string app_id_; nit: these two can be const. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:40: KioskAppIconLoader* icon_loader_; KioskAppIconLoader* const icon_loader_; https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:55: base::Unretained(this), icon_path)); base::Unretained(this) is unsafe in all this class: deleting |task_runner_| does not guarantee that it will be deleted since something else might have a reference to it, and so accessing it might fail. Use a weakptr for |this|. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:68: IconImageRequest* image_request = new IconImageRequest(task_runner_, this); Please note that you are transferring ownership of |image_request| to ImageDecoder, and will delete itself on completion. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:69: ImageDecoder::Start(image_request, raw_icon_->data()); Do you need the refcounted string? Why not transfer ownership of the data? ImageDecoder::Start(image_request, std::vector<uint8_t>(image_data.begin(), image_data.end())) Also, this class is going away: https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h?type=cs&q... Can you use its replacement instead?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Sorry, in some files there were some legacy code and old solutions, because some of the code was extracted from parts written many years ago. While I like to improve it, I don't want massive refactors, so I skipped some comments. If you are not comfortable with some solutions here, comment and I'll fix it. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:218: // Apps are keys by package name. http://crbug.com/665904 On 2017/03/30 16:49:53, Luis Héctor Chávez wrote: > nit: keyed Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:170: app_icon_.reset( On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > nit: base::MakeUnique Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data.cc:47: const char kKeyRequiredPlatformVersion[] = "required_platform_version"; On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > nit: constexpr Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data.h (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data.h:17: #include "ui/gfx/image/image_skia.h" On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > unused? Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data.h:42: public base::SupportsWeakPtr<KioskAppData>, On 2017/03/30 16:46:59, xiyuan wrote: > Can we remove this inheritance since KioskAppDataBase derives from it already ? > > Or even better, drop base::SupportsWeakPtr and use WeakPtrFactory instead for > this and KioskAppDataBase Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:27: const char kIconFileExtension[] = ".png"; On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > nit: constexpr? Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:37: CHECK(base::CreateDirectory(dir)); On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > don't use CHECK: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:55: void KioskAppDataBase::SaveToDictionary(DictionaryPrefUpdate& dict_update) { On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > Can you add DCHECK_CURRENTLY_ON to all methods? Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:56: const std::string app_key = std::string(kKeyApps) + '.' + app_id_; On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > It's probably better to use base::StringPrintf instead. I copied the code from original functions, prefer not to change it and IMHO it doesn't look so bad here. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:78: (new KioskAppIconLoader(AsWeakPtr()))->Start(icon_path_); On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > Is this the only caller? (Seems like it) > > If so, can you instead make this NOT delete itself? Instead make this class own > a std::unique_ptr<KioskAppIconLoader>. That way you don't need to have this > class support WeakPtr. What if KioskAppIconLoader will live longer than KioskAppDataBase ? It's possible if the icon will be loaded slower and callback in ImageRequest will be called after destruction of KioskAppDataBase. Anyway, it seems like at least one WeakPtr is needed - either here, or there. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:86: scoped_refptr<base::RefCountedString> raw_icon(new base::RefCountedString); On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > nit: Avoid parenthesisless ctors > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... > (or document that you're doing it as an optimization. Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:93: base::Bind(&SaveIconToLocalOnBlockingPool, icon_path, raw_icon)); On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > Why are you wrapping image_data in a scoped_refptr? Can't you just use > base::Passed(std::move(image_data)) to avoid the copy? Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:104: dict_update->Remove(app_key, NULL); On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > nit: nullptr Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.h (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:35: virtual const char* kiosk_dictionary_name() const = 0; On 2017/03/30 16:46:59, xiyuan wrote: > nit: Slightly prefer to pass this in as an arg of constructor and store it in a > const member. Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:38: void OnIconLoadSuccess(const gfx::ImageSkia& icon) override = 0; On 2017/03/30 16:46:59, xiyuan wrote: > It is a bit confusing to inherit from an interface but leaving the > implementation to derived class. Can we let the derived class to directly derive > from KioskAppIconLoader::Delegate instead? IconLoader is used from this class - see end of LoadFromDictionary() function, so the passing class (KioskAppDataBase) should be inherited from KioskAppIconLoader::Delegate https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:50: // Helper to load name and icon from provided dictionary. On 2017/03/30 16:46:59, xiyuan wrote: > nit: insert a blank line between each function. Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:55: std::string app_id_; On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > nit: these two can be const. Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:60: base::FilePath icon_path_; On 2017/03/30 16:46:59, xiyuan wrote: > Can this and above members be private since we have accessors for them already? Some of them should be changeable from derived class, like name_ and icon_. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:63: class IconLoader; On 2017/03/30 16:46:59, xiyuan wrote: > remove since it is now KioskAppIconLoader ? Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:40: KioskAppIconLoader* icon_loader_; On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > KioskAppIconLoader* const icon_loader_; Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:55: base::Unretained(this), icon_path)); On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > base::Unretained(this) is unsafe in all this class: deleting |task_runner_| does > not guarantee that it will be deleted since something else might have a > reference to it, and so accessing it might fail. Use a weakptr for |this|. KioskAppIconLoader is self-owned and as I can see nothing else can have a reference to it. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:68: IconImageRequest* image_request = new IconImageRequest(task_runner_, this); On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > Please note that you are transferring ownership of |image_request| to > ImageDecoder, and will delete itself on completion. Done. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:69: ImageDecoder::Start(image_request, raw_icon_->data()); On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > Do you need the refcounted string? Why not transfer ownership of the data? > > ImageDecoder::Start(image_request, std::vector<uint8_t>(image_data.begin(), > image_data.end())) > > > Also, this class is going away: > https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h?type=cs&q... > > Can you use its replacement instead? Done for first comment. I think refactoring to use the replacement is out of scope of the CL. Most of this class was just extracted from KioskAppData::IconLoader, so I prefer to not do complex refactoring, rather then nits or bug fixes.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data.h (right): https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data.h:41: public base::SupportsWeakPtr<KioskAppData>, Remove this since we have |weak_factory_| now. https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:38: DCHECK(base::CreateDirectory(dir)); DCHECK could a removed for release build and cause base::CreateDirectory not being called. If we don't use CHECK, we can do if (!base::CreateDirectory(dir)) { NOTREACHED(); return; } But this would cause inconsistency in Local State. We need to handle missing icon files in such case, or at least adding a ERROR log.
https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:38: DCHECK(base::CreateDirectory(dir)); On 2017/03/30 18:40:08, xiyuan wrote: > DCHECK could a removed for release build and cause base::CreateDirectory not > being called. > > If we don't use CHECK, we can do > if (!base::CreateDirectory(dir)) { > NOTREACHED(); > return; > } > > But this would cause inconsistency in Local State. We need to handle missing > icon files in such case, or at least adding a ERROR log. I think when we work with FS it is better not force that something is created and written. Failing to write file is not stopper for Chrome. IMHO having ERROR logging here is enough. In ARC for example we handle such cases https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_l...
https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:55: base::Unretained(this), icon_path)); On 2017/03/30 18:22:08, Sergey Poromov wrote: > On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > > base::Unretained(this) is unsafe in all this class: deleting |task_runner_| > does > > not guarantee that it will be deleted since something else might have a > > reference to it, and so accessing it might fail. Use a weakptr for |this|. > > KioskAppIconLoader is self-owned and as I can see nothing else can have a > reference to it. Looked at the impl of this particular flavor of task runner. Even if you do delete the task runner itself, by the time this returns, the task has already been handed over to the persistent pool and you no longer have any way to notify it of cancellation. Given the recommendation I made in the other file to avoid having this class be self-owned, please move the weakptr to this class instead.
https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:78: (new KioskAppIconLoader(AsWeakPtr()))->Start(icon_path_); On 2017/03/30 18:22:07, Sergey Poromov wrote: > On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > > Is this the only caller? (Seems like it) > > > > If so, can you instead make this NOT delete itself? Instead make this class > own > > a std::unique_ptr<KioskAppIconLoader>. That way you don't need to have this > > class support WeakPtr. > > What if KioskAppIconLoader will live longer than KioskAppDataBase ? It's > possible if the icon will be loaded slower and callback in ImageRequest will be > called after destruction of KioskAppDataBase. Anyway, it seems like at least one > WeakPtr is needed - either here, or there. That's fine, you still _need_ the weak ptr in KioskAppIconLoader. https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:40: CHECK_EQ( Please remove all CHECKs from this change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:55: base::Unretained(this), icon_path)); On 2017/03/30 19:52:27, Luis Héctor Chávez wrote: > On 2017/03/30 18:22:08, Sergey Poromov wrote: > > On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > > > base::Unretained(this) is unsafe in all this class: deleting |task_runner_| > > does > > > not guarantee that it will be deleted since something else might have a > > > reference to it, and so accessing it might fail. Use a weakptr for |this|. > > > > KioskAppIconLoader is self-owned and as I can see nothing else can have a > > reference to it. > > Looked at the impl of this particular flavor of task runner. Even if you do > delete the task runner itself, by the time this returns, the task has already > been handed over to the persistent pool and you no longer have any way to notify > it of cancellation. > > Given the recommendation I made in the other file to avoid having this class be > self-owned, please move the weakptr to this class instead. Not sure why deleting |task_runner_| is relevant here. The KioskAppIconLoader manages its own lifetime. No one holds an reference to it. It runs until finished then update its Delegate with loaded and decoded image data. Making it owned by its Delegate (KioskAppData) works. But IMHO, it is not the most optimized. The image loading is a one time thing while KioskAppData would be held in memory indefinitely. Making loader a member of the Delegate seems a bit wasteful. This fire-and-forget pattern is used in a few places, e.g. SafeJsonParserImpl [1], IconLoader [2]. [1]: https://cs.chromium.org/chromium/src/components/safe_json/safe_json_parser_im... [2]: https://cs.chromium.org/chromium/src/chrome/browser/icon_loader_chromeos.cc?r...
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:78: (new KioskAppIconLoader(AsWeakPtr()))->Start(icon_path_); On 2017/03/30 19:57:22, Luis Héctor Chávez wrote: > On 2017/03/30 18:22:07, Sergey Poromov wrote: > > On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > > > Is this the only caller? (Seems like it) > > > > > > If so, can you instead make this NOT delete itself? Instead make this class > > own > > > a std::unique_ptr<KioskAppIconLoader>. That way you don't need to have this > > > class support WeakPtr. > > > > What if KioskAppIconLoader will live longer than KioskAppDataBase ? It's > > possible if the icon will be loaded slower and callback in ImageRequest will > be > > called after destruction of KioskAppDataBase. Anyway, it seems like at least > one > > WeakPtr is needed - either here, or there. > > That's fine, you still _need_ the weak ptr in KioskAppIconLoader. Acknowledged. https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:55: base::Unretained(this), icon_path)); On 2017/03/30 22:23:56, xiyuan wrote: > On 2017/03/30 19:52:27, Luis Héctor Chávez wrote: > > On 2017/03/30 18:22:08, Sergey Poromov wrote: > > > On 2017/03/30 16:49:54, Luis Héctor Chávez wrote: > > > > base::Unretained(this) is unsafe in all this class: deleting > |task_runner_| > > > does > > > > not guarantee that it will be deleted since something else might have a > > > > reference to it, and so accessing it might fail. Use a weakptr for |this|. > > > > > > KioskAppIconLoader is self-owned and as I can see nothing else can have a > > > reference to it. > > > > Looked at the impl of this particular flavor of task runner. Even if you do > > delete the task runner itself, by the time this returns, the task has already > > been handed over to the persistent pool and you no longer have any way to > notify > > it of cancellation. > > > > Given the recommendation I made in the other file to avoid having this class > be > > self-owned, please move the weakptr to this class instead. > > Not sure why deleting |task_runner_| is relevant here. The KioskAppIconLoader > manages its own lifetime. No one holds an reference to it. It runs until > finished then update its Delegate with loaded and decoded image data. > > Making it owned by its Delegate (KioskAppData) works. But IMHO, it is not the > most optimized. The image loading is a one time thing while KioskAppData would > be held in memory indefinitely. Making loader a member of the Delegate seems a > bit wasteful. > > This fire-and-forget pattern is used in a few places, e.g. SafeJsonParserImpl > [1], IconLoader [2]. > > [1]: > https://cs.chromium.org/chromium/src/components/safe_json/safe_json_parser_im... > [2]: > https://cs.chromium.org/chromium/src/chrome/browser/icon_loader_chromeos.cc?r... I agree that this pattern is suitable here, as KioskAppIconLoader should be alive only during the real load, no need to keep pointer to it after callbacks are called. On the other hand I understand Luis that it's safer to use weak_ptr here to not rely on what happens in task_runner. So, I implemented the suggested change in the latest Patch Set 7 - all other comments were resolved before in previous Patch Set 6. Also, KioskAppIconLoader can still be released in OnIconLoadSuccess()/Failure() callbacks - it will be the same behavior as before in terms of lifetime. https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data.h (right): https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data.h:41: public base::SupportsWeakPtr<KioskAppData>, On 2017/03/30 18:40:08, xiyuan wrote: > Remove this since we have |weak_factory_| now. Done. https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:38: DCHECK(base::CreateDirectory(dir)); On 2017/03/30 18:45:47, khmel wrote: > On 2017/03/30 18:40:08, xiyuan wrote: > > DCHECK could a removed for release build and cause base::CreateDirectory not > > being called. > > > > If we don't use CHECK, we can do > > if (!base::CreateDirectory(dir)) { > > NOTREACHED(); > > return; > > } > > > > But this would cause inconsistency in Local State. We need to handle missing > > icon files in such case, or at least adding a ERROR log. > > I think when we work with FS it is better not force that something is created > and written. Failing to write file is not stopper for Chrome. IMHO having ERROR > logging here is enough. In ARC for example we handle such cases > https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_l... Agree about incorrect DCHECK. I refactored CHECKs here to only LOG errors. I can see the only problem is that if IO error happens on icon saving, |icon_path_| will have not initialized value. But it was the same before as saving icon happened asynchronously with writing file name to prefs. The difference is LOG error instead of CHECK. WDYT, is the change correct? https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:40: CHECK_EQ( On 2017/03/30 19:57:22, Luis Héctor Chávez wrote: > Please remove all CHECKs from this change. Done. Is changing to LOG(ERROR) and return after sufficient for the cases here?
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:38: if (!base::PathExists(dir) || !base::CreateDirectory(dir)) { Would it be !base::PathExists(dir) && !base::CreateDirectory(dir)? https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:43: int wrote = base::WriteFile( tiny nit: here ant at L102, L107 you may use const.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:38: if (!base::PathExists(dir) || !base::CreateDirectory(dir)) { On 2017/03/31 14:37:13, khmel wrote: > Would it be !base::PathExists(dir) && !base::CreateDirectory(dir)? Sure! Thank you! My bad... https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:43: int wrote = base::WriteFile( On 2017/03/31 14:37:13, khmel wrote: > tiny nit: here ant at L102, L107 you may use const. Done.
https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:38: if (!base::PathExists(dir) || !base::CreateDirectory(dir)) { On 2017/03/31 14:37:13, khmel wrote: > Would it be !base::PathExists(dir) && !base::CreateDirectory(dir)? Good catch. It should be &&. https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.h (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:50: bool LoadFromDictionary(const base::DictionaryValue* dict); nit: prevailing style is to use & with const, i.e. const base::DictionaryValue& https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:55: std::string name_; nit: Put a comment about why these are in protected, e.g. "In protected section to allow derived classes to modify.". https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:59: std::unique_ptr<KioskAppIconLoader> kiosk_app_icon_loader_; nit: #include <memory> https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:83: if (!delegate_) nit: no need to check |delegate_| now since |delegate_| owns this and must out-live it. https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:96: delete this; Remove this since |delegate_| delegate owns it now. https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:30: Delegate() = default; No need to define a ctor for interface.
https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.h (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:50: bool LoadFromDictionary(const base::DictionaryValue* dict); On 2017/03/31 15:10:02, xiyuan wrote: > nit: prevailing style is to use & with const, i.e. const base::DictionaryValue& Done. https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:55: std::string name_; On 2017/03/31 15:10:02, xiyuan wrote: > nit: Put a comment about why these are in protected, > e.g. "In protected section to allow derived classes to modify.". Done. https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:59: std::unique_ptr<KioskAppIconLoader> kiosk_app_icon_loader_; On 2017/03/31 15:10:02, xiyuan wrote: > nit: #include <memory> Done. https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:83: if (!delegate_) On 2017/03/31 15:10:03, xiyuan wrote: > nit: no need to check |delegate_| now since |delegate_| owns this and must > out-live it. Done. https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:96: delete this; On 2017/03/31 15:10:03, xiyuan wrote: > Remove this since |delegate_| delegate owns it now. Done. https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:30: Delegate() = default; On 2017/03/31 15:10:03, xiyuan wrote: > No need to define a ctor for interface. Done.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Cool.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you all! Waiting for approval from Luis as he had many comments.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
sorry for the delay u_u. https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:40: CHECK_EQ( On 2017/03/31 12:56:25, Sergey Poromov wrote: > On 2017/03/30 19:57:22, Luis Héctor Chávez wrote: > > Please remove all CHECKs from this change. > > Done. Is changing to LOG(ERROR) and return after sufficient for the cases here? I believe so, I don't think we can do anything else. https://codereview.chromium.org/2778053002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc (right): https://codereview.chromium.org/2778053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc:45: if (wrote != static_cast<int>(image_data.size())) { nit: elide braces https://codereview.chromium.org/2778053002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:57: Delegate* const delegate_; nit: mention that the Delegate owns this object. https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc (right): https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc:56: CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)); nit: there's still a few straggling CHECKs in this change. https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.h (right): https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:23: class KioskAppDataBase : public KioskAppIconLoader::Delegate { nit: KioskAppDatabase. (I could not find any other instance of code in src/chrome with that capitalization). https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:53: task_runner_->PostTask(FROM_HERE, Sorry, should have been a bit clearer here. There's still a bit of a problem in this code because you are accessing "this" racily on two threads: the UI thread and the blocking pool. Would it be possible to not access "this" on the blocking pool at all? i.e. task_runner_->PostTask(FROM_HERE, base::Bind(LoadOnBlockingPool, icon_path, task_runner_)); .. void LoadOnBlockingPool(const base::FilePath& icon_path, XxxCallback success_callback, XxxCallback error_callback, scoped_refptr<base::SequencedTaskRunner> callback_task_runner) { ... // IconImageRequest will delete itself on completion of ImageDecoder callback. IconImageRequest* image_request = new IconImageRequest(callback_task_runner, success_callback, error_callback); ImageDecoder::Start(image_request, std::vector<uint8_t>(data.begin(), data.end())); } and then in the KioskAppIconLoader::IconImageRequest instead of calling ReportResultOnBlockingPool, you can directly do callback_task_runner_->PostTask(FROM_HERE, base::Bind(success_callback_, decoded_icon)); or callback_task_runner_->PostTask(FROM_HERE, base::Bind(error_callback_, FAILED_TO_DECODE)); Then again, it does not look like you're using the result at all, so you might was well just have a single function void KioskAppIconLoader::OnImageDecodingFinished(base::Optional<gfx::ImageSkia> result) { DCHECK_CURRENTLY_ON(BrowserThread::UI); if (result.has_value()) { delegate_->OnIconLoadSuccess(std::move(result.value())); } else { delegate_->OnIconLoadFailure(); } } and then add logging statements where you actually encountered the failure (either in L63 or in L34).
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc (right): https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc:56: CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)); On 2017/04/03 22:58:55, Luis Héctor Chávez wrote: > nit: there's still a few straggling CHECKs in this change. Done. https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.h (right): https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:23: class KioskAppDataBase : public KioskAppIconLoader::Delegate { On 2017/04/03 22:58:55, Luis Héctor Chávez wrote: > nit: KioskAppDatabase. (I could not find any other instance of code in > src/chrome with that capitalization). This is really Base class for KioskAppData classes, so it's really "...Base", not "...Database" https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc (right): https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc:53: task_runner_->PostTask(FROM_HERE, On 2017/04/03 22:58:55, Luis Héctor Chávez wrote: > Sorry, should have been a bit clearer here. There's still a bit of a problem in > this code because you are accessing "this" racily on two threads: the UI thread > and the blocking pool. Would it be possible to not access "this" on the blocking > pool at all? i.e. > > task_runner_->PostTask(FROM_HERE, > base::Bind(LoadOnBlockingPool, icon_path, task_runner_)); > > .. > > void LoadOnBlockingPool(const base::FilePath& icon_path, XxxCallback > success_callback, XxxCallback error_callback, > scoped_refptr<base::SequencedTaskRunner> callback_task_runner) { > ... > > // IconImageRequest will delete itself on completion of ImageDecoder callback. > IconImageRequest* image_request = new IconImageRequest(callback_task_runner, > success_callback, error_callback); > ImageDecoder::Start(image_request, > std::vector<uint8_t>(data.begin(), data.end())); > } > > and then in the KioskAppIconLoader::IconImageRequest instead of calling > ReportResultOnBlockingPool, you can directly do > > callback_task_runner_->PostTask(FROM_HERE, base::Bind(success_callback_, > decoded_icon)); > > or > > callback_task_runner_->PostTask(FROM_HERE, base::Bind(error_callback_, > FAILED_TO_DECODE)); > > Then again, it does not look like you're using the result at all, so you might > was well just have a single function > > void KioskAppIconLoader::OnImageDecodingFinished(base::Optional<gfx::ImageSkia> > result) { > DCHECK_CURRENTLY_ON(BrowserThread::UI); > > if (result.has_value()) { > delegate_->OnIconLoadSuccess(std::move(result.value())); > } else { > delegate_->OnIconLoadFailure(); > } > } > > and then add logging statements where you actually encountered the failure > (either in L63 or in L34). Done. Please check that I did it right. Also, I can't do manual test until next week - PatchSet 10 is the last tested, but now this PS differs a lot, so I'll probably wait until next week to land.
just a couple of more minor nits, otherwise lgtm! https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data_base.h (right): https://codereview.chromium.org/2778053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data_base.h:23: class KioskAppDataBase : public KioskAppIconLoader::Delegate { On 2017/04/06 10:29:58, Sergey Poromov wrote: > On 2017/04/03 22:58:55, Luis Héctor Chávez wrote: > > nit: KioskAppDatabase. (I could not find any other instance of code in > > src/chrome with that capitalization). > > This is really Base class for KioskAppData classes, so it's really "...Base", > not "...Database" D'Oh. Sorry for the noise. https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:148: for (auto& app : apps_) Ugh missed this in a previous review. nit: Add braces since the body is not a one-liner (but the if can still elide its braces). https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:38: typedef base::Callback<void(base::Optional<gfx::ImageSkia> result)> prefer using: using ResultCallback = base::Callback<...>; Also nit: #include "base/callback_forward.h" https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:50: // Delegate always live longer than this class as it's owned by delegate. nit: s/live/lives/
https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:148: for (auto& app : apps_) On 2017/04/06 16:16:50, Luis Héctor Chávez wrote: > Ugh missed this in a previous review. nit: Add braces since the body is not a > one-liner (but the if can still elide its braces). Done. https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h (right): https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:38: typedef base::Callback<void(base::Optional<gfx::ImageSkia> result)> On 2017/04/06 16:16:50, Luis Héctor Chávez wrote: > prefer using: > > using ResultCallback = base::Callback<...>; > > Also nit: #include "base/callback_forward.h" Done. https://codereview.chromium.org/2778053002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h:50: // Delegate always live longer than this class as it's owned by delegate. On 2017/04/06 16:16:50, Luis Héctor Chávez wrote: > nit: s/live/lives/ Done.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by poromov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org, xiyuan@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2778053002/#ps240001 (title: "some nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491911652402750,
"parent_rev": "a9ec6cb4b268d78bd6efc1327758e40067e4a304", "commit_rev":
"a5709293688a27bb196e33b9488193012f653f63"}
Message was sent while issue was closed.
Description was changed from ========== Fetch ARC Kiosk app name and icon from Android side. To provide appropriate and up-to-date app name and icon for ARC Kiosk apps on sign in screen, this data is updated from ARC side when Android container contains information about them, i.e.apps are installed in ARC Kiosk session. Before that name is provided by DMServer and default icon is used. ArcKioskAppData is extended from simple structure to a class, which caches the data from above to be used on sign in screen. IconLoader from KioskAppData is extracted as general KioskAppIconLoader for both Chrome and ARC apps. BUG=679928 TEST=Manual ========== to ========== Fetch ARC Kiosk app name and icon from Android side. To provide appropriate and up-to-date app name and icon for ARC Kiosk apps on sign in screen, this data is updated from ARC side when Android container contains information about them, i.e.apps are installed in ARC Kiosk session. Before that name is provided by DMServer and default icon is used. ArcKioskAppData is extended from simple structure to a class, which caches the data from above to be used on sign in screen. IconLoader from KioskAppData is extracted as general KioskAppIconLoader for both Chrome and ARC apps. BUG=679928 TEST=Manual Review-Url: https://codereview.chromium.org/2778053002 Cr-Commit-Position: refs/heads/master@{#463607} Committed: https://chromium.googlesource.com/chromium/src/+/a5709293688a27bb196e33b94881... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/a5709293688a27bb196e33b94881...
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2810973004/ by rouslan@chromium.org. The reason for reverting is: Appears to have broken the following tests on Ubuntu 14.04 memory bot: KioskUpdateTest.LaunchOfflineEnabledAppNoUpdate KioskUpdateTest.LaunchCachedNewVersionOfflineEnabledAppNoNetwork KioskUpdateTest.UpdateAppWithSharedModuleRemoveAllSecondaryApps KioskUpdateTest.UpdateMultiAppKioskRemoveOneApp KioskUpdateTest.LaunchOfflineEnabledAppNoNetwork KioskUpdateTest.UpdateMultiAppKioskAddOneApp KioskUpdateTest.PRE_IncompliantPlatformDelayInstall KioskUpdateTest.LaunchCachedOfflineEnabledAppNoNetwork KioskUpdateTest.PreserveLocalData KioskUpdateTest.PermissionChange KioskAppManagerTest.GetAutoLaunchAppRequiredPlatformVersion KioskAppManagerTest.IsPlatformCompliantWithApp KioskUpdateTest.LaunchAppWithUpdatedModule KioskAppManagerTest.UpdateAppDataFromProfile KioskUpdateTest.LaunchOfflineEnabledAppHasUpdate KioskAppManagerTest.ClearAppData KioskUpdateTest.UsbStickUpdateAppNoNetwork KioskAppManagerTest.UpdateAppDataFromCrx KioskAppManagerTest.LoadCached KioskTest.LaunchAppNetworkDown http://uberchromegw/i/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%2....
Message was sent while issue was closed.
https://codereview.chromium.org/2778053002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (right): https://codereview.chromium.org/2778053002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data.cc:432: kiosk_app_icon_loader_.release(); Oops.. This should be kiosk_app_icon_loader_.reset(). release() gives up the ownership but does not free the memory. I wish we had WARN_UNUSED_RESULT to tag the call. :(
Message was sent while issue was closed.
Description was changed from ========== Fetch ARC Kiosk app name and icon from Android side. To provide appropriate and up-to-date app name and icon for ARC Kiosk apps on sign in screen, this data is updated from ARC side when Android container contains information about them, i.e.apps are installed in ARC Kiosk session. Before that name is provided by DMServer and default icon is used. ArcKioskAppData is extended from simple structure to a class, which caches the data from above to be used on sign in screen. IconLoader from KioskAppData is extracted as general KioskAppIconLoader for both Chrome and ARC apps. BUG=679928 TEST=Manual Review-Url: https://codereview.chromium.org/2778053002 Cr-Commit-Position: refs/heads/master@{#463607} Committed: https://chromium.googlesource.com/chromium/src/+/a5709293688a27bb196e33b94881... ========== to ========== Fetch ARC Kiosk app name and icon from Android side. To provide appropriate and up-to-date app name and icon for ARC Kiosk apps on sign in screen, this data is updated from ARC side when Android container contains information about them, i.e.apps are installed in ARC Kiosk session. Before that name is provided by DMServer and default icon is used. ArcKioskAppData is extended from simple structure to a class, which caches the data from above to be used on sign in screen. IconLoader from KioskAppData is extracted as general KioskAppIconLoader for both Chrome and ARC apps. BUG=679928 TEST=Manual Review-Url: https://codereview.chromium.org/2778053002 Cr-Commit-Position: refs/heads/master@{#463607} Committed: https://chromium.googlesource.com/chromium/src/+/a5709293688a27bb196e33b94881... ==========
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Re-landed in http://crrev.com/2810053003 https://codereview.chromium.org/2778053002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/kiosk_app_data.cc (right): https://codereview.chromium.org/2778053002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/kiosk_app_data.cc:432: kiosk_app_icon_loader_.release(); On 2017/04/11 16:22:01, xiyuan wrote: > Oops.. This should be kiosk_app_icon_loader_.reset(). > > release() gives up the ownership but does not free the memory. I wish we had > WARN_UNUSED_RESULT to tag the call. :( Done. Thank you!
Message was sent while issue was closed.
Patchset #15 (id:280001) has been deleted
Message was sent while issue was closed.
Patchset #14 (id:260001) has been deleted |
