From b49208df9a832c438183c6c5eeda2613bd24d65b Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Tue, 11 Dec 2018 10:25:57 -0800 Subject: [PATCH] Fixed 2 Augmented Autofill issues: - NPE when FillCallback.onSuccess is invoked with null FillResponse. - Updated AutofillProxy properties when field focused again. Test: manual verification Fixes: 120788613 Fixes: 120854842 Change-Id: I6d614c11afa501eba88e3d62334cfd4dd4624f79 --- .../augmented/AugmentedAutofillService.java | 47 ++++++++++++++----- .../autofill/augmented/FillCallback.java | 10 ++++ .../autofill/augmented/FillRequest.java | 6 +-- .../autofill/augmented/FillResponse.java | 2 + 4 files changed, 51 insertions(+), 14 deletions(-) diff --git a/core/java/android/service/autofill/augmented/AugmentedAutofillService.java b/core/java/android/service/autofill/augmented/AugmentedAutofillService.java index 68a86f3c8bcab..9aff281b04723 100644 --- a/core/java/android/service/autofill/augmented/AugmentedAutofillService.java +++ b/core/java/android/service/autofill/augmented/AugmentedAutofillService.java @@ -32,7 +32,6 @@ import android.os.IBinder; import android.os.Looper; import android.os.RemoteException; import android.os.SystemClock; -import android.service.autofill.augmented.AugmentedAutofillService.AutofillProxy; import android.service.autofill.augmented.PresentationParams.SystemPopupPresentationParams; import android.util.Log; import android.util.Pair; @@ -172,7 +171,9 @@ public abstract class AugmentedAutofillService extends Service { mAutofillProxies.put(sessionId, proxy); } else { // TODO(b/111330312): figure out if it's ok to reuse the proxy; add logging + // TODO(b/111330312): also make sure to cover scenario on CTS test if (DEBUG) Log.d(TAG, "Reusing proxy for session " + sessionId); + proxy.update(focusedId, focusedValue); } // TODO(b/111330312): set cancellation signal final CancellationSignal cancellationSignal = null; @@ -248,8 +249,10 @@ public abstract class AugmentedAutofillService extends Service { private final IFillCallback mCallback; public final int taskId; public final ComponentName componentName; - public final AutofillId focusedId; - public final AutofillValue focusedValue; + @GuardedBy("mLock") + private AutofillId mFocusedId; + @GuardedBy("mLock") + private AutofillValue mFocusedValue; // Objects used to log metrics private final long mRequestTime; @@ -272,8 +275,8 @@ public abstract class AugmentedAutofillService extends Service { mCallback = callback; this.taskId = taskId; this.componentName = componentName; - this.focusedId = focusedId; - this.focusedValue = focusedValue; + this.mFocusedId = focusedId; + this.mFocusedValue = focusedValue; this.mRequestTime = requestTime; // TODO(b/111330312): linkToDeath } @@ -286,13 +289,13 @@ public abstract class AugmentedAutofillService extends Service { } Rect rect; try { - rect = mClient.getViewCoordinates(focusedId); + rect = mClient.getViewCoordinates(mFocusedId); } catch (RemoteException e) { - Log.w(TAG, "Could not get coordinates for " + focusedId); + Log.w(TAG, "Could not get coordinates for " + mFocusedId); return null; } if (rect == null) { - if (DEBUG) Log.d(TAG, "getViewCoordinates(" + focusedId + ") returned null"); + if (DEBUG) Log.d(TAG, "getViewCoordinates(" + mFocusedId + ") returned null"); return null; } mSmartSuggestion = new SystemPopupPresentationParams(this, rect); @@ -325,6 +328,28 @@ public abstract class AugmentedAutofillService extends Service { } } + private void update(@NonNull AutofillId focusedId, @NonNull AutofillValue focusedValue) { + synchronized (mLock) { + // TODO(b/111330312): should we close the popupwindow if the focused id changed? + mFocusedId = focusedId; + mFocusedValue = focusedValue; + } + } + + @NonNull + public AutofillId getFocusedId() { + synchronized (mLock) { + return mFocusedId; + } + } + + @NonNull + public AutofillValue getFocusedValue() { + synchronized (mLock) { + return mFocusedValue; + } + } + // Used (mostly) for metrics. public void report(@ReportEvent int event) { switch (event) { @@ -372,9 +397,9 @@ public abstract class AugmentedAutofillService extends Service { pw.print(prefix); pw.print("taskId: "); pw.println(taskId); pw.print(prefix); pw.print("component: "); pw.println(componentName.flattenToShortString()); - pw.print(prefix); pw.print("focusedId: "); pw.println(focusedId); - if (focusedValue != null) { - pw.print(prefix); pw.print("focusedValue: "); pw.println(focusedValue); + pw.print(prefix); pw.print("focusedId: "); pw.println(mFocusedId); + if (mFocusedValue != null) { + pw.print(prefix); pw.print("focusedValue: "); pw.println(mFocusedValue); } pw.print(prefix); pw.print("client: "); pw.println(mClient); final String prefix2 = prefix + " "; diff --git a/core/java/android/service/autofill/augmented/FillCallback.java b/core/java/android/service/autofill/augmented/FillCallback.java index 054646572a588..620ec59c9e55a 100644 --- a/core/java/android/service/autofill/augmented/FillCallback.java +++ b/core/java/android/service/autofill/augmented/FillCallback.java @@ -15,10 +15,13 @@ */ package android.service.autofill.augmented; +import static android.service.autofill.augmented.AugmentedAutofillService.DEBUG; + import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.SystemApi; import android.service.autofill.augmented.AugmentedAutofillService.AutofillProxy; +import android.util.Log; /** * Callback used to indicate at {@link FillRequest} has been fulfilled. @@ -27,6 +30,9 @@ import android.service.autofill.augmented.AugmentedAutofillService.AutofillProxy */ @SystemApi public final class FillCallback { + + private static final String TAG = FillCallback.class.getSimpleName(); + private final AutofillProxy mProxy; FillCallback(@NonNull AutofillProxy proxy) { @@ -40,7 +46,11 @@ public final class FillCallback { * could not provide autofill for the request. */ public void onSuccess(@Nullable FillResponse response) { + if (DEBUG) Log.d(TAG, "onSuccess(): " + response); + mProxy.report(AutofillProxy.REPORT_EVENT_ON_SUCCESS); + if (response == null) return; + final FillWindow fillWindow = response.getFillWindow(); if (fillWindow != null) { fillWindow.show(); diff --git a/core/java/android/service/autofill/augmented/FillRequest.java b/core/java/android/service/autofill/augmented/FillRequest.java index fd75b156858ee..57d2cc85f7eaf 100644 --- a/core/java/android/service/autofill/augmented/FillRequest.java +++ b/core/java/android/service/autofill/augmented/FillRequest.java @@ -58,7 +58,7 @@ public final class FillRequest { */ @NonNull public AutofillId getFocusedId() { - return mProxy.focusedId; + return mProxy.getFocusedId(); } /** @@ -66,7 +66,7 @@ public final class FillRequest { */ @NonNull public AutofillValue getFocusedValue() { - return mProxy.focusedValue; + return mProxy.getFocusedValue(); } /** @@ -82,6 +82,6 @@ public final class FillRequest { @Override public String toString() { return "FillRequest[act=" + getActivityComponent().flattenToShortString() - + ", id=" + mProxy.focusedId + "]"; + + ", id=" + mProxy.getFocusedId() + "]"; } } diff --git a/core/java/android/service/autofill/augmented/FillResponse.java b/core/java/android/service/autofill/augmented/FillResponse.java index 7064b6fe80795..1ecfab4edc12f 100644 --- a/core/java/android/service/autofill/augmented/FillResponse.java +++ b/core/java/android/service/autofill/augmented/FillResponse.java @@ -102,6 +102,8 @@ public final class FillResponse implements Parcelable { // TODO(b/111330312): add methods to disable app / activity, either here or on manager } + // TODO(b/111330312): implement to String + @Override public int describeContents() { return 0;