From 2f9be5290ce3a682738a68cd337872c031a5f30d Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Thu, 16 May 2019 15:01:53 -0700 Subject: Clean up STOPSHIP comments for role. Bug: 132909319 Test: presubmit Change-Id: I3cc812563515b08cfc79f97df1afb658b9c8a68b --- .../android/packageinstaller/role/model/PreferredActivity.java | 3 ++- .../packageinstaller/role/model/RequiredContentProvider.java | 2 +- src/com/android/packageinstaller/role/model/Role.java | 9 +++++++-- src/com/android/packageinstaller/role/model/SmsRoleBehavior.java | 5 +++-- .../packageinstaller/role/service/RoleControllerServiceImpl.java | 4 ++-- .../packageinstaller/role/ui/DefaultAppChildFragment.java | 1 - .../android/packageinstaller/role/ui/RequestRoleActivity.java | 1 - .../android/packageinstaller/role/ui/RequestRoleFragment.java | 1 - .../packageinstaller/role/ui/SpecialAppAccessChildFragment.java | 1 - 9 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/com/android/packageinstaller/role/model/PreferredActivity.java b/src/com/android/packageinstaller/role/model/PreferredActivity.java index f5004002..5062e9e0 100644 --- a/src/com/android/packageinstaller/role/model/PreferredActivity.java +++ b/src/com/android/packageinstaller/role/model/PreferredActivity.java @@ -72,8 +72,9 @@ public class PreferredActivity { PackageManager packageManager = context.getPackageManager(); ComponentName packageActivity = mActivity.getQualifyingComponentForPackage( packageName, context); - // TODO: STOPSHIP: Race condition, what if packageActivity became null? Just don't crash? if (packageActivity == null) { + // We might be running into some race condition here, but we can't do anything about it. + // This should be handled by a future reconciliation started by the package change. return; } diff --git a/src/com/android/packageinstaller/role/model/RequiredContentProvider.java b/src/com/android/packageinstaller/role/model/RequiredContentProvider.java index 76fd3944..69b5a64a 100644 --- a/src/com/android/packageinstaller/role/model/RequiredContentProvider.java +++ b/src/com/android/packageinstaller/role/model/RequiredContentProvider.java @@ -60,7 +60,7 @@ public class RequiredContentProvider extends RequiredComponent { @Nullable @Override protected String getComponentPermission(@NonNull ResolveInfo resolveInfo) { - // TODO: STOPSHIP: Which permission? Or both? + // TODO: Which permission? Or both? //return resolveInfo.providerInfo.readPermission; throw new UnsupportedOperationException(); } diff --git a/src/com/android/packageinstaller/role/model/Role.java b/src/com/android/packageinstaller/role/model/Role.java index 615480ec..58cbf622 100644 --- a/src/com/android/packageinstaller/role/model/Role.java +++ b/src/com/android/packageinstaller/role/model/Role.java @@ -525,7 +525,9 @@ public class Role { return false; } - // TODO: STOPSHIP: Check for disabled packages? + if (!applicationInfo.enabled) { + return false; + } if (applicationInfo.isInstantApp()) { return false; @@ -613,7 +615,10 @@ public class Role { appOp.revoke(packageName, context); } - // TODO: STOPSHIP: Revoke preferred activities? + // TODO: Revoke preferred activities? But this is unnecessary for most roles using it as + // they have fallback holders. Moreover, clearing the preferred activity might result in + // other system components listening to preferred activity change get notified for the + // wrong thing when we are removing a exclusive role holder for adding another. if (mBehavior != null) { mBehavior.revoke(this, packageName, context); diff --git a/src/com/android/packageinstaller/role/model/SmsRoleBehavior.java b/src/com/android/packageinstaller/role/model/SmsRoleBehavior.java index 2cd2080a..606b95c8 100644 --- a/src/com/android/packageinstaller/role/model/SmsRoleBehavior.java +++ b/src/com/android/packageinstaller/role/model/SmsRoleBehavior.java @@ -66,8 +66,9 @@ public class SmsRoleBehavior implements RoleBehavior { return defaultPackageName; } - // TODO: STOPSHIP: This was the previous behavior, however this allows any third-party app - // to suddenly become the default SMS app and get the permissions. + // TODO(b/132916161): This was the previous behavior, however this may allow any third-party + // app to suddenly become the default SMS app and get the permissions, if no system default + // SMS app is available. List qualifyingPackageNames = role.getQualifyingPackagesAsUser( Process.myUserHandle(), context); return CollectionUtils.firstOrNull(qualifyingPackageNames); diff --git a/src/com/android/packageinstaller/role/service/RoleControllerServiceImpl.java b/src/com/android/packageinstaller/role/service/RoleControllerServiceImpl.java index 2c462845..3a0b1e01 100644 --- a/src/com/android/packageinstaller/role/service/RoleControllerServiceImpl.java +++ b/src/com/android/packageinstaller/role/service/RoleControllerServiceImpl.java @@ -45,7 +45,7 @@ public class RoleControllerServiceImpl extends RoleControllerService { private static final String LOG_TAG = RoleControllerServiceImpl.class.getSimpleName(); - // TODO: STOPSHIP: Turn off debugging before we ship. + // STOPSHIP: Turn off debugging before we ship. private static final boolean DEBUG = true; private RoleManager mRoleManager; @@ -53,6 +53,7 @@ public class RoleControllerServiceImpl extends RoleControllerService { @Override public void onCreate() { super.onCreate(); + mRoleManager = getSystemService(RoleManager.class); } @@ -335,7 +336,6 @@ public class RoleControllerServiceImpl extends RoleControllerService { } if (applicationInfo != null) { - // TODO: STOPSHIP: Pass in appropriate arguments. role.revoke(packageName, dontKillApp, false, this); } diff --git a/src/com/android/packageinstaller/role/ui/DefaultAppChildFragment.java b/src/com/android/packageinstaller/role/ui/DefaultAppChildFragment.java index b64ea46f..675f35d8 100644 --- a/src/com/android/packageinstaller/role/ui/DefaultAppChildFragment.java +++ b/src/com/android/packageinstaller/role/ui/DefaultAppChildFragment.java @@ -217,7 +217,6 @@ public class DefaultAppChildFragment