Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896973002:
arc: Invalide app icon on package update. (Closed)
|
Created:
3 years, 7 months ago by khmel Modified:
3 years, 7 months ago Reviewers:
xiyuan CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, Matt Giuca Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Invalide app icon on package update.
ARC app icons need to be invalidated and app icon in app launcher and in
shelf should be automatically updated.
BUG=725305
TEST=Manually + unit tests.
Review-Url: https://codereview.chromium.org/2896973002
Cr-Commit-Position: refs/heads/master@{#474443}
Committed: https://chromium.googlesource.com/chromium/src/+/1f90c7dc31ae659ff560baebcabfe24c3e036312
Patch Set 1 #
Total comments: 4
Patch Set 2 : pref based solution #
Messages
Total messages: 15 (5 generated)
khmel@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan, PTAL Thanks!
https://codereview.chromium.org/2896973002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2896973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1119: base::PostTaskWithTraitsAndReplyWithResult( Why do we need InvalidateAppIconFromFileThread? Can we simplify to just call MaybeRequestIcon ? The icon files will be overwritten anyway. It seems not necessary to delete it first. Or is the intention to only update existing icons for an app? Why do we care? Shouldn't we get all icons of the refreshed apps?
https://codereview.chromium.org/2896973002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2896973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1119: base::PostTaskWithTraitsAndReplyWithResult( On 2017/05/24 15:51:15, xiyuan wrote: > Why do we need InvalidateAppIconFromFileThread? Can we simplify to just call > MaybeRequestIcon ? The icon files will be overwritten anyway. It seems not > necessary to delete it first. > > Or is the intention to only update existing icons for an app? Why do we care? > Shouldn't we get all icons of the refreshed apps? My intention was to remove deprecated icons physically. For example if app updated and for some reason icon was not requested from Android we would stay with outdated icons and on next start we would wrongly consider that icons are up to date
https://codereview.chromium.org/2896973002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2896973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1119: base::PostTaskWithTraitsAndReplyWithResult( On 2017/05/24 15:54:52, khmel wrote: > On 2017/05/24 15:51:15, xiyuan wrote: > > Why do we need InvalidateAppIconFromFileThread? Can we simplify to just call > > MaybeRequestIcon ? The icon files will be overwritten anyway. It seems not > > necessary to delete it first. > > > > Or is the intention to only update existing icons for an app? Why do we care? > > Shouldn't we get all icons of the refreshed apps? > > My intention was to remove deprecated icons physically. For example if app > updated and for some reason icon was not requested from Android we would stay > with outdated icons and on next start we would wrongly consider that icons are > up to date Thanks for the clarification. IMO, it is probably better to add a bool to app prefs to indicate icons need refresh instead of relying on the FS update. File deletion in background thread is more fragile than pref value in prefs file in case of browser crash/shutdown.
Thanks for idea! PTAL https://codereview.chromium.org/2896973002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2896973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1119: base::PostTaskWithTraitsAndReplyWithResult( On 2017/05/24 16:12:08, xiyuan wrote: > On 2017/05/24 15:54:52, khmel wrote: > > On 2017/05/24 15:51:15, xiyuan wrote: > > > Why do we need InvalidateAppIconFromFileThread? Can we simplify to just call > > > MaybeRequestIcon ? The icon files will be overwritten anyway. It seems not > > > necessary to delete it first. > > > > > > Or is the intention to only update existing icons for an app? Why do we > care? > > > Shouldn't we get all icons of the refreshed apps? > > > > My intention was to remove deprecated icons physically. For example if app > > updated and for some reason icon was not requested from Android we would stay > > with outdated icons and on next start we would wrongly consider that icons are > > up to date > > Thanks for the clarification. IMO, it is probably better to add a bool to app > prefs to indicate icons need refresh instead of relying on the FS update. File > deletion in background thread is more fragile than pref value in prefs file in > case of browser crash/shutdown. Well, that is much better :)
lgtm
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...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495662148620130,
"parent_rev": "2b6e2e8fb771e659e88885302e1cd83cc6252d68", "commit_rev":
"1f90c7dc31ae659ff560baebcabfe24c3e036312"}
Message was sent while issue was closed.
Description was changed from ========== arc: Invalide app icon on package update. ARC app icons need to be invalidated and app icon in app launcher and in shelf should be automatically updated. BUG=725305 TEST=Manually + unit tests. ========== to ========== arc: Invalide app icon on package update. ARC app icons need to be invalidated and app icon in app launcher and in shelf should be automatically updated. BUG=725305 TEST=Manually + unit tests. Review-Url: https://codereview.chromium.org/2896973002 Cr-Commit-Position: refs/heads/master@{#474443} Committed: https://chromium.googlesource.com/chromium/src/+/1f90c7dc31ae659ff560baebcabf... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1f90c7dc31ae659ff560baebcabf...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 474443 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 #2 id:20001) has been created in https://codereview.chromium.org/2896323004/ by khmel@chromium.org. The reason for reverting is: Will take a look mem issue.
Message was sent while issue was closed.
Patchset #3 (id:40001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
