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

Issue 2520173002: arc: Fix duplicate icons in app launcher in case of crash. (Closed)

Created:
4 years ago by khmel
Modified:
4 years 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.

Description

arc: Fix duplicate icons in app launcher in case of crash. This CL initializes tracking items by existing app set on start in order to prevent race condition that leads to arc app created is called twice for some apps. TBR=xiyuan@chromium.org TEST=Extended unit_tests TEST=Manually on device with chrome://inducebrowsercrashforrealz BUG=b/32765086 BUG=667529 Committed: https://crrev.com/4e1e8774f3b147906917dbd4e50f30bfc7372ba6 Cr-Commit-Position: refs/heads/master@{#433980}

Patch Set 1 #

Patch Set 2 : clean up #

Total comments: 10

Patch Set 3 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -43 lines) Patch
M chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc View 1 3 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_test.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_test.cc View 1 2 4 chunks +35 lines, -28 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_unittest.cc View 1 2 9 chunks +67 lines, -12 lines 0 comments Download

Messages

Total messages: 24 (13 generated)
khmel
Hi Luis, PTAL
4 years ago (2016-11-21 23:49:23 UTC) #2
Luis Héctor Chávez
-the wrong lhchavez
4 years ago (2016-11-22 16:55:24 UTC) #4
Luis Héctor Chávez
lgtm with nits https://codereview.chromium.org/2520173002/diff/20001/chrome/browser/ui/app_list/arc/arc_app_test.cc File chrome/browser/ui/app_list/arc/arc_app_test.cc (right): https://codereview.chromium.org/2520173002/diff/20001/chrome/browser/ui/app_list/arc/arc_app_test.cc#newcode170 chrome/browser/ui/app_list/arc/arc_app_test.cc:170: arc_session_manager_.reset(); This was already destroyed a ...
4 years ago (2016-11-22 17:14:14 UTC) #5
khmel
Thank you Luis for review. I updated the code. I additionally moved AppModelRestart test above ...
4 years ago (2016-11-22 18:43:06 UTC) #6
Luis Héctor Chávez
lgtm if trybots are happy as long as you address the issue soon.
4 years ago (2016-11-22 19:04:24 UTC) #7
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/2520173002/40001
4 years ago (2016-11-22 20:55:34 UTC) #13
commit-bot: I haz the power
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_presubmit/builds/310823)
4 years ago (2016-11-22 21:03:19 UTC) #15
khmel
Adding Xiyuan TBR.
4 years ago (2016-11-22 21:07:26 UTC) #17
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/2520173002/40001
4 years ago (2016-11-22 21:08:26 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years ago (2016-11-22 21:14:01 UTC) #22
commit-bot: I haz the power
4 years ago (2016-11-22 21:16:01 UTC) #24
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/4e1e8774f3b147906917dbd4e50f30bfc7372ba6
Cr-Commit-Position: refs/heads/master@{#433980}

Powered by Google App Engine
This is Rietveld 408576698