Skip to content

Commit

Permalink
Android: Dedupe startFileIntent() in intent launching code
Browse files Browse the repository at this point in the history 
This CL dedupes the implementations of startFileIntent() between the
//chrome and //weblayer embeddings of //components/external_intents. We
do this by moving these functions into the shared
ExternalNavigationHandler. This necessitates adding a new
ExternalNavigationDelegate method to determine whether the current tab
is incognito.

Bug: 1071390
Change-Id: I35afcb366dafed2f21eeb08377c0f42bff3f45dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2179905
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#765905}
  • Loading branch information
@colinblundell
colinblundell authored and Commit Bot committed May 6, 2020
1 parent f7c2d78 commit c4c41ca
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 75 deletions.
33 changes: 5 additions & 28 deletions ...roid/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

package org.chromium.chrome.browser.externalnav;

import android.Manifest.permission;
import android.app.Activity;
import android.content.ActivityNotFoundException;
import android.content.Context;
Expand Down Expand Up @@ -49,7 +48,6 @@
import org.chromium.content_public.browser.LoadUrlParams;
import org.chromium.content_public.browser.WebContents;
import org.chromium.ui.UiUtils;
import org.chromium.ui.base.PermissionCallback;
import org.chromium.ui.base.WindowAndroid;
import org.chromium.webapk.lib.client.WebApkValidator;

Expand Down Expand Up @@ -305,32 +303,6 @@ public void onCancel(DialogInterface dialog) {
return true;
}

@Override
public void startFileIntent(
final Intent intent, final String referrerUrl, final boolean needsToCloseTab) {
PermissionCallback permissionCallback = new PermissionCallback() {
@Override
public void onRequestPermissionsResult(String[] permissions, int[] grantResults) {
if (grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED
&& hasValidTab()) {
ExternalNavigationHandler.loadUrlFromIntent(referrerUrl, intent.getDataString(),
null, ExternalNavigationDelegateImpl.this, needsToCloseTab,
mTab.isIncognito());
} else {
// TODO(tedchoc): Show an indication to the user that the navigation failed
// instead of silently dropping it on the floor.
if (needsToCloseTab) {
// If the access was not granted, then close the tab if necessary.
closeTab();
}
}
}
};
if (!hasValidTab()) return;
mTab.getWindowAndroid().requestPermissions(
new String[] {permission.READ_EXTERNAL_STORAGE}, permissionCallback);
}

@Override
public boolean supportsCreatingNewTabs() {
return true;
Expand Down Expand Up @@ -382,6 +354,11 @@ public void closeTab() {
}
}

@Override
public boolean isIncognito() {
return mTab.isIncognito();
}

@Override
public void maybeAdjustInstantAppExtras(Intent intent, boolean isIntentToInstantApp) {
if (isIntentToInstantApp) {
Expand Down
13 changes: 3 additions & 10 deletions ...android/java/src/org/chromium/components/external_intents/ExternalNavigationDelegate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,6 @@ boolean startIncognitoIntent(Intent intent, String referrerUrl, String fallbackU
@OverrideUrlLoadingResult
int handleIncognitoIntentTargetingSelf(Intent intent, String referrerUrl, String fallbackUrl);

/**
* Trigger a UI affordance that will ask the user to grant file access. After the access
* has been granted or denied, continue loading the specified file URL.
*
* @param intent The intent to continue loading the file URL.
* @param referrerUrl The HTTP referrer URL.
* @param needsToCloseTab Whether this action should close the current tab.
*/
void startFileIntent(Intent intent, String referrerUrl, boolean needsToCloseTab);

/**
* Loads a URL as specified by |loadUrlParams| if possible. May fail in exceptional conditions
* (e.g., if there is no valid tab).
Expand Down Expand Up @@ -181,6 +171,9 @@ boolean maybeLaunchInstantApp(
/* Invoked when the tab associated with this delegate should be closed. */
void closeTab();

/* Returns whether whether the tab associated with this delegate is incognito. */
boolean isIncognito();

/**
* @param intent The intent to launch.
* @return Whether the Intent points to an app that we trust and that launched Chrome.
Expand Down
35 changes: 34 additions & 1 deletion .../android/java/src/org/chromium/components/external_intents/ExternalNavigationHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import org.chromium.content_public.common.Referrer;
import org.chromium.network.mojom.ReferrerPolicy;
import org.chromium.ui.base.PageTransition;
import org.chromium.ui.base.PermissionCallback;
import org.chromium.url.URI;

import java.lang.annotation.Retention;
Expand Down Expand Up @@ -347,14 +348,46 @@ private boolean startFileIntentIfNecessary(
ExternalNavigationParams params, Intent targetIntent) {
if (params.getUrl().startsWith(UrlConstants.FILE_URL_SHORT_PREFIX)
&& shouldRequestFileAccess(params.getUrl())) {
mDelegate.startFileIntent(targetIntent, params.getReferrerUrl(),
startFileIntent(targetIntent, params.getReferrerUrl(),
params.shouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent());
if (DEBUG) Log.i(TAG, "Requesting filesystem access");
return true;
}
return false;
}

/**
* Trigger a UI affordance that will ask the user to grant file access. After the access
* has been granted or denied, continue loading the specified file URL.
*
* @param intent The intent to continue loading the file URL.
* @param referrerUrl The HTTP referrer URL.
* @param needsToCloseTab Whether this action should close the current tab.
*/
protected void startFileIntent(
final Intent intent, final String referrerUrl, final boolean needsToCloseTab) {
PermissionCallback permissionCallback = new PermissionCallback() {
@Override
public void onRequestPermissionsResult(String[] permissions, int[] grantResults) {
if (grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED
&& mDelegate.hasValidTab()) {
loadUrlFromIntent(referrerUrl, intent.getDataString(), null, mDelegate,
needsToCloseTab, mDelegate.isIncognito());
} else {
// TODO(tedchoc): Show an indication to the user that the navigation failed
// instead of silently dropping it on the floor.
if (needsToCloseTab) {
// If the access was not granted, then close the tab if necessary.
mDelegate.closeTab();
}
}
}
};
if (!mDelegate.hasValidTab()) return;
mDelegate.getWindowAndroid().requestPermissions(
new String[] {permission.READ_EXTERNAL_STORAGE}, permissionCallback);
}

/**
* Clobber the current tab and try not to pass an intent when it should be handled internally
* so that we can deliver HTTP referrer information safely.
Expand Down
20 changes: 12 additions & 8 deletions ...javatests/src/org/chromium/components/external_intents/ExternalNavigationHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1712,6 +1712,7 @@ private static class ExternalNavigationHandlerForTesting extends ExternalNavigat
public boolean mShouldRequestFileAccess;
public String mNewUrlAfterClobbering;
public String mReferrerUrlForClobbering;
public boolean mStartFileIntentCalled;

public ExternalNavigationHandlerForTesting(ExternalNavigationDelegate delegate) {
super(delegate);
Expand Down Expand Up @@ -1742,6 +1743,11 @@ protected boolean shouldRequestFileAccess(String url) {
return mShouldRequestFileAccess;
}

@Override
protected void startFileIntent(Intent intent, String referrerUrl, boolean needsToCloseTab) {
mStartFileIntentCalled = true;
}

@Override
protected @OverrideUrlLoadingResult int clobberCurrentTab(String url, String referrerUrl) {
mNewUrlAfterClobbering = url;
Expand Down Expand Up @@ -1858,11 +1864,6 @@ public boolean startIncognitoIntent(Intent intent, String referrerUrl, String fa
return OverrideUrlLoadingResult.OVERRIDE_WITH_EXTERNAL_INTENT;
}

@Override
public void startFileIntent(Intent intent, String referrerUrl, boolean needsToCloseTab) {
startFileIntentCalled = true;
}

@Override
public boolean supportsCreatingNewTabs() {
return false;
Expand All @@ -1879,6 +1880,11 @@ public boolean canLoadUrlInCurrentTab() {
@Override
public void closeTab() {}

@Override
public boolean isIncognito() {
return false;
}

@Override
public void loadUrlIfPossible(LoadUrlParams loadUrlParams) {}

Expand Down Expand Up @@ -1970,7 +1976,6 @@ public void reset() {
startActivityIntent = null;
startIncognitoIntentCalled = false;
handleIncognitoIntentTargetingSelfCalled = false;
startFileIntentCalled = false;
mCalledWithProxy = false;
}

Expand Down Expand Up @@ -2030,7 +2035,6 @@ public void setCanLoadUrlInTab(boolean value) {
public boolean startIncognitoIntentCalled;
public boolean handleIncognitoIntentTargetingSelfCalled;
public boolean maybeSetUserGestureCalled;
public boolean startFileIntentCalled;

private String mReferrerWebappPackageName;

Expand Down Expand Up @@ -2157,7 +2161,7 @@ public void expecting(
Assert.assertEquals(expectStartIncognito, mDelegate.startIncognitoIntentCalled);
Assert.assertEquals(expectStartActivity, startActivityCalled);
Assert.assertEquals(expectStartWebApk, startWebApkCalled);
Assert.assertEquals(expectStartFile, mDelegate.startFileIntentCalled);
Assert.assertEquals(expectStartFile, mUrlHandler.mStartFileIntentCalled);
Assert.assertEquals(expectProxyForIA, mDelegate.mCalledWithProxy);

if (startActivityCalled && expectSaneIntent) {
Expand Down
33 changes: 5 additions & 28 deletions weblayer/browser/java/org/chromium/weblayer_private/ExternalNavigationDelegateImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

package org.chromium.weblayer_private;

import android.Manifest.permission;
import android.app.Activity;
import android.content.Context;
import android.content.Intent;
Expand All @@ -23,7 +22,6 @@
import org.chromium.components.external_intents.ExternalNavigationParams;
import org.chromium.content_public.browser.LoadUrlParams;
import org.chromium.content_public.browser.WebContents;
import org.chromium.ui.base.PermissionCallback;
import org.chromium.ui.base.WindowAndroid;

import java.util.ArrayList;
Expand Down Expand Up @@ -181,32 +179,6 @@ public boolean startIncognitoIntent(final Intent intent, final String referrerUr
return OverrideUrlLoadingResult.NO_OVERRIDE;
}

@Override
public void startFileIntent(
final Intent intent, final String referrerUrl, final boolean needsToCloseTab) {
PermissionCallback permissionCallback = new PermissionCallback() {
@Override
public void onRequestPermissionsResult(String[] permissions, int[] grantResults) {
if (grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED
&& hasValidTab()) {
ExternalNavigationHandler.loadUrlFromIntent(referrerUrl, intent.getDataString(),
null, ExternalNavigationDelegateImpl.this, needsToCloseTab,
mTab.getProfile().isIncognito());
} else {
// TODO(tedchoc): Show an indication to the user that the navigation failed
// instead of silently dropping it on the floor.
if (needsToCloseTab) {
// If the access was not granted, then close the tab if necessary.
closeTab();
}
}
}
};
if (!hasValidTab()) return;
mTab.getBrowser().getWindowAndroid().requestPermissions(
new String[] {permission.READ_EXTERNAL_STORAGE}, permissionCallback);
}

@Override
public void loadUrlIfPossible(LoadUrlParams loadUrlParams) {
if (!hasValidTab()) return;
Expand Down Expand Up @@ -248,6 +220,11 @@ public void closeTab() {
InterceptNavigationDelegateClientImpl.closeTab(mTab);
}

@Override
public boolean isIncognito() {
return mTab.getProfile().isIncognito();
}

@Override
public void maybeAdjustInstantAppExtras(Intent intent, boolean isIntentToInstantApp) {}

Expand Down

0 comments on commit c4c41ca

Please sign in to comment.