Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2468973006:
arc: Fix default app availability reporting. (Closed)
|
Created:
4 years, 1 month ago by khmel Modified:
4 years, 1 month ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, Matt Giuca Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Fix default app availability reporting.
This solves the case when App list model is created before
ArcAppListPrefs and notifications about defualt apps are
not dispatched.
BUG=641535
TEST=Locally on device.
Committed: https://crrev.com/fdae64e689c5c1424d5da8f827e9d1d10d905c1c
Cr-Commit-Position: refs/heads/master@{#429408}
Patch Set 1 #
Total comments: 8
Messages
Total messages: 15 (4 generated)
khmel@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan, PTAL https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:727: const bool was_tracked = tracked_apps_.count(app_id); IsRegistered does not fit well for the purpose to detect when app registration was notified or not. For default app and second login we may ignore OnAppRegistered callback.
https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:585: ready_apps_.erase(app_id); Do we need to erase from |tracked_apps_| too? https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:874: ready_apps_.clear(); Do we need to clear |tracked_apps_| here?
https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:585: ready_apps_.erase(app_id); On 2016/11/02 19:56:53, xiyuan wrote: > Do we need to erase from |tracked_apps_| too? RemoveApp above updates |tracked_apps_|. Here app becomes disabled, but still exists (OnAppReadyChanged is called) https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:874: ready_apps_.clear(); On 2016/11/02 19:56:53, xiyuan wrote: > Do we need to clear |tracked_apps_| here? No, this affects only ready state. If app is detected as removed then RemoveApp is called, where tracked_app_ is updated (line 823 called from 902).
lgtm https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:585: ready_apps_.erase(app_id); On 2016/11/02 20:00:56, khmel wrote: > On 2016/11/02 19:56:53, xiyuan wrote: > > Do we need to erase from |tracked_apps_| too? > > RemoveApp above updates |tracked_apps_|. Here app becomes disabled, but still > exists (OnAppReadyChanged is called) Acknowledged. https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:874: ready_apps_.clear(); On 2016/11/02 20:00:57, khmel wrote: > On 2016/11/02 19:56:53, xiyuan wrote: > > Do we need to clear |tracked_apps_| here? > > No, this affects only ready state. If app is detected as removed then RemoveApp > is called, where tracked_app_ is updated (line 823 called from 902). Acknowledged.
Thank you for review!
The CQ bit was checked by khmel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Drive-by: https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2468973006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:568: if (!tracked_apps_.count(app_id)) { if (tracked_apps_.insert(app_id).second) { ... } is a C++ way.
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
On 2016/11/02 21:40:23, commit-bot: I haz the power wrote: > Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== arc: Fix default app availability reporting. This solves the case when App list model is created before ArcAppListPrefs and notifications about defualt apps are not dispatched. BUG=641535 TEST=Locally on device. ========== to ========== arc: Fix default app availability reporting. This solves the case when App list model is created before ArcAppListPrefs and notifications about defualt apps are not dispatched. BUG=641535 TEST=Locally on device. Committed: https://crrev.com/fdae64e689c5c1424d5da8f827e9d1d10d905c1c Cr-Commit-Position: refs/heads/master@{#429408} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/fdae64e689c5c1424d5da8f827e9d1d10d905c1c Cr-Commit-Position: refs/heads/master@{#429408}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2477323002/ by kinaba@chromium.org. The reason for reverting is: It's breaking all ARC CTS autotests b/32660818, and I highly suspect it is also affecting the Chrome OS CQ crbug.com/662451. BUG=b/32660818. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
