From 94ed570f3527e693c2874069c872054970599ec7 Mon Sep 17 00:00:00 2001 From: Mark Punzalan Date: Wed, 30 Mar 2022 18:19:54 +0000 Subject: [PATCH 1/2] Reduce duplicate UiTranslationStateCallback calls. We don't invoke methods when translation state doesn't change (e.g., for consecutive startTranslation() calls). Bug: 192331240 Test: atest UiTranslationManagerTest Change-Id: I96def427b1fcabb7b83cb188c4b1ee017b5882a9 --- .../UiTranslationStateCallback.java | 28 ++- .../TranslationManagerServiceImpl.java | 212 ++++++++++++++---- 2 files changed, 193 insertions(+), 47 deletions(-) diff --git a/core/java/android/view/translation/UiTranslationStateCallback.java b/core/java/android/view/translation/UiTranslationStateCallback.java index 3ccca5f5290fb..96b6f8ca6c4a2 100644 --- a/core/java/android/view/translation/UiTranslationStateCallback.java +++ b/core/java/android/view/translation/UiTranslationStateCallback.java @@ -25,14 +25,28 @@ import java.util.concurrent.Executor; * Callback for listening to UI Translation state changes. See {@link * UiTranslationManager#registerUiTranslationStateCallback(Executor, UiTranslationStateCallback)}. *

- * Prior to Android version {@link android.os.Build.VERSION_CODES#TIRAMISU}, callback methods - * without {@code packageName} are invoked. Apps with minSdkVersion lower than {@link - * android.os.Build.VERSION_CODES#TIRAMISU} must implement those methods if they want to - * handle the events. + * Prior to Android version {@link android.os.Build.VERSION_CODES#TIRAMISU}: + *

*

- * In Android version {@link android.os.Build.VERSION_CODES#TIRAMISU} and later, if both methods - * with and without {@code packageName} are implemented (e.g., {@link #onFinished()} and {@link - * #onFinished(String)}, only the one with {@code packageName} will be called. + * In Android version {@link android.os.Build.VERSION_CODES#TIRAMISU} and later: + *

*/ public interface UiTranslationStateCallback { diff --git a/services/translation/java/com/android/server/translation/TranslationManagerServiceImpl.java b/services/translation/java/com/android/server/translation/TranslationManagerServiceImpl.java index 97d5215213a9a..5e13031763cd5 100644 --- a/services/translation/java/com/android/server/translation/TranslationManagerServiceImpl.java +++ b/services/translation/java/com/android/server/translation/TranslationManagerServiceImpl.java @@ -41,10 +41,10 @@ import android.os.RemoteCallbackList; import android.os.RemoteException; import android.os.ResultReceiver; import android.service.translation.TranslationServiceInfo; +import android.util.ArrayMap; import android.util.ArraySet; import android.util.Log; import android.util.Slog; -import android.util.SparseArray; import android.view.autofill.AutofillId; import android.view.inputmethod.InputMethodInfo; import android.view.translation.ITranslationServiceCallback; @@ -71,7 +71,8 @@ import java.lang.ref.WeakReference; import java.util.List; final class TranslationManagerServiceImpl extends - AbstractPerUserSystemService { + AbstractPerUserSystemService + implements IBinder.DeathRecipient { private static final String TAG = "TranslationManagerServiceImpl"; @SuppressLint("IsLoggableTagLength") @@ -100,10 +101,10 @@ final class TranslationManagerServiceImpl extends private final ArraySet mWaitingFinishedCallbackActivities = new ArraySet<>(); /** - * Key is translated activity uid, value is the specification and state for the translation. + * Key is translated activity token, value is the specification and state for the translation. */ @GuardedBy("mLock") - private final SparseArray mActiveTranslations = new SparseArray<>(); + private final ArrayMap mActiveTranslations = new ArrayMap<>(); protected TranslationManagerServiceImpl( @NonNull TranslationManagerService master, @@ -262,46 +263,159 @@ final class TranslationManagerServiceImpl extends int translationActivityUid = getActivityUidByComponentName(getContext(), componentName, getUserId()); String packageName = componentName.getPackageName(); - if (state != STATE_UI_TRANSLATION_FINISHED) { - invokeCallbacks(state, sourceSpec, targetSpec, packageName, translationActivityUid); - updateActiveTranslations(state, sourceSpec, targetSpec, packageName, - translationActivityUid); - } else { - if (mActiveTranslations.contains(translationActivityUid)) { - mActiveTranslations.delete(translationActivityUid); - } else { - Slog.w(TAG, "Finishing translation for activity with uid=" + translationActivityUid - + " but no active translation was found for it"); + + invokeCallbacksIfNecessaryLocked(state, sourceSpec, targetSpec, packageName, activityToken, + translationActivityUid); + updateActiveTranslationsLocked(state, sourceSpec, targetSpec, packageName, activityToken, + translationActivityUid); + } + + @GuardedBy("mLock") + private void updateActiveTranslationsLocked(int state, TranslationSpec sourceSpec, + TranslationSpec targetSpec, String packageName, IBinder activityToken, + int translationActivityUid) { + // We keep track of active translations and their state so that we can: + // 1. Trigger callbacks that are registered after translation has started. + // See registerUiTranslationStateCallbackLocked(). + // 2. NOT trigger callbacks when the state didn't change. + // See invokeCallbacksIfNecessaryLocked(). + ActiveTranslation activeTranslation = mActiveTranslations.get(activityToken); + switch (state) { + case STATE_UI_TRANSLATION_STARTED: { + if (activeTranslation == null) { + try { + activityToken.linkToDeath(this, /* flags= */ 0); + } catch (RemoteException e) { + Slog.w(TAG, "Failed to call linkToDeath for translated app with uid=" + + translationActivityUid + "; activity is already dead", e); + + // Apps with registered callbacks were just notified that translation + // started. We should let them know translation is finished too. + invokeCallbacks(STATE_UI_TRANSLATION_FINISHED, sourceSpec, targetSpec, + packageName, translationActivityUid); + return; + } + mActiveTranslations.put(activityToken, + new ActiveTranslation(sourceSpec, targetSpec, translationActivityUid, + packageName)); + } + break; } + + case STATE_UI_TRANSLATION_PAUSED: { + if (activeTranslation != null) { + activeTranslation.isPaused = true; + } + break; + } + + case STATE_UI_TRANSLATION_RESUMED: { + if (activeTranslation != null) { + activeTranslation.isPaused = false; + } + break; + } + + case STATE_UI_TRANSLATION_FINISHED: { + if (activeTranslation != null) { + mActiveTranslations.remove(activityToken); + } + break; + } + } + + if (DEBUG) { + Slog.d(TAG, + "Updating to translation state=" + state + " for activity with uid=" + + translationActivityUid + " packageName=" + packageName); } } @GuardedBy("mLock") - private void updateActiveTranslations(int state, TranslationSpec sourceSpec, - TranslationSpec targetSpec, String packageName, int translationActivityUid) { - // Keep track of active translations so that we can trigger callbacks that are - // registered after translation has started. - switch (state) { - case STATE_UI_TRANSLATION_STARTED: { - ActiveTranslation activeTranslation = new ActiveTranslation(sourceSpec, - targetSpec, packageName); - mActiveTranslations.put(translationActivityUid, activeTranslation); - break; + private void invokeCallbacksIfNecessaryLocked(int state, TranslationSpec sourceSpec, + TranslationSpec targetSpec, String packageName, IBinder activityToken, + int translationActivityUid) { + boolean shouldInvokeCallbacks = true; + int stateForCallbackInvocation = state; + + ActiveTranslation activeTranslation = mActiveTranslations.get(activityToken); + if (activeTranslation == null) { + if (state != STATE_UI_TRANSLATION_STARTED) { + shouldInvokeCallbacks = false; + Slog.w(TAG, + "Updating to translation state=" + state + " for activity with uid=" + + translationActivityUid + " packageName=" + packageName + + " but no active translation was found for it"); } - case STATE_UI_TRANSLATION_PAUSED: - case STATE_UI_TRANSLATION_RESUMED: { - ActiveTranslation activeTranslation = mActiveTranslations.get( - translationActivityUid); - if (activeTranslation != null) { - activeTranslation.isPaused = (state == STATE_UI_TRANSLATION_PAUSED); - } else { - Slog.w(TAG, "Pausing or resuming translation for activity with uid=" - + translationActivityUid - + " but no active translation was found for it"); + } else { + switch (state) { + case STATE_UI_TRANSLATION_STARTED: { + boolean specsAreIdentical = activeTranslation.sourceSpec.getLocale().equals( + sourceSpec.getLocale()) + && activeTranslation.targetSpec.getLocale().equals( + targetSpec.getLocale()); + if (specsAreIdentical) { + if (activeTranslation.isPaused) { + // Ideally UiTranslationManager.resumeTranslation() should be first + // used to resume translation, but for the purposes of invoking the + // callback, we want to call onResumed() instead of onStarted(). This + // way there can only be one call to onStarted() for the lifetime of + // a translated activity and this will simplify the number of states + // apps have to handle. + stateForCallbackInvocation = STATE_UI_TRANSLATION_RESUMED; + } else { + // Don't invoke callbacks if the state or specs didn't change. For a + // given activity, startTranslation() will be called every time there + // are new views to be translated, but we don't need to repeatedly + // notify apps about it. + shouldInvokeCallbacks = false; + } + } + break; + } + + case STATE_UI_TRANSLATION_PAUSED: { + if (activeTranslation.isPaused) { + // Don't invoke callbacks if the state didn't change. + shouldInvokeCallbacks = false; + } + break; + } + + case STATE_UI_TRANSLATION_RESUMED: { + if (!activeTranslation.isPaused) { + // Don't invoke callbacks if the state didn't change. Either + // resumeTranslation() was called consecutive times, or right after + // startTranslation(). The latter case shouldn't happen normally, so we + // don't want apps to have to handle that particular transition. + shouldInvokeCallbacks = false; + } + break; + } + + case STATE_UI_TRANSLATION_FINISHED: { + // Note: Here finishTranslation() was called but we don't want to invoke + // onFinished() on the callbacks. They will be invoked when + // UiTranslationManager.onTranslationFinished() is called (see + // onTranslationFinishedLocked()). + shouldInvokeCallbacks = false; + break; } - break; } } + + if (DEBUG) { + Slog.d(TAG, + (shouldInvokeCallbacks ? "" : "NOT ") + + "Invoking callbacks for translation state=" + + stateForCallbackInvocation + " for activity with uid=" + + translationActivityUid + " packageName=" + packageName); + } + + if (shouldInvokeCallbacks) { + invokeCallbacks(stateForCallbackInvocation, sourceSpec, targetSpec, packageName, + translationActivityUid); + } } @GuardedBy("mLock") @@ -417,11 +531,8 @@ final class TranslationManagerServiceImpl extends // Trigger the callback for already active translations. List enabledInputMethods = getEnabledInputMethods(); for (int i = 0; i < mActiveTranslations.size(); i++) { - int activeTranslationUid = mActiveTranslations.keyAt(i); ActiveTranslation activeTranslation = mActiveTranslations.valueAt(i); - if (activeTranslation == null) { - continue; - } + int activeTranslationUid = activeTranslation.translationActivityUid; String packageName = activeTranslation.packageName; if (DEBUG) { Slog.d(TAG, "Triggering callback for sourceUid=" + sourceUid @@ -492,13 +603,34 @@ final class TranslationManagerServiceImpl extends public final TranslationSpec sourceSpec; public final TranslationSpec targetSpec; public final String packageName; + public final int translationActivityUid; public boolean isPaused = false; private ActiveTranslation(TranslationSpec sourceSpec, TranslationSpec targetSpec, - String packageName) { + int translationActivityUid, String packageName) { this.sourceSpec = sourceSpec; this.targetSpec = targetSpec; + this.translationActivityUid = translationActivityUid; this.packageName = packageName; } } + + @Override + public void binderDied() { + // Don't need to implement this with binderDied(IBinder) implemented. + } + + @Override + public void binderDied(IBinder who) { + synchronized (mLock) { + mWaitingFinishedCallbackActivities.remove(who); + ActiveTranslation activeTranslation = mActiveTranslations.remove(who); + if (activeTranslation != null) { + // Let apps with registered callbacks know about the activity's death. + invokeCallbacks(STATE_UI_TRANSLATION_FINISHED, activeTranslation.sourceSpec, + activeTranslation.targetSpec, activeTranslation.packageName, + activeTranslation.translationActivityUid); + } + } + } } From 4fe28a4d242f8fb67baaeac655a93d1d2ac6b4fc Mon Sep 17 00:00:00 2001 From: Mark Punzalan Date: Mon, 4 Apr 2022 19:17:26 +0000 Subject: [PATCH 2/2] Rename translationActivityUid to translatedAppUid. Bug: 192331240 Test: N/A - Just renaming Change-Id: Ib85ba777a15f9c63bb6349dbe5974c84a1f6b0eb --- .../TranslationManagerServiceImpl.java | 74 +++++++++---------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/services/translation/java/com/android/server/translation/TranslationManagerServiceImpl.java b/services/translation/java/com/android/server/translation/TranslationManagerServiceImpl.java index 5e13031763cd5..eafcef2f1d38a 100644 --- a/services/translation/java/com/android/server/translation/TranslationManagerServiceImpl.java +++ b/services/translation/java/com/android/server/translation/TranslationManagerServiceImpl.java @@ -191,25 +191,24 @@ final class TranslationManagerServiceImpl extends } } - private int getActivityUidByComponentName(Context context, ComponentName componentName, - int userId) { - int translationActivityUid = -1; + private int getAppUidByComponentName(Context context, ComponentName componentName, int userId) { + int translatedAppUid = -1; try { if (componentName != null) { - translationActivityUid = context.getPackageManager().getApplicationInfoAsUser( + translatedAppUid = context.getPackageManager().getApplicationInfoAsUser( componentName.getPackageName(), 0, userId).uid; } } catch (PackageManager.NameNotFoundException e) { Slog.d(TAG, "Cannot find packageManager for" + componentName); } - return translationActivityUid; + return translatedAppUid; } @GuardedBy("mLock") public void onTranslationFinishedLocked(boolean activityDestroyed, IBinder token, ComponentName componentName) { - final int translationActivityUid = - getActivityUidByComponentName(getContext(), componentName, getUserId()); + final int translatedAppUid = + getAppUidByComponentName(getContext(), componentName, getUserId()); final String packageName = componentName.getPackageName(); if (activityDestroyed) { // In the Activity destroy case, we only calls onTranslationFinished() in @@ -217,13 +216,13 @@ final class TranslationManagerServiceImpl extends // should remove the waiting callback to avoid callback twice. invokeCallbacks(STATE_UI_TRANSLATION_FINISHED, /* sourceSpec= */ null, /* targetSpec= */ null, - packageName, translationActivityUid); + packageName, translatedAppUid); mWaitingFinishedCallbackActivities.remove(token); } else { if (mWaitingFinishedCallbackActivities.contains(token)) { invokeCallbacks(STATE_UI_TRANSLATION_FINISHED, /* sourceSpec= */ null, /* targetSpec= */ null, - packageName, translationActivityUid); + packageName, translatedAppUid); mWaitingFinishedCallbackActivities.remove(token); } } @@ -260,20 +259,20 @@ final class TranslationManagerServiceImpl extends } ComponentName componentName = mActivityTaskManagerInternal.getActivityName(activityToken); - int translationActivityUid = - getActivityUidByComponentName(getContext(), componentName, getUserId()); + int translatedAppUid = + getAppUidByComponentName(getContext(), componentName, getUserId()); String packageName = componentName.getPackageName(); invokeCallbacksIfNecessaryLocked(state, sourceSpec, targetSpec, packageName, activityToken, - translationActivityUid); + translatedAppUid); updateActiveTranslationsLocked(state, sourceSpec, targetSpec, packageName, activityToken, - translationActivityUid); + translatedAppUid); } @GuardedBy("mLock") private void updateActiveTranslationsLocked(int state, TranslationSpec sourceSpec, TranslationSpec targetSpec, String packageName, IBinder activityToken, - int translationActivityUid) { + int translatedAppUid) { // We keep track of active translations and their state so that we can: // 1. Trigger callbacks that are registered after translation has started. // See registerUiTranslationStateCallbackLocked(). @@ -287,16 +286,16 @@ final class TranslationManagerServiceImpl extends activityToken.linkToDeath(this, /* flags= */ 0); } catch (RemoteException e) { Slog.w(TAG, "Failed to call linkToDeath for translated app with uid=" - + translationActivityUid + "; activity is already dead", e); + + translatedAppUid + "; activity is already dead", e); // Apps with registered callbacks were just notified that translation // started. We should let them know translation is finished too. invokeCallbacks(STATE_UI_TRANSLATION_FINISHED, sourceSpec, targetSpec, - packageName, translationActivityUid); + packageName, translatedAppUid); return; } mActiveTranslations.put(activityToken, - new ActiveTranslation(sourceSpec, targetSpec, translationActivityUid, + new ActiveTranslation(sourceSpec, targetSpec, translatedAppUid, packageName)); } break; @@ -326,15 +325,15 @@ final class TranslationManagerServiceImpl extends if (DEBUG) { Slog.d(TAG, - "Updating to translation state=" + state + " for activity with uid=" - + translationActivityUid + " packageName=" + packageName); + "Updating to translation state=" + state + " for app with uid=" + + translatedAppUid + " packageName=" + packageName); } } @GuardedBy("mLock") private void invokeCallbacksIfNecessaryLocked(int state, TranslationSpec sourceSpec, TranslationSpec targetSpec, String packageName, IBinder activityToken, - int translationActivityUid) { + int translatedAppUid) { boolean shouldInvokeCallbacks = true; int stateForCallbackInvocation = state; @@ -343,8 +342,8 @@ final class TranslationManagerServiceImpl extends if (state != STATE_UI_TRANSLATION_STARTED) { shouldInvokeCallbacks = false; Slog.w(TAG, - "Updating to translation state=" + state + " for activity with uid=" - + translationActivityUid + " packageName=" + packageName + "Updating to translation state=" + state + " for app with uid=" + + translatedAppUid + " packageName=" + packageName + " but no active translation was found for it"); } } else { @@ -408,13 +407,13 @@ final class TranslationManagerServiceImpl extends Slog.d(TAG, (shouldInvokeCallbacks ? "" : "NOT ") + "Invoking callbacks for translation state=" - + stateForCallbackInvocation + " for activity with uid=" - + translationActivityUid + " packageName=" + packageName); + + stateForCallbackInvocation + " for app with uid=" + translatedAppUid + + " packageName=" + packageName); } if (shouldInvokeCallbacks) { invokeCallbacks(stateForCallbackInvocation, sourceSpec, targetSpec, packageName, - translationActivityUid); + translatedAppUid); } } @@ -457,15 +456,14 @@ final class TranslationManagerServiceImpl extends private void invokeCallbacks( int state, TranslationSpec sourceSpec, TranslationSpec targetSpec, String packageName, - int translationActivityUid) { + int translatedAppUid) { Bundle result = createResultForCallback(state, sourceSpec, targetSpec, packageName); if (mCallbacks.getRegisteredCallbackCount() == 0) { return; } List enabledInputMethods = getEnabledInputMethods(); mCallbacks.broadcast((callback, uid) -> { - invokeCallback((int) uid, translationActivityUid, callback, result, - enabledInputMethods); + invokeCallback((int) uid, translatedAppUid, callback, result, enabledInputMethods); }); } @@ -488,9 +486,9 @@ final class TranslationManagerServiceImpl extends } private void invokeCallback( - int callbackSourceUid, int translationActivityUid, IRemoteCallback callback, + int callbackSourceUid, int translatedAppUid, IRemoteCallback callback, Bundle result, List enabledInputMethods) { - if (callbackSourceUid == translationActivityUid) { + if (callbackSourceUid == translatedAppUid) { // Invoke callback for the application being translated. try { callback.sendResult(result); @@ -532,25 +530,25 @@ final class TranslationManagerServiceImpl extends List enabledInputMethods = getEnabledInputMethods(); for (int i = 0; i < mActiveTranslations.size(); i++) { ActiveTranslation activeTranslation = mActiveTranslations.valueAt(i); - int activeTranslationUid = activeTranslation.translationActivityUid; + int translatedAppUid = activeTranslation.translatedAppUid; String packageName = activeTranslation.packageName; if (DEBUG) { Slog.d(TAG, "Triggering callback for sourceUid=" + sourceUid - + " for translated activity with uid=" + activeTranslationUid + + " for translated app with uid=" + translatedAppUid + "packageName=" + packageName + " isPaused=" + activeTranslation.isPaused); } Bundle startedResult = createResultForCallback(STATE_UI_TRANSLATION_STARTED, activeTranslation.sourceSpec, activeTranslation.targetSpec, packageName); - invokeCallback(sourceUid, activeTranslationUid, callback, startedResult, + invokeCallback(sourceUid, translatedAppUid, callback, startedResult, enabledInputMethods); if (activeTranslation.isPaused) { // Also send event so callback owners know that translation was started then paused. Bundle pausedResult = createResultForCallback(STATE_UI_TRANSLATION_PAUSED, activeTranslation.sourceSpec, activeTranslation.targetSpec, packageName); - invokeCallback(sourceUid, activeTranslationUid, callback, pausedResult, + invokeCallback(sourceUid, translatedAppUid, callback, pausedResult, enabledInputMethods); } } @@ -603,14 +601,14 @@ final class TranslationManagerServiceImpl extends public final TranslationSpec sourceSpec; public final TranslationSpec targetSpec; public final String packageName; - public final int translationActivityUid; + public final int translatedAppUid; public boolean isPaused = false; private ActiveTranslation(TranslationSpec sourceSpec, TranslationSpec targetSpec, - int translationActivityUid, String packageName) { + int translatedAppUid, String packageName) { this.sourceSpec = sourceSpec; this.targetSpec = targetSpec; - this.translationActivityUid = translationActivityUid; + this.translatedAppUid = translatedAppUid; this.packageName = packageName; } } @@ -629,7 +627,7 @@ final class TranslationManagerServiceImpl extends // Let apps with registered callbacks know about the activity's death. invokeCallbacks(STATE_UI_TRANSLATION_FINISHED, activeTranslation.sourceSpec, activeTranslation.targetSpec, activeTranslation.packageName, - activeTranslation.translationActivityUid); + activeTranslation.translatedAppUid); } } }