Skip to content

Commit

Permalink
Remove CancelRequest for PermissionManager/PermissionContextBase/Perm…
Browse files Browse the repository at this point in the history 
…issionRequest

This removes the CancelRequest functionality from the permissions UI
code. Cancelling permission requests is rarely used and very
complicated. It's not actually needed as requests will automatically be
cancelled when the user navigates. Callers can use weak pointers in
the request callback if they expect that the lifetime of the request will
outlast the lifetime of the callsite.

Bug: 792382
Change-Id: Ib757b9d796b80474e06af3a341bbf565ff9ff2e9
Tbr: lcwu@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/876746
Commit-Queue: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Reviewed-by: David Vallet <dvallet@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532295}
  • Loading branch information
Raymes Khoury authored and Commit Bot committed Jan 28, 2018
1 parent 301e853 commit 5bf2b08
Show file tree
Hide file tree
Showing 52 changed files with 68 additions and 848 deletions.
67 changes: 33 additions & 34 deletions android_webview/browser/aw_permission_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,39 @@ void AwPermissionManager::OnRequestResponse(
pair.first.Run(pair.second);
}

void AwPermissionManager::ResetPermission(PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) {
result_cache_->ClearResult(permission, requesting_origin, embedding_origin);
}

PermissionStatus AwPermissionManager::GetPermissionStatus(
PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) {
// Method is called outside the Permissions API only for this permission.
if (permission == PermissionType::PROTECTED_MEDIA_IDENTIFIER) {
return result_cache_->GetResult(permission, requesting_origin,
embedding_origin);
} else if (permission == PermissionType::MIDI ||
permission == PermissionType::SENSORS) {
return PermissionStatus::GRANTED;
}

return PermissionStatus::DENIED;
}

int AwPermissionManager::SubscribePermissionStatusChange(
PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin,
const base::Callback<void(PermissionStatus)>& callback) {
return kNoPendingOperation;
}

void AwPermissionManager::UnsubscribePermissionStatusChange(
int subscription_id) {}

void AwPermissionManager::CancelPermissionRequest(int request_id) {
PendingRequest* pending_request = pending_requests_.Lookup(request_id);
if (!pending_request || pending_request->IsCancelled())
Expand Down Expand Up @@ -488,40 +521,6 @@ void AwPermissionManager::CancelPermissionRequest(int request_id) {
pending_requests_.Remove(request_id);
}

void AwPermissionManager::ResetPermission(PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) {
result_cache_->ClearResult(permission, requesting_origin, embedding_origin);
}

PermissionStatus AwPermissionManager::GetPermissionStatus(
PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) {
// Method is called outside the Permissions API only for this permission.
if (permission == PermissionType::PROTECTED_MEDIA_IDENTIFIER) {
return result_cache_->GetResult(permission, requesting_origin,
embedding_origin);
} else if (permission == PermissionType::MIDI ||
permission == PermissionType::SENSORS) {
return PermissionStatus::GRANTED;
}

return PermissionStatus::DENIED;
}

int AwPermissionManager::SubscribePermissionStatusChange(
PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin,
const base::Callback<void(PermissionStatus)>& callback) {
return kNoPendingOperation;
}

void AwPermissionManager::UnsubscribePermissionStatusChange(
int subscription_id) {
}

void AwPermissionManager::CancelPermissionRequests() {
std::vector<int> request_ids;
for (PendingRequestsMap::Iterator<PendingRequest> it(&pending_requests_);
Expand Down
2 changes: 1 addition & 1 deletion android_webview/browser/aw_permission_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ class AwPermissionManager : public content::PermissionManager {
const base::Callback<
void(const std::vector<blink::mojom::PermissionStatus>&)>& callback)
override;
void CancelPermissionRequest(int request_id) override;
void ResetPermission(content::PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) override;
Expand All @@ -56,6 +55,7 @@ class AwPermissionManager : public content::PermissionManager {
void UnsubscribePermissionStatusChange(int subscription_id) override;

protected:
void CancelPermissionRequest(int request_id);
void CancelPermissionRequests();

private:
Expand Down
177 changes: 0 additions & 177 deletions android_webview/browser/aw_permission_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -714,183 +714,6 @@ TEST_F(AwPermissionManagerTest, ComplicatedRequestScenario4) {
EXPECT_EQ(3, resolved_permission_request_id[5]);
}

// Test the case CancelPermissionRequest is called for an invalid request.
TEST_F(AwPermissionManagerTest, InvalidRequestIsCancelled) {
manager->CancelPermissionRequest(0);
}

// Test the case a delegate is called, and cancelled.
TEST_F(AwPermissionManagerTest, SinglePermissionRequestIsCancelled) {
int request_id = manager->RequestPermission(
PermissionType::GEOLOCATION, render_frame_host, GURL(kRequestingOrigin1),
true, base::Bind(&AwPermissionManagerTest::PermissionRequestResponse,
base::Unretained(this), 0));
EXPECT_NE(kNoPendingOperation, request_id);
EXPECT_EQ(0u, resolved_permission_status.size());

manager->CancelPermissionRequest(request_id);
EXPECT_EQ(0u, resolved_permission_status.size());

// This should not resolve the permission.
manager->EnqueuePermissionResponse(kRequestingOrigin1,
PermissionType::GEOLOCATION, true);
EXPECT_EQ(0u, resolved_permission_status.size());
}

// Test the case multiple permissions are requested through single call, and
// cancelled.
TEST_F(AwPermissionManagerTest, SinglePermissionsRequestIsCancelled) {
std::vector<PermissionType> permissions_1 = {PermissionType::MIDI,
PermissionType::MIDI_SYSEX};

int request_id = manager->RequestPermissions(
permissions_1, render_frame_host, GURL(kRequestingOrigin1), true,
base::Bind(&AwPermissionManagerTest::PermissionsRequestResponse,
base::Unretained(this), 0));
EXPECT_NE(kNoPendingOperation, request_id);
EXPECT_EQ(0u, resolved_permission_status.size());

manager->CancelPermissionRequest(request_id);
EXPECT_EQ(0u, resolved_permission_status.size());

std::vector<PermissionType> permissions_2 = {PermissionType::GEOLOCATION,
PermissionType::MIDI_SYSEX};

request_id = manager->RequestPermissions(
permissions_2, render_frame_host, GURL(kRequestingOrigin1), true,
base::Bind(&AwPermissionManagerTest::PermissionsRequestResponse,
base::Unretained(this), 0));
EXPECT_NE(kNoPendingOperation, request_id);
EXPECT_EQ(0u, resolved_permission_status.size());

manager->CancelPermissionRequest(request_id);
EXPECT_EQ(0u, resolved_permission_status.size());

// This should not resolve the permission.
manager->EnqueuePermissionResponse(kRequestingOrigin1,
PermissionType::GEOLOCATION, true);
manager->EnqueuePermissionResponse(kRequestingOrigin1,
PermissionType::MIDI_SYSEX, true);
EXPECT_EQ(0u, resolved_permission_status.size());
}

// Test the case multiple permissions are requested, and cancelled as follow.
// 1. Permission A is requested.
// 2. Permission A is requested for the same origin again.
// 3. The first request is cancelled.
TEST_F(AwPermissionManagerTest, ComplicatedCancelScenario1) {
int request_1 = manager->RequestPermission(
PermissionType::GEOLOCATION, render_frame_host, GURL(kRequestingOrigin1),
true, base::Bind(&AwPermissionManagerTest::PermissionRequestResponse,
base::Unretained(this), 1));
EXPECT_NE(kNoPendingOperation, request_1);
EXPECT_EQ(0u, resolved_permission_status.size());

int request_2 = manager->RequestPermission(
PermissionType::GEOLOCATION, render_frame_host, GURL(kRequestingOrigin1),
true, base::Bind(&AwPermissionManagerTest::PermissionRequestResponse,
base::Unretained(this), 2));
EXPECT_NE(kNoPendingOperation, request_2);
EXPECT_EQ(0u, resolved_permission_status.size());

EXPECT_NE(request_1, request_2);

manager->CancelPermissionRequest(request_1);
EXPECT_EQ(0u, resolved_permission_status.size());

// This should resolve the second request.
manager->EnqueuePermissionResponse(kRequestingOrigin1,
PermissionType::GEOLOCATION, true);
EXPECT_EQ(1u, resolved_permission_status.size());
EXPECT_EQ(PermissionStatus::GRANTED, resolved_permission_status[0]);
EXPECT_EQ(2, resolved_permission_request_id[0]);
}

// Test the case multiple permissions are requested, and cancelled as follow.
// 1. Permission A is requested.
// 2. Permission A is requested for a different origin.
// 3. The first request is cancelled.
TEST_F(AwPermissionManagerTest, ComplicatedCancelScenario2) {
int request_1 = manager->RequestPermission(
PermissionType::GEOLOCATION, render_frame_host, GURL(kRequestingOrigin1),
true, base::Bind(&AwPermissionManagerTest::PermissionRequestResponse,
base::Unretained(this), 1));
EXPECT_NE(kNoPendingOperation, request_1);
EXPECT_EQ(0u, resolved_permission_status.size());

int request_2 = manager->RequestPermission(
PermissionType::GEOLOCATION, render_frame_host, GURL(kRequestingOrigin2),
true, base::Bind(&AwPermissionManagerTest::PermissionRequestResponse,
base::Unretained(this), 2));
EXPECT_NE(kNoPendingOperation, request_2);
EXPECT_EQ(0u, resolved_permission_status.size());

EXPECT_NE(request_1, request_2);

manager->CancelPermissionRequest(request_1);
EXPECT_EQ(0u, resolved_permission_status.size());

// This should not resolve the first request.
manager->EnqueuePermissionResponse(kRequestingOrigin1,
PermissionType::GEOLOCATION, true);
EXPECT_EQ(0u, resolved_permission_status.size());

// This should resolve the second request.
manager->EnqueuePermissionResponse(kRequestingOrigin2,
PermissionType::GEOLOCATION, true);
EXPECT_EQ(1u, resolved_permission_status.size());
EXPECT_EQ(PermissionStatus::GRANTED, resolved_permission_status[0]);
EXPECT_EQ(2, resolved_permission_request_id[0]);
}

// Test the case multiple permissions are requested, and cancelled as follow.
// 1. Permission A and B are requested.
// 2. Permission A and B are requested for a different origin.
// 3. Permission A for the second request is resolved.
// 4. The second request is cancelled.
TEST_F(AwPermissionManagerTest, ComplicatedCancelScenario3) {
std::vector<PermissionType> permissions = {PermissionType::GEOLOCATION,
PermissionType::MIDI_SYSEX};

int request_1 = manager->RequestPermissions(
permissions, render_frame_host, GURL(kRequestingOrigin1), true,
base::Bind(&AwPermissionManagerTest::PermissionsRequestResponse,
base::Unretained(this), 1));
EXPECT_NE(kNoPendingOperation, request_1);
EXPECT_EQ(0u, resolved_permission_status.size());

int request_2 = manager->RequestPermissions(
permissions, render_frame_host, GURL(kRequestingOrigin2), true,
base::Bind(&AwPermissionManagerTest::PermissionsRequestResponse,
base::Unretained(this), 2));
EXPECT_NE(kNoPendingOperation, request_2);
EXPECT_EQ(0u, resolved_permission_status.size());

EXPECT_NE(request_1, request_2);

manager->EnqueuePermissionResponse(kRequestingOrigin2,
PermissionType::GEOLOCATION, true);

manager->CancelPermissionRequest(request_1);
EXPECT_EQ(0u, resolved_permission_status.size());

// This should not resolve the first request.
manager->EnqueuePermissionResponse(kRequestingOrigin1,
PermissionType::GEOLOCATION, true);
manager->EnqueuePermissionResponse(kRequestingOrigin1,
PermissionType::MIDI_SYSEX, true);
EXPECT_EQ(0u, resolved_permission_status.size());

// This should resolve the second request.
manager->EnqueuePermissionResponse(kRequestingOrigin2,
PermissionType::MIDI_SYSEX, true);
EXPECT_EQ(2u, resolved_permission_status.size());
EXPECT_EQ(PermissionStatus::GRANTED, resolved_permission_status[0]);
EXPECT_EQ(2, resolved_permission_request_id[0]);
EXPECT_EQ(PermissionStatus::GRANTED, resolved_permission_status[1]);
EXPECT_EQ(2, resolved_permission_request_id[1]);
}

} // namespace

} // namespace android_webview
101 changes: 0 additions & 101 deletions ...ndroid/javatests/src/org/chromium/chrome/browser/infobar/PermissionUpdateInfobarTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import org.chromium.base.ThreadUtils;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.RetryOnFailure;
import org.chromium.chrome.browser.ChromeSwitches;
import org.chromium.chrome.browser.preferences.website.ContentSetting;
import org.chromium.chrome.browser.preferences.website.GeolocationInfo;
Expand Down Expand Up @@ -160,106 +159,6 @@ public void run() {
}
}

// Flaky: http://crbug.com/749369
@RetryOnFailure
@Test
@MediumTest
public void testInfobarFrameNavigationForGeolocation()
throws IllegalArgumentException, InterruptedException, TimeoutException {
ChromeTabUtils.newTabFromMenu(
InstrumentationRegistry.getInstrumentation(), mActivityTestRule.getActivity());

// Register for animation notifications
CriteriaHelper.pollInstrumentationThread(new Criteria() {
@Override
public boolean isSatisfied() {
if (mActivityTestRule.getActivity().getActivityTab() == null) return false;
if (mActivityTestRule.getActivity().getActivityTab().getInfoBarContainer()
== null) {
return false;
}
return true;
}
});
InfoBarContainer container =
mActivityTestRule.getActivity().getActivityTab().getInfoBarContainer();
mListener = new InfoBarTestAnimationListener();
container.addAnimationListener(mListener);

final String locationUrl = mTestServer.getURL(GEOLOCATION_IFRAME_PAGE);
final GeolocationInfo geolocationSettings = ThreadUtils.runOnUiThreadBlockingNoException(
new Callable<GeolocationInfo>() {
@Override
public GeolocationInfo call() {
return new GeolocationInfo(locationUrl, null, false);
}
});

mActivityTestRule.getActivity().getWindowAndroid().setAndroidPermissionDelegate(
new TestAndroidPermissionDelegate(
null, Arrays.asList(Manifest.permission.ACCESS_FINE_LOCATION), null));
LocationSettingsTestUtil.setSystemLocationSettingEnabled(true);

try {
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
@Override
public void run() {
geolocationSettings.setContentSetting(ContentSetting.ALLOW);
}
});

mActivityTestRule.loadUrl(mTestServer.getURL(GEOLOCATION_IFRAME_PAGE));
mListener.addInfoBarAnimationFinished("InfoBar not added");
Assert.assertEquals(1, mActivityTestRule.getInfoBars().size());

final WebContents webContents = ThreadUtils.runOnUiThreadBlockingNoException(
new Callable<WebContents>() {
@Override
public WebContents call() throws Exception {
return mActivityTestRule.getActivity()
.getActivityTab()
.getWebContents();
}
});
Assert.assertFalse(webContents.isDestroyed());

mActivityTestRule.runJavaScriptCodeInCurrentTab(
"document.querySelector('iframe').src = '';");
CriteriaHelper.pollUiThread(Criteria.equals(0, new Callable<Integer>() {
@Override
public Integer call() {
return mActivityTestRule.getInfoBars().size();
}
}));

ChromeTabUtils.closeCurrentTab(
InstrumentationRegistry.getInstrumentation(), mActivityTestRule.getActivity());
CriteriaHelper.pollUiThread(new Criteria() {
@Override
public boolean isSatisfied() {
return webContents.isDestroyed();
}
});

CriteriaHelper.pollUiThread(Criteria.equals(1, new Callable<Integer>() {
@Override
public Integer call() {
return mActivityTestRule.getActivity()
.getTabModelSelector()
.getModel(false)
.getCount();
}
}));
} finally {
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
@Override
public void run() {
geolocationSettings.setContentSetting(ContentSetting.DEFAULT);
}
});
}
}

private static class TestAndroidPermissionDelegate implements AndroidPermissionDelegate {
private final Set<String> mHasPermissions;
private final Set<String> mRequestablePermissions;
Expand Down
Loading

0 comments on commit 5bf2b08

Please sign in to comment.