Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(345)

Issue 2810053003: Reland of Fetch ARC Kiosk app name and icon from Android side. (Closed)

Created:
3 years, 8 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.

Description

Reland of Fetch ARC Kiosk app name and icon from Android side. (patchset #1 id:1 of https://codereview.chromium.org/2810973004/ ) Reason for revert: Fix memory leak and re-land. Original issue's description: > Revert of Fetch ARC Kiosk app name and icon from Android side. (patchset #13 id:240001 of https://codereview.chromium.org/2778053002/ ) > > Reason for revert: > 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%20LSan%20Tests%20%281%29/builds/20548 > > Original issue's description: > > 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/+/a5709293688a27bb196e33b9488193012f653f63 > > TBR=khmel@chromium.org,xiyuan@chromium.org,lhchavez@chromium.org,poromov@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=679928 > > Review-Url: https://codereview.chromium.org/2810973004 > Cr-Commit-Position: refs/heads/master@{#463648} > Committed: https://chromium.googlesource.com/chromium/src/+/8150e6af3dc7dead6b5f1a126bede6064036b43a TBR=khmel@chromium.org,xiyuan@chromium.org,lhchavez@chromium.org,rouslan@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=679928 Review-Url: https://codereview.chromium.org/2810053003 Cr-Commit-Position: refs/heads/master@{#464004} Committed: https://chromium.googlesource.com/chromium/src/+/8a4add0c160a539fba0a3ddd47b7d2b05d0d5e4d

Patch Set 1 #

Patch Set 2 : fix memory leak, reland the CL #

Unified diffs Side-by-side diffs Delta from patch set Stats (+686 lines, -343 lines) Patch
M chrome/browser/chromeos/BUILD.gn View 1 2 chunks +6 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.h View 1 chunk +47 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_data.cc View 1 1 chunk +82 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h View 1 5 chunks +13 lines, -28 lines 0 comments Download
M chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc View 1 9 chunks +42 lines, -30 lines 0 comments Download
M chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager_browsertest.cc View 1 4 chunks +23 lines, -18 lines 0 comments Download
M chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h View 1 5 chunks +9 lines, -1 line 0 comments Download
M chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc View 1 4 chunks +28 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/app_mode/kiosk_app_data.h View 1 7 chunks +8 lines, -25 lines 0 comments Download
M chrome/browser/chromeos/app_mode/kiosk_app_data.cc View 1 13 chunks +43 lines, -225 lines 0 comments Download
A chrome/browser/chromeos/app_mode/kiosk_app_data_base.h View 1 chunk +77 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/app_mode/kiosk_app_data_base.cc View 1 chunk +130 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.h View 1 chunk +65 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/app_mode/kiosk_app_icon_loader.cc View 1 chunk +97 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/app_mode/kiosk_app_manager.h View 1 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/chromeos/app_mode/kiosk_app_manager.cc View 1 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc View 1 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc View 1 1 chunk +14 lines, -8 lines 0 comments Download

Messages

Total messages: 11 (7 generated)
Sergey Poromov
Created Reland of Fetch ARC Kiosk app name and icon from Android side.
3 years, 8 months ago (2017-04-12 12:19:37 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2810053003/260001
3 years, 8 months ago (2017-04-12 12:40:44 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (id:260001) as https://chromium.googlesource.com/chromium/src/+/8a4add0c160a539fba0a3ddd47b7d2b05d0d5e4d
3 years, 8 months ago (2017-04-12 12:42:32 UTC) #10
Luis Héctor Chávez
3 years, 8 months ago (2017-04-12 15:04:32 UTC) #11
Message was sent while issue was closed.
for the next time, can you go through the CQ normally instead of reverting the
revert and chumping it? That is very dangerous since it skips all checks:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Also, try to avoid doing a rebase and the actual fix in the same patchset. Do
the rebase first, upload that, and then fix whatever needs to be fixed.

otherwise lgtm.

Powered by Google App Engine
This is Rietveld 408576698