Skip to content

Commit

Permalink
Remove window from IntentRequestTracker#onActivityResult() params
Browse files Browse the repository at this point in the history
Since conceptually, each IntentRequest is owned by one activity and
shared by an indefinite number WindowAndroids, it doesn't make sense to
have IntentRequestTracker#onActivityResult() return a WindowAndroid.

This CL removes WindowAndroid from this method and refactors the
following callers who depended on it.
* SelectFileDialog
* SmsVerificationReceiver

Bug: 1221410
Change-Id: I92fe3384a928e046eff6de49d57d93011365b43e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3100385
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#913790}
  • Loading branch information
maxlgu authored and Chromium LUCI CQ committed Aug 20, 2021
1 parent 5620606 commit a34e9fb
Show file tree
Hide file tree
Showing 25 changed files with 63 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -666,8 +666,7 @@ public ActivityWindowAndroid getWindowAndroid() {
@CallSuper
@Override
public boolean onActivityResultWithNative(int requestCode, int resultCode, Intent intent) {
if (mIntentRequestTracker.onActivityResult(
requestCode, resultCode, intent, mWindowAndroid)) {
if (mIntentRequestTracker.onActivityResult(requestCode, resultCode, intent)) {
return true;
}
mLifecycleDispatcher.dispatchOnActivityResultWithNative(requestCode, resultCode, intent);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,9 @@ public void testSelectFileAndCancelRequest() throws Throwable {

private void resetActivityWindowAndroidForTest() {
TestThreadUtils.runOnUiThreadBlocking(
() -> mActivityWindowAndroidForTest.lastCallback.onIntentCompleted(
mActivityWindowAndroidForTest, Activity.RESULT_CANCELED, null));
()
-> mActivityWindowAndroidForTest.lastCallback.onIntentCompleted(
Activity.RESULT_CANCELED, null));
mActivityWindowAndroidForTest.lastCallback = null;
mActivityWindowAndroidForTest.lastIntent = null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ public int showCancelableIntent(Intent intent, IntentCallback callback, Integer
mCancelableIntent = intent;
mCallback = callback;
if (mCancelableIntentSuccess) {
callback.onIntentCompleted(mWindowAndroid, mResultCode, mResults);
callback.onIntentCompleted(mResultCode, mResults);
return 0;
}
return WindowAndroid.START_INTENT_FAILURE;
Expand Down Expand Up @@ -1554,7 +1554,7 @@ public void testCallback_CalledTwice() {
Assert.assertEquals(-1, mHandler.getVoiceSearchUnexpectedResultSource());

IntentCallback callback = mWindowAndroid.getIntentCallback();
callback.onIntentCompleted(mWindowAndroid, Activity.RESULT_CANCELED, null);
callback.onIntentCompleted(Activity.RESULT_CANCELED, null);
Assert.assertEquals(
VoiceInteractionSource.NTP, mHandler.getVoiceSearchUnexpectedResultSource());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ public int showCancelableIntent(
PendingIntent intent, WindowAndroid.IntentCallback callback, Integer errorId) {
// Bypass GmsCore and just call onIntentCompleted.
if (mCancelableIntentSuccess) {
callback.onIntentCompleted(this, mResultCode, mResponseIntent);
callback.onIntentCompleted(mResultCode, mResponseIntent);
return 0;
}
return WindowAndroid.START_INTENT_FAILURE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,7 @@ public void addAccount(Callback<String> callback) {
if (intent != null) {
WindowAndroid.IntentCallback intentCallback = new WindowAndroid.IntentCallback() {
@Override
public void onIntentCompleted(
WindowAndroid window, int resultCode, Intent data) {
public void onIntentCompleted(int resultCode, Intent data) {
if (resultCode == Activity.RESULT_OK) {
callback.onResult(data.getStringExtra(AccountManager.KEY_ACCOUNT_NAME));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@ void promoByRoleManager() {
RoleManager roleManager = (RoleManager) mActivity.getSystemService(Context.ROLE_SERVICE);

Intent intent = roleManager.createRequestRoleIntent(RoleManager.ROLE_BROWSER);
mWindowAndroid.showCancelableIntent(intent, (window, resultCode, data) -> {}, null);
mWindowAndroid.showCancelableIntent(intent, (resultCode, data) -> {}, null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ public VoiceRecognitionCompleteCallback(

// WindowAndroid.IntentCallback implementation:
@Override
public void onIntentCompleted(WindowAndroid window, int resultCode, Intent data) {
public void onIntentCompleted(int resultCode, Intent data) {
if (mCallbackComplete) {
recordVoiceSearchUnexpectedResult(mSource, mTarget);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ public void onReceive(Context context, Intent intent) {
}

@Override
public void onIntentCompleted(WindowAndroid window, int resultCode, Intent data) {
public void onIntentCompleted(int resultCode, Intent data) {
// NOTE: The validity of the returned |resultCode| is somewhat unexpected. For
// background, a sharing flow starts with a "Chooser" activity that enables the user
// to select the app to share to, and then when the user selects that application,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,10 @@ public void launchPaymentApp(Intent intent, Callback<String> errorCallback,

// WindowAndroid.IntentCallback implementation.
@Override
public void onIntentCompleted(WindowAndroid windowAndroid, int resultCode, Intent data) {
public void onIntentCompleted(int resultCode, Intent data) {
assert mIntentCallback != null;
windowAndroid.removeIntentCallback(this);
WindowAndroid window = mWebContents.getTopLevelNativeWindow();
if (window != null) window.removeIntentCallback(this);
IntentResult intentResult = new IntentResult();
intentResult.resultCode = resultCode;
intentResult.data = data;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ public void onClick(PropertyModel model, int buttonType) {
try {
mWindowAndroid.showIntent(intent, new WindowAndroid.IntentCallback() {
@Override
public void onIntentCompleted(
WindowAndroid window, int resultCode, Intent data) {
public void onIntentCompleted(int resultCode, Intent data) {
mModalDialogManager.dismissDialog(
model, DialogDismissalCause.POSITIVE_BUTTON_CLICKED);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public void setUp() {
return null;
})
.when(mWindowAndroidIntentCallback)
.onIntentCompleted(any(WindowAndroid.class), anyInt(), any(Intent.class));
.onIntentCompleted(anyInt(), any(Intent.class));

mNfcSystemLevelPrompt = new NfcSystemLevelPrompt();
mNfcSystemLevelPrompt.show(mWindowAndroid, mModalDialogManager, new Runnable() {
Expand Down Expand Up @@ -137,8 +137,7 @@ public void testTurnOnCallback() {
Assert.assertEquals(0, mDialogCallback.getCallCount());
Assert.assertEquals(1, mIntentCallback.getCallCount());

mWindowAndroidIntentCallback.onIntentCompleted(
mWindowAndroid, 0 /* resultCode */, new Intent());
mWindowAndroidIntentCallback.onIntentCompleted(0 /* resultCode */, new Intent());
Assert.assertEquals(1, mDialogCallback.getCallCount());
Assert.assertEquals(1, mIntentCallback.getCallCount());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ private void onGotPendingIntent(PendingIntent pendingIntent) {

// Handles the result.
@Override
public void onIntentCompleted(WindowAndroid window, int resultCode, Intent data) {
public void onIntentCompleted(int resultCode, Intent data) {
if (data == null) {
Log.e(TAG, "Received a null intent.");
// The user canceled the request.
Expand Down
6 changes: 2 additions & 4 deletions content/browser/sms/sms_provider_gms_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,16 +116,14 @@ class SmsProviderGmsBaseTest : public RenderViewHostTestHarness {
SmsFetchType fetch_type = SmsFetchType::kLocal) {
JNIEnv* env = base::android::AttachCurrentThread();
Java_FakeSmsRetrieverClient_triggerUserDeniesPermission(
env, j_fake_sms_retriever_client_, test_window_->GetJavaObject(),
fetch_type == SmsFetchType::kLocal);
env, j_fake_sms_retriever_client_, fetch_type == SmsFetchType::kLocal);
}

void TriggerUserGrantsPermission(
SmsFetchType fetch_type = SmsFetchType::kLocal) {
JNIEnv* env = base::android::AttachCurrentThread();
Java_FakeSmsRetrieverClient_triggerUserGrantsPermission(
env, j_fake_sms_retriever_client_, test_window_->GetJavaObject(),
fetch_type == SmsFetchType::kLocal);
env, j_fake_sms_retriever_client_, fetch_type == SmsFetchType::kLocal);
}

void TriggerAPIFailure(const std::string& failure_type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,7 @@ private void processText(Intent intent) {
try {
mWindowAndroid.showIntent(intent, new WindowAndroid.IntentCallback() {
@Override
public void onIntentCompleted(WindowAndroid window, int resultCode, Intent data) {
public void onIntentCompleted(int resultCode, Intent data) {
onReceivedProcessTextResult(resultCode, data);
}
}, null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public void listen(WindowAndroid window, boolean isLocalRequest) {
&& (!isLocalRequest || mBackend != GmsBackend.USER_CONSENT);
boolean shouldUseUserConsentReceiver = mUserConsentReceiver != null && isLocalRequest
&& mBackend != GmsBackend.VERIFICATION && window != null;
if (shouldUseVerificationReceiver) mVerificationReceiver.listen(window, isLocalRequest);
if (shouldUseVerificationReceiver) mVerificationReceiver.listen(isLocalRequest);
if (shouldUseUserConsentReceiver) mUserConsentReceiver.listen(window);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public void onReceive(Context context, Intent intent) {
intent.getExtras().getParcelable(SmsRetriever.EXTRA_CONSENT_INTENT);
try {
mProvider.getWindow().showIntent(consentIntent,
(window, resultCode, data) -> onConsentResult(resultCode, data), null);
(resultCode, data) -> onConsentResult(resultCode, data), null);
} catch (android.content.ActivityNotFoundException e) {
if (DEBUG) Log.d(TAG, "Error starting activity for result.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import org.chromium.base.Log;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.content.browser.sms.Wrappers.WebOTPServiceContext;
import org.chromium.ui.base.WindowAndroid;

/**
Expand All @@ -48,7 +49,7 @@ private enum BackendAvailability {
NUM_ENTRIES
}

public SmsVerificationReceiver(SmsProviderGms provider, Wrappers.WebOTPServiceContext context) {
public SmsVerificationReceiver(SmsProviderGms provider, WebOTPServiceContext context) {
if (DEBUG) Log.d(TAG, "Creating SmsVerificationReceiver.");

mDestroyed = false;
Expand Down Expand Up @@ -117,12 +118,12 @@ public void onReceive(Context context, Intent intent) {
}
}

public void onPermissionDone(WindowAndroid window, int resultCode, boolean isLocalRequest) {
public void onPermissionDone(int resultCode, boolean isLocalRequest) {
if (resultCode == Activity.RESULT_OK) {
// We have been granted permission to use the SmsCoderetriever so restart the process.
// |listen| will record the backend availability so no need to do it here.
if (DEBUG) Log.d(TAG, "The one-time permission was granted");
listen(window, isLocalRequest);
listen(isLocalRequest);
} else {
cancelRequestAndReportBackendAvailability();
if (DEBUG) Log.d(TAG, "The one-time permission was rejected");
Expand All @@ -133,7 +134,7 @@ public void onPermissionDone(WindowAndroid window, int resultCode, boolean isLoc
* Handles failure for the `SmsCodeBrowserClient.startSmsCodeRetriever()`
* task.
*/
public void onRetrieverTaskFailure(WindowAndroid window, boolean isLocalRequest, Exception e) {
public void onRetrieverTaskFailure(boolean isLocalRequest, Exception e) {
if (DEBUG) Log.d(TAG, "Task failed. Attempting recovery.", e);
ApiException exception = (ApiException) e;
if (exception.getStatusCode() == SmsRetrieverStatusCodes.API_NOT_CONNECTED) {
Expand Down Expand Up @@ -161,14 +162,15 @@ public void onRetrieverTaskFailure(WindowAndroid window, boolean isLocalRequest,
ResolvableApiException rex = (ResolvableApiException) exception;
try {
PendingIntent resolutionIntent = rex.getResolution();
window.showIntent(resolutionIntent, new WindowAndroid.IntentCallback() {
@Override
public void onIntentCompleted(
WindowAndroid w, int resultCode, Intent data) {
// Backend availability will be recorded inside |onPermissionDone|.
onPermissionDone(w, resultCode, isLocalRequest);
}
}, null);
mProvider.getWindow().showIntent(
resolutionIntent, new WindowAndroid.IntentCallback() {
@Override
public void onIntentCompleted(int resultCode, Intent data) {
// Backend availability will be recorded inside
// |onPermissionDone|.
onPermissionDone(resultCode, isLocalRequest);
}
}, null);
} catch (Exception ex) {
cancelRequestAndReportBackendAvailability();
Log.e(TAG, "Cannot launch user permission", ex);
Expand All @@ -179,7 +181,7 @@ public void onIntentCompleted(
}
}

public void listen(WindowAndroid window, boolean isLocalRequest) {
public void listen(boolean isLocalRequest) {
Wrappers.SmsRetrieverClientWrapper client = mProvider.getClient();
Task<Void> task = client.startSmsCodeBrowserRetriever();

Expand All @@ -188,7 +190,7 @@ public void listen(WindowAndroid window, boolean isLocalRequest) {
mProvider.verificationReceiverSucceeded(isLocalRequest);
});
task.addOnFailureListener((Exception e) -> {
this.onRetrieverTaskFailure(window, isLocalRequest, e);
this.onRetrieverTaskFailure(isLocalRequest, e);
mProvider.verificationReceiverFailed(isLocalRequest);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.JNIAdditionalImport;
import org.chromium.base.annotations.JNINamespace;
import org.chromium.ui.base.WindowAndroid;

@JNINamespace("content")
@JNIAdditionalImport(Wrappers.class)
Expand Down Expand Up @@ -108,16 +107,16 @@ private void triggerUserConsentTimeout() {
}

@CalledByNative("FakeSmsRetrieverClient")
private void triggerUserDeniesPermission(WindowAndroid window, boolean isLocalRequest) {
private void triggerUserDeniesPermission(boolean isLocalRequest) {
Wrappers.WebOTPServiceContext context = super.getContext();
assert context != null;

SmsVerificationReceiver receiver = context.createVerificationReceiverForTesting();
receiver.onPermissionDone(window, Activity.RESULT_CANCELED, isLocalRequest);
receiver.onPermissionDone(Activity.RESULT_CANCELED, isLocalRequest);
}

@CalledByNative("FakeSmsRetrieverClient")
private void triggerUserGrantsPermission(WindowAndroid window, boolean isLocalRequest) {
private void triggerUserGrantsPermission(boolean isLocalRequest) {
Wrappers.WebOTPServiceContext context = super.getContext();
if (context == null) {
Log.v(TAG,
Expand All @@ -128,7 +127,7 @@ private void triggerUserGrantsPermission(WindowAndroid window, boolean isLocalRe

SmsVerificationReceiver receiver =
(SmsVerificationReceiver) context.createVerificationReceiverForTesting();
receiver.onPermissionDone(window, Activity.RESULT_OK, isLocalRequest);
receiver.onPermissionDone(Activity.RESULT_OK, isLocalRequest);
}

@CalledByNative("FakeSmsRetrieverClient")
Expand Down Expand Up @@ -157,7 +156,7 @@ private void triggerFailure(String type, boolean isLocalRequest) {

ApiException e = new ApiException(new Status(code));

receiver.onRetrieverTaskFailure(null, isLocalRequest, e);
receiver.onRetrieverTaskFailure(isLocalRequest, e);
}

// ---------------------------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public void testUserConsentBackendWithLocalRequest() {
Mockito.verify(mProvider.getUserConsentReceiverForTesting(), times(1))
.listen(mWindowAndroid);
Mockito.verify(mProvider.getVerificationReceiverForTesting(), times(0))
.listen(any(), anyBoolean());
.listen(anyBoolean());
}

@Test
Expand All @@ -84,7 +84,7 @@ public void testUserConsentBackendWithRemoteRequest() {
mProvider.listen(mWindowAndroid, isLocalRequest);
Mockito.verify(mProvider.getUserConsentReceiverForTesting(), times(0)).listen(any());
Mockito.verify(mProvider.getVerificationReceiverForTesting(), times(1))
.listen(mWindowAndroid, isLocalRequest);
.listen(isLocalRequest);
}

@Test
Expand All @@ -94,7 +94,7 @@ public void testVerificationBackendWithLocalRequest() {
mProvider.listen(mWindowAndroid, isLocalRequest);
Mockito.verify(mProvider.getUserConsentReceiverForTesting(), times(0)).listen(any());
Mockito.verify(mProvider.getVerificationReceiverForTesting(), times(1))
.listen(mWindowAndroid, isLocalRequest);
.listen(isLocalRequest);
}

@Test
Expand All @@ -104,7 +104,7 @@ public void testVerificationBackendWithRemoteRequest() {
mProvider.listen(mWindowAndroid, isLocalRequest);
Mockito.verify(mProvider.getUserConsentReceiverForTesting(), times(0)).listen(any());
Mockito.verify(mProvider.getVerificationReceiverForTesting(), times(1))
.listen(mWindowAndroid, isLocalRequest);
.listen(isLocalRequest);
}

@Test
Expand All @@ -115,7 +115,7 @@ public void testAutoBackendWithLocalRequest() {
Mockito.verify(mProvider.getUserConsentReceiverForTesting(), times(1))
.listen(mWindowAndroid);
Mockito.verify(mProvider.getVerificationReceiverForTesting(), times(1))
.listen(mWindowAndroid, isLocalRequest);
.listen(isLocalRequest);
}

@Test
Expand All @@ -125,7 +125,7 @@ public void testAutoBackendWithRemoteRequest() {
mProvider.listen(mWindowAndroid, isLocalRequest);
Mockito.verify(mProvider.getUserConsentReceiverForTesting(), times(0)).listen(any());
Mockito.verify(mProvider.getVerificationReceiverForTesting(), times(1))
.listen(mWindowAndroid, isLocalRequest);
.listen(isLocalRequest);
}

@Test
Expand Down Expand Up @@ -174,7 +174,7 @@ public void testVerificationBackendWithoutWindow() {
boolean isLocalRequest = true;
mProvider.listen(/*window=*/null, isLocalRequest);
Mockito.verify(mProvider.getVerificationReceiverForTesting(), times(1))
.listen(null, isLocalRequest);
.listen(isLocalRequest);
}

@Test
Expand All @@ -184,6 +184,6 @@ public void testAutoBackendWithoutWindow() {
mProvider.listen(/*window=*/null, isLocalRequest);
Mockito.verify(mProvider.getUserConsentReceiverForTesting(), times(0)).listen(null);
Mockito.verify(mProvider.getVerificationReceiverForTesting(), times(1))
.listen(null, isLocalRequest);
.listen(isLocalRequest);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ protected void onStart() {
@Override
public void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);
mIntentRequestTracker.onActivityResult(requestCode, resultCode, data, mWindowAndroid);
mIntentRequestTracker.onActivityResult(requestCode, resultCode, data);
}

@Override
Expand Down
Loading

0 comments on commit a34e9fb

Please sign in to comment.