arc: UserManager/SessionManager usage in ARC services [chromium/src : main]
0 views
Skip to first unread message
Achuith Bhandarkar (Gerrit)
12:18 AM (20 hours ago) 12:18 AM
to Jun Ishiguro, Hidehiko Abe, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, khmel...@chromium.org, yhanada+...@chromium.org
Attention needed from Jun Ishiguro
Achuith Bhandarkar added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Jun Ishiguro
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jun Ishiguro (Gerrit)
12:56 AM (19 hours ago) 12:56 AM
to Achuith Bhandarkar, Hidehiko Abe, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, khmel...@chromium.org, yhanada+...@chromium.org
Attention needed from Achuith Bhandarkar
Jun Ishiguro voted and added 6 comments![]( "Open in Gerrit")
Votes added by Jun Ishiguro
| Code-Review | +1 |
6 comments
Patchset-level comments
Jun Ishiguro . resolved
LGTM with a few requests
File chrome/browser/ash/arc/auth/arc_auth_service.cc
Line 154, Patchset 5 (Parent):
// (Primary) account that participates in multi-signin.Jun Ishiguro . unresolved
nit: how about preserving this comment?
File chrome/browser/ash/arc/wallpaper/arc_wallpaper_service.cc
Line 139, Patchset 5 (Latest):
DCHECK(primary_user);Jun Ishiguro . unresolved
Let's use CHECK instead of DCHECK.
File chrome/browser/ash/arc/wallpaper/arc_wallpaper_service_unittest.cc
Line 122, Patchset 5 (Latest):
protected:Jun Ishiguro . unresolved
Can we keep protected before private?
Line 125, Patchset 5 (Latest):
raw_ptr<arc::ArcWallpaperService, DanglingUntriaged> service_ = nullptr;Jun Ishiguro . unresolved
Nice to reset this pointer on TearDown and drop DanglingUntriaged.
Related details
Attention is currently required from:
- Achuith Bhandarkar
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
1:56 PM (6 hours ago) 1:56 PM
to Jun Ishiguro, Hidehiko Abe, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, khmel...@chromium.org, yhanada+...@chromium.org
Achuith Bhandarkar voted and added 6 comments![]( "Open in Gerrit")
Votes added by Achuith Bhandarkar
| Commit-Queue | +2 |
6 comments
Patchset-level comments
File chrome/browser/ash/arc/auth/arc_auth_service.cc
Line 154, Patchset 5 (Parent):
// (Primary) account that participates in multi-signin.Jun Ishiguro . resolved
nit: how about preserving this comment?
Achuith Bhandarkar
Done
File chrome/browser/ash/arc/wallpaper/arc_wallpaper_service.cc
Let's use CHECK instead of DCHECK.
Achuith Bhandarkar
Done
File chrome/browser/ash/arc/wallpaper/arc_wallpaper_service_unittest.cc
Can we keep protected before private?
Achuith Bhandarkar
Done
Line 125, Patchset 5:
raw_ptr<arc::ArcWallpaperService, DanglingUntriaged> service_ = nullptr;Jun Ishiguro . resolved
Nice to reset this pointer on TearDown and drop DanglingUntriaged.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
2:51 PM (5 hours ago) 2:51 PM
to Achuith Bhandarkar, Jun Ishiguro, Hidehiko Abe, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, khmel...@chromium.org, yhanada+...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ash/arc/wallpaper/arc_wallpaper_service_unittest.cc
Insertions: 7, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/ash/arc/auth/arc_auth_service.cc
Insertions: 4, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/ash/arc/wallpaper/arc_wallpaper_service.cc
Insertions: 4, Deletions: 3.
The diff is too large to show. Please review the diff.
```
Change information
Commit message:
arc: UserManager/SessionManager usage in ARC services
- Replace user_manager::UserManager::GetPrimaryUser() with
session_manager::SessionManager::GetPrimarySession() in
ArcAuthService, ArcBluetoothBridge, ArcWallpaperService, and Arc UMA.
- Replace deprecated FakeChromeUserManager, ScopedUserManager, and
manual SessionManager setup in unit tests with
ash::test::TestUserSessionManager.
- Enforce strict LIFO teardown order, check nodiscard returns with
ASSERT_TRUE, and align SetUp() initialization order with class member
declaration order across unit tests.
TAG=agy
CONV=95bb43ef-75f9-4667-81fe-c7efbc76f892
Bug: 531278783
Change-Id: I577278a7a253628bf4b720f53a6c40ad84697aaf
Reviewed-by: Jun Ishiguro <ju...@google.com>
Commit-Queue: Achuith Bhandarkar <ach...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1659734}
Files:
- M chrome/browser/ash/arc/arc_optin_uma.cc
- M chrome/browser/ash/arc/auth/BUILD.gn
- M chrome/browser/ash/arc/auth/arc_auth_service.cc
- M chrome/browser/ash/arc/bluetooth/BUILD.gn
- M chrome/browser/ash/arc/bluetooth/arc_bluetooth_bridge.cc
- M chrome/browser/ash/arc/wallpaper/BUILD.gn
- M chrome/browser/ash/arc/wallpaper/arc_wallpaper_service.cc
- M chrome/browser/ash/arc/wallpaper/arc_wallpaper_service_unittest.cc
Change size: M
Delta: 8 files changed, 65 insertions(+), 33 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Jun Ishiguro
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages