Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2749973002:
arc: Fix Default icon issue when cached icon file is corrupted. (Closed)
|
Created:
3 years, 9 months ago by lgcheng Modified:
3 years, 9 months ago 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/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Default icon issue when cached icon file is corrupted.
Some crahes lead to app cached icon file corruption. Re send icon
request to ARC if bad icon is observed. Limit icon request to once per
app per scale per user session.
Bug=701979
Test=Unit test added.
Test=Manual test. Manual corrupted cached icon. See icon restored.
Review-Url: https://codereview.chromium.org/2749973002
Cr-Commit-Position: refs/heads/master@{#457294}
Committed: https://chromium.googlesource.com/chromium/src/+/f24408d3df3bf74f497111dd551d26a7509a07f4
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address Yury's comments. #
Total comments: 32
Patch Set 3 : Address comments. #
Total comments: 4
Patch Set 4 : Nits fix. #
Messages
Total messages: 25 (13 generated)
Description was changed from ========== Fix Default icon issue when cached icon file is corrupted. Some crahes lead to app cached icon file corruption. Re send icon request to ARC is bad icon is observed. Limit icon request to once per app per scale per user session. Bug= http://b/35947207 Test=Unit test added. Test=Manual test. Manual corrupted cached icon. See icon restored. ========== to ========== Fix Default icon issue when cached icon file is corrupted. Some crahes lead to app cached icon file corruption. Re send icon request to ARC is bad icon is observed. Limit icon request to once per app per scale per user session. Bug= http://b/35947207 Test=Unit test added. Test=Manual test. Manual corrupted cached icon. See icon restored. ==========
lgcheng@google.com changed reviewers: + khmel@chromium.org
Hi Yury, Would you PTAL when having a moment? Thanks!
In generally good solutions, thanks! Some top level comments: https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_icon.cc:283: if (prefs->IsIconRequestRecorded(app_id_, scale_factor)) Could you please move this logic into prefs. It would be nice to have centralized logic that handles request there. https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_icon.cc:334: if (read_result->request_to_install || read_result->unsafe_icon_data.empty()) Could you please move read_result->unsafe_icon_data.empty() check to place where it is actually used? https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:247: bool IsIconRequestRecorded(const std::string& app_id, nit: as commented above please move this to private and use this logic internally. https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:406: std::map<std::string, uint32_t> request_icon_record_; Can we use request_icon_deferred_ for this purpose, I feel it is duplicate by logic
lgcheng@google.com changed reviewers: + lhchavez@chromium.org, xiyuan@chromium.org
Thanks Yury for review. Comments address. Hi Xiyuan@ Would you PTAL c/b/ui/app_list Luis@ Would you PTAL components/arc Thanks! https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_icon.cc:283: if (prefs->IsIconRequestRecorded(app_id_, scale_factor)) On 2017/03/15 01:16:55, khmel wrote: > Could you please move this logic into prefs. It would be nice to have > centralized logic that handles request there. Done. https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_icon.cc:334: if (read_result->request_to_install || read_result->unsafe_icon_data.empty()) On 2017/03/15 01:16:55, khmel wrote: > Could you please move read_result->unsafe_icon_data.empty() check to place where > it is actually used? Done. https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:247: bool IsIconRequestRecorded(const std::string& app_id, On 2017/03/15 01:16:56, khmel wrote: > nit: as commented above please move this to private and use this logic > internally. Done. https://codereview.chromium.org/2749973002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:406: std::map<std::string, uint32_t> request_icon_record_; On 2017/03/15 01:16:56, khmel wrote: > Can we use request_icon_deferred_ for this purpose, I feel it is duplicate by > logic Done.
Do you want to merge this to a release branch? If so, this needs a crbug bug. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_icon.cc:313: // If unsafe_icon_data is empty typically means we have a file corruption on nit: |unsafe_icon_data| https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_icon.cc:315: request_to_install = request_to_install || unsafe_icon_data.empty(); nit: request_to_install |= unsafe_icon_data.empty() https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:326: return iter->second & 1 << scale_factor; nit: (1 << scale_factor). (even if the precedence is correct in this case, it'll avoid future readers from a trip to the operator precedence table :P). https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:355: request_icon_recorded_[app_id] | 1 << scale_factor; same here: (1 << scale_factor) https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:202: // Calls RequestIcon is not request is recorded. nit: if no request is recorded? https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:405: // Mainly two usage: nit: usages https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:410: // When Arc is disabled or the app is uninstalled the record will be erased. nit: ARC. "uninstalled, the record" https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1102: // request will not be re-sent without clear the request record in nit: s/clear/clearing/. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1141: // Remove the IconRequestRecord for app_id to observe the icon request for nit: |app_id|. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1175: // request will not be re-sent without clear the request record in nit: s/clear/clearing/ https://codereview.chromium.org/2749973002/diff/20001/components/arc/test/fak... File components/arc/test/fake_app_instance.h (right): https://codereview.chromium.org/2749973002/diff/20001/components/arc/test/fak... components/arc/test/fake_app_instance.h:130: std::string* png_data_as_string); can you convert this to a std::vector<uint8_t>*? std::string seems like a bad abstraction for binary data.
https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_icon.cc:315: request_to_install = request_to_install || unsafe_icon_data.empty(); nit: request_to_install |= unsafe_icon_data.empty(); https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_icon.cc:320: return base::WrapUnique(new ArcAppIcon::ReadResult( nit: base::MakeUnique<ArcAppIcon::ReadResult>(...) https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:867: auto deferred_icons = request_icon_recorded_.find(app_id); nit: deferred_icons -> pending_icons https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1367: Do we need to update |request_icon_recorded_| that the icon request is fulfilled? https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:202: // Calls RequestIcon is not request is recorded. nit: not -> no ?
Description was changed from ========== Fix Default icon issue when cached icon file is corrupted. Some crahes lead to app cached icon file corruption. Re send icon request to ARC is bad icon is observed. Limit icon request to once per app per scale per user session. Bug= http://b/35947207 Test=Unit test added. Test=Manual test. Manual corrupted cached icon. See icon restored. ========== to ========== Fix Default icon issue when cached icon file is corrupted. Some crahes lead to app cached icon file corruption. Re send icon request to ARC is bad icon is observed. Limit icon request to once per app per scale per user session. Bug=701979 Test=Unit test added. Test=Manual test. Manual corrupted cached icon. See icon restored. ==========
Thanks for comments! Issue addressed, PTAL. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_icon.cc:313: // If unsafe_icon_data is empty typically means we have a file corruption on On 2017/03/15 20:28:51, Luis Héctor Chávez wrote: > nit: |unsafe_icon_data| Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_icon.cc:315: request_to_install = request_to_install || unsafe_icon_data.empty(); On 2017/03/15 20:28:51, Luis Héctor Chávez wrote: > nit: request_to_install |= unsafe_icon_data.empty() Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_icon.cc:320: return base::WrapUnique(new ArcAppIcon::ReadResult( On 2017/03/15 20:30:26, xiyuan wrote: > nit: base::MakeUnique<ArcAppIcon::ReadResult>(...) Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:326: return iter->second & 1 << scale_factor; On 2017/03/15 20:28:51, Luis Héctor Chávez wrote: > nit: (1 << scale_factor). > > (even if the precedence is correct in this case, it'll avoid future readers from > a trip to the operator precedence table :P). Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:355: request_icon_recorded_[app_id] | 1 << scale_factor; On 2017/03/15 20:28:51, Luis Héctor Chávez wrote: > same here: (1 << scale_factor) Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:867: auto deferred_icons = request_icon_recorded_.find(app_id); On 2017/03/15 20:30:26, xiyuan wrote: > nit: deferred_icons -> pending_icons Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1367: On 2017/03/15 20:30:26, xiyuan wrote: > Do we need to update |request_icon_recorded_| that the icon request is > fulfilled? We intentionally keep the record in |request_icon_recorded_| to prevent re-request for icon to ARC in same user session. These requests will be sent to Android only when app state changes from unready to ready, which will not happen more then once unless user disables ARC or ARC crashes. In both cases we clear |request_icon_recorded_| in OnInstanceClosed(). https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:202: // Calls RequestIcon is not request is recorded. On 2017/03/15 20:30:26, xiyuan wrote: > nit: not -> no ? Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:405: // Mainly two usage: On 2017/03/15 20:28:51, Luis Héctor Chávez wrote: > nit: usages Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:410: // When Arc is disabled or the app is uninstalled the record will be erased. On 2017/03/15 20:28:51, Luis Héctor Chávez wrote: > nit: ARC. "uninstalled, the record" Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1102: // request will not be re-sent without clear the request record in On 2017/03/15 20:28:52, Luis Héctor Chávez wrote: > nit: s/clear/clearing/. Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1141: // Remove the IconRequestRecord for app_id to observe the icon request for On 2017/03/15 20:28:51, Luis Héctor Chávez wrote: > nit: |app_id|. Done. https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1175: // request will not be re-sent without clear the request record in On 2017/03/15 20:28:51, Luis Héctor Chávez wrote: > nit: s/clear/clearing/ Done. https://codereview.chromium.org/2749973002/diff/20001/components/arc/test/fak... File components/arc/test/fake_app_instance.h (right): https://codereview.chromium.org/2749973002/diff/20001/components/arc/test/fak... components/arc/test/fake_app_instance.h:130: std::string* png_data_as_string); On 2017/03/15 20:28:52, Luis Héctor Chávez wrote: > can you convert this to a std::vector<uint8_t>*? std::string seems like a bad > abstraction for binary data. In fact the std::string* png_data_as_string is not need in the test I added. I remove the last argument. But for the existing functions, base::ReadFileToString is used to read binary, so I'd not touch std::string* png_data_as_string there. WDYT?
lgtm with a couple nits i missed the previous round. https://codereview.chromium.org/2749973002/diff/20001/components/arc/test/fak... File components/arc/test/fake_app_instance.h (right): https://codereview.chromium.org/2749973002/diff/20001/components/arc/test/fak... components/arc/test/fake_app_instance.h:130: std::string* png_data_as_string); On 2017/03/15 22:05:34, lgcheng wrote: > On 2017/03/15 20:28:52, Luis Héctor Chávez wrote: > > can you convert this to a std::vector<uint8_t>*? std::string seems like a bad > > abstraction for binary data. > > In fact the std::string* png_data_as_string is not need in the test I added. I > remove the last argument. > But for the existing functions, base::ReadFileToString is used to read binary, > so I'd not touch std::string* png_data_as_string there. WDYT? I see, the removal of this parameter sgtm. The other usage is kind of unfortunate though :/ I think I'm begrudgingly fine with leaving that one as-is since this is testing-only code. https://codereview.chromium.org/2749973002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2749973002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:353: // when icon file decode failure is sufferd in case app sends bad icon. nit: suffered https://codereview.chromium.org/2749973002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:354: request_icon_recorded_[app_id] = ah, missed this |= too.
lgtm https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2749973002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1367: On 2017/03/15 22:05:33, lgcheng wrote: > On 2017/03/15 20:30:26, xiyuan wrote: > > Do we need to update |request_icon_recorded_| that the icon request is > > fulfilled? > > We intentionally keep the record in |request_icon_recorded_| to prevent > re-request for icon to ARC in same user session. > > These requests will be sent to Android only when app state changes from unready > to ready, which will not happen more then once unless user disables ARC or ARC > crashes. In both cases we clear |request_icon_recorded_| in OnInstanceClosed(). Acknowledged.
lgtm
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Nits fixed. Thanks for review! https://codereview.chromium.org/2749973002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2749973002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:353: // when icon file decode failure is sufferd in case app sends bad icon. On 2017/03/15 22:49:50, Luis Héctor Chávez wrote: > nit: suffered Done. https://codereview.chromium.org/2749973002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:354: request_icon_recorded_[app_id] = On 2017/03/15 22:49:50, Luis Héctor Chávez wrote: > ah, missed this |= too. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix Default icon issue when cached icon file is corrupted. Some crahes lead to app cached icon file corruption. Re send icon request to ARC is bad icon is observed. Limit icon request to once per app per scale per user session. Bug=701979 Test=Unit test added. Test=Manual test. Manual corrupted cached icon. See icon restored. ========== to ========== Fix Default icon issue when cached icon file is corrupted. Some crahes lead to app cached icon file corruption. Re send icon request to ARC if bad icon is observed. Limit icon request to once per app per scale per user session. Bug=701979 Test=Unit test added. Test=Manual test. Manual corrupted cached icon. See icon restored. ==========
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 lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org, khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2749973002/#ps60001 (title: "Nits fix.")
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": 60001, "attempt_start_ts": 1489626250052890,
"parent_rev": "96f053fa8bf0334516e965764f35f4de92444e12", "commit_rev":
"f24408d3df3bf74f497111dd551d26a7509a07f4"}
Message was sent while issue was closed.
Description was changed from ========== Fix Default icon issue when cached icon file is corrupted. Some crahes lead to app cached icon file corruption. Re send icon request to ARC if bad icon is observed. Limit icon request to once per app per scale per user session. Bug=701979 Test=Unit test added. Test=Manual test. Manual corrupted cached icon. See icon restored. ========== to ========== Fix Default icon issue when cached icon file is corrupted. Some crahes lead to app cached icon file corruption. Re send icon request to ARC if bad icon is observed. Limit icon request to once per app per scale per user session. Bug=701979 Test=Unit test added. Test=Manual test. Manual corrupted cached icon. See icon restored. Review-Url: https://codereview.chromium.org/2749973002 Cr-Commit-Position: refs/heads/master@{#457294} Committed: https://chromium.googlesource.com/chromium/src/+/f24408d3df3bf74f497111dd551d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f24408d3df3bf74f497111dd551d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
