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

Issue 2531543003: Resolve initialize/destory order between ArcService and ArcSessionManager. (Closed)

Created:
4 years ago by hidehiko
Modified:
4 years ago
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, yusukes+watch_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, hidehiko+watch_chromium.org, sdefresne+watchlist_chromium.org, achuith+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, khmel
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Resolve initialize/destory order between ArcService and ArcSessionManager. This CL ensures that the ArcSessionManager is alive during the whole each ArcService's life time, by merging two class initialization list in ArcSessionManager and ArcServiceLauncher. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. Committed: https://crrev.com/79a569780d9e7e234a0d6d8777b91db4f9b2b8d9 Cr-Commit-Position: refs/heads/master@{#434990}

Patch Set 1 #

Total comments: 12

Patch Set 2 : rebase #

Patch Set 3 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+130 lines, -126 lines) Patch
M chrome/browser/chromeos/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_service_launcher.h View 1 chunk +16 lines, -1 line 0 comments Download
M chrome/browser/chromeos/arc/arc_service_launcher.cc View 1 2 1 chunk +104 lines, -34 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc View 2 chunks +2 lines, -7 lines 0 comments Download
M chrome/browser/chromeos/login/session/chrome_session_manager.cc View 4 chunks +3 lines, -12 lines 0 comments Download
M chrome/browser/chromeos/login/session/user_session_manager.cc View 4 chunks +2 lines, -21 lines 0 comments Download
M components/arc/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M components/arc/DEPS View 1 chunk +0 lines, -1 line 0 comments Download
M components/arc/arc_service_manager.h View 2 chunks +0 lines, -7 lines 0 comments Download
M components/arc/arc_service_manager.cc View 4 chunks +2 lines, -42 lines 0 comments Download

Messages

Total messages: 16 (9 generated)
hidehiko
PTAL. lhchavez@: c/b/c/arc, components/arc xiyuan@: c/b/c/login. With this change, each ArcService implementation can assume that ...
4 years ago (2016-11-25 06:44:30 UTC) #4
Luis Héctor Chávez
lgtm with nits. https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc/arc_service_launcher.cc File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc/arc_service_launcher.cc#newcode45 chrome/browser/chromeos/arc/arc_service_launcher.cc:45: ArcServiceLauncher* g_arc_service_launcher = nullptr; Please mention ...
4 years ago (2016-11-28 22:17:41 UTC) #7
xiyuan
lgtm https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/login/session/user_session_manager.cc File chrome/browser/chromeos/login/session/user_session_manager.cc (left): https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/login/session/user_session_manager.cc#oldcode1844 chrome/browser/chromeos/login/session/user_session_manager.cc:1844: if (arc::ArcBridgeService::GetEnabled( On 2016/11/25 06:44:30, hidehiko wrote: > ...
4 years ago (2016-11-28 22:33:28 UTC) #8
hidehiko
Thank you for review. Submitting. https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc/arc_service_launcher.cc File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2531543003/diff/1/chrome/browser/chromeos/arc/arc_service_launcher.cc#newcode45 chrome/browser/chromeos/arc/arc_service_launcher.cc:45: ArcServiceLauncher* g_arc_service_launcher = nullptr; ...
4 years ago (2016-11-29 14:14:12 UTC) #9
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/2531543003/40001
4 years ago (2016-11-29 14:14:32 UTC) #12
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years ago (2016-11-29 15:15:37 UTC) #14
commit-bot: I haz the power
4 years ago (2016-11-29 15:18:17 UTC) #16
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/79a569780d9e7e234a0d6d8777b91db4f9b2b8d9
Cr-Commit-Position: refs/heads/master@{#434990}

Powered by Google App Engine
This is Rietveld 408576698