Skip to content

Commit

Permalink
The permissions API code cleanup.
Browse files Browse the repository at this point in the history
PermissionManager contains duplicated methods that differentiate by permissions type enum (ContentSettingsType vs PermissionType). This CL removed most of the ContentSettingsType-related methods, because they had limited usage, mostly in tests.

Methods removed from PermissionManager:
* RequestPermission(ContentSettingsType)
* RequestPermissions(ContentSettingsType)
* RequestPermissionFromCurrentDocument
(ContentSettingsType)
* PermissionManager::RequestPermissionsFromCurrentDocument
(ContentSettingsType)
* GetPermissionStatusForFrame(ContentSettingsType)
* GetPermissionStatusForWorker(ContentSettingsType)
* GetPermissionStatusForFrame(PermissionType)

Methods removed from PermissionControllerDelegate:
* GetPermissionStatusForFrame(PermissionType)

Methods added to PermissionControllerDelegate:
* RequestPermissionsFromCurrentDocument(PermissionType)

Methods added to PermissionManager:
* RequestPermissionsFromCurrentDocument(PermissionType)

Bug: 1271543
Change-Id: I7c718e3a1a390cd5be6f143ad8f7a07477473e3a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3599030
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Commit-Queue: Peter Kvitek <kvitekp@chromium.org>
Reviewed-by: Ravjit Uppal <ravjit@chromium.org>
Reviewed-by: Sean Topping <seantopping@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Illia Klimov <elklm@google.com>
Reviewed-by: Michael Bai <michaelbai@chromium.org>
Reviewed-by: Florent Castelli <orphis@chromium.org>
Reviewed-by: Peter Kvitek <kvitekp@chromium.org>
Quick-Run: Peter Kvitek <kvitekp@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002151}
  • Loading branch information
Illia Klimov authored and Chromium LUCI CQ committed May 11, 2022
1 parent 7470b43 commit 27239ed
Show file tree
Hide file tree
Showing 41 changed files with 535 additions and 673 deletions.
27 changes: 15 additions & 12 deletions android_webview/browser/aw_permission_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,17 @@ void AwPermissionManager::ResetPermission(PermissionType permission,
result_cache_->ClearResult(permission, requesting_origin, embedding_origin);
}

void AwPermissionManager::RequestPermissionsFromCurrentDocument(
const std::vector<PermissionType>& permissions,
content::RenderFrameHost* render_frame_host,
bool user_gesture,
base::OnceCallback<void(const std::vector<blink::mojom::PermissionStatus>&)>
callback) {
RequestPermissions(permissions, render_frame_host,
LastCommittedOrigin(render_frame_host), user_gesture,
std::move(callback));
}

PermissionStatus AwPermissionManager::GetPermissionStatus(
PermissionType permission,
const GURL& requesting_origin,
Expand All @@ -457,23 +468,15 @@ PermissionStatus AwPermissionManager::GetPermissionStatus(
return PermissionStatus::DENIED;
}

PermissionStatus AwPermissionManager::GetPermissionStatusForFrame(
PermissionType permission,
content::RenderFrameHost* render_frame_host,
const GURL& requesting_origin) {
return GetPermissionStatus(
permission, requesting_origin,
permissions::PermissionUtil::GetLastCommittedOriginAsURL(
render_frame_host));
}

PermissionStatus AwPermissionManager::GetPermissionStatusForCurrentDocument(
PermissionType permission,
content::RenderFrameHost* render_frame_host) {
return GetPermissionStatus(
permission, render_frame_host->GetLastCommittedOrigin().GetURL(),
permission,
permissions::PermissionUtil::GetLastCommittedOriginAsURL(
render_frame_host),
permissions::PermissionUtil::GetLastCommittedOriginAsURL(
render_frame_host));
render_frame_host->GetMainFrame()));
}

PermissionStatus AwPermissionManager::GetPermissionStatusForWorker(
Expand Down
11 changes: 7 additions & 4 deletions android_webview/browser/aw_permission_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,17 @@ class AwPermissionManager : public content::PermissionControllerDelegate {
void ResetPermission(blink::PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) override;
void RequestPermissionsFromCurrentDocument(
const std::vector<blink::PermissionType>& permissions,
content::RenderFrameHost* render_frame_host,
bool user_gesture,
base::OnceCallback<
void(const std::vector<blink::mojom::PermissionStatus>&)> callback)
override;
blink::mojom::PermissionStatus GetPermissionStatus(
blink::PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) override;
blink::mojom::PermissionStatus GetPermissionStatusForFrame(
blink::PermissionType permission,
content::RenderFrameHost* render_frame_host,
const GURL& requesting_origin) override;
blink::mojom::PermissionStatus GetPermissionStatusForCurrentDocument(
blink::PermissionType permission,
content::RenderFrameHost* render_frame_host) override;
Expand Down
10 changes: 6 additions & 4 deletions chrome/browser/chrome_back_forward_cache_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -253,10 +253,12 @@ IN_PROC_BROWSER_TEST_F(ChromeBackForwardCacheBrowserTest,
content::RenderFrameHost::LifecycleState::kInBackForwardCache);
base::MockOnceCallback<void(blink::mojom::PermissionStatus)> callback;
EXPECT_CALL(callback, Run(blink::mojom::PermissionStatus::ASK));
// PermissionManagerFactory::GetForProfile(browser()->profile())
browser()->profile()->GetPermissionController()->RequestPermission(
blink::PermissionType::GEOLOCATION, rfh_a.get(), url_a,
/* user_gesture = */ true, callback.Get());
browser()
->profile()
->GetPermissionController()
->RequestPermissionFromCurrentDocument(
blink::PermissionType::GEOLOCATION, rfh_a.get(),
/* user_gesture = */ true, callback.Get());

// Ensure |rfh_a| is evicted from the cache because it is not allowed to
// service the GEOLOCATION permission request.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "components/permissions/permission_result.h"
#include "components/permissions/test/mock_permission_prompt_factory.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/permissions/permission_utils.h"

#if BUILDFLAG(IS_ANDROID)
#include "chrome/browser/android/search_permissions/search_permissions_service.h"
Expand Down Expand Up @@ -66,13 +67,21 @@ class GeolocationPermissionContextDelegateTests
}

void RequestPermissionFromCurrentDocument(
ContentSettingsType permission,
blink::PermissionType permission,
content::RenderFrameHost* render_frame_host,
bool user_gesture,
base::OnceCallback<void(ContentSetting)> callback) {
base::OnceCallback<void(blink::mojom::PermissionStatus)> callback) {
PermissionManagerFactory::GetForProfile(profile())
->RequestPermissionFromCurrentDocument(
permission, render_frame_host, user_gesture, std::move(callback));
->RequestPermissionsFromCurrentDocument(
{permission}, render_frame_host, user_gesture,
base::BindOnce(
[](base::OnceCallback<void(blink::mojom::PermissionStatus)>
callback,
const std::vector<blink::mojom::PermissionStatus>& state) {
DCHECK_EQ(state.size(), 1U);
std::move(callback).Run(state[0]);
},
std::move(callback)));
}

permissions::PermissionResult GetPermissionStatusForDisplayOnSettingsUI(
Expand All @@ -94,10 +103,10 @@ TEST_F(GeolocationPermissionContextDelegateTests, TabContentSettingIsUpdated) {

base::RunLoop run_loop;
RequestPermissionFromCurrentDocument(
ContentSettingsType::GEOLOCATION, main_rfh(), true,
blink::PermissionType::GEOLOCATION, main_rfh(), true,
base::BindOnce(
[](base::RunLoop* run_loop, ContentSetting content_setting) {
EXPECT_EQ(content_setting, CONTENT_SETTING_ALLOW);
[](base::RunLoop* run_loop, blink::mojom::PermissionStatus status) {
EXPECT_EQ(status, blink::mojom::PermissionStatus::GRANTED);
run_loop->Quit();
},
&run_loop));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
#include "components/permissions/test/mock_permission_prompt_factory.h"
#include "components/permissions/test/mock_permission_request.h"
#include "components/variations/variations_associated_data.h"
#include "content/public/browser/permission_controller_delegate.h"
#include "content/public/browser/permission_controller.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/render_view_host.h"
Expand Down Expand Up @@ -1235,10 +1235,12 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerWithFencedFrameTest,

base::MockOnceCallback<void(blink::mojom::PermissionStatus)> callback;
EXPECT_CALL(callback, Run(blink::mojom::PermissionStatus::DENIED));
auto* delegate = browser()->profile()->GetPermissionControllerDelegate();
delegate->RequestPermission(blink::PermissionType::SENSORS, fenced_frame_host,
fenced_frame_url,
/* user_gesture = */ true, callback.Get());

content::PermissionController* permission_controller =
browser()->profile()->GetPermissionController();
permission_controller->RequestPermissionFromCurrentDocument(
blink::PermissionType::SENSORS, fenced_frame_host,
/* user_gesture = */ true, callback.Get());
console_observer.Wait();
ASSERT_EQ(1u, console_observer.messages().size());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -901,25 +901,8 @@ IN_PROC_BROWSER_TEST_F(PermissionsSecurityModelInteractiveUITest,
EXPECT_EQ("https://chromium.org/", main_rfh->GetLastCommittedURL().spec());
EXPECT_TRUE(main_rfh->GetLastCommittedOrigin().GetURL().SchemeIsFile());

const struct {
std::string check_permission;
std::string request_permission;
} kTests[] = {
{kCheckCamera, kRequestCamera},
{kCheckGeolocation, kRequestGeolocation},
};

for (const auto& test : kTests) {
ASSERT_FALSE(
content::EvalJs(main_rfh, test.check_permission).value.GetBool());
EXPECT_EQ("granted", content::EvalJs(main_rfh, test.request_permission));
ASSERT_TRUE(
content::EvalJs(main_rfh, test.check_permission).value.GetBool());
}

// Notifications is not supported for file:/// with changed URL.
ASSERT_FALSE(content::EvalJs(main_rfh, kCheckNotifications).value.GetBool());
EXPECT_EQ("denied", content::EvalJs(main_rfh, kRequestNotifications));
// `https://chromium.org` is used for permissions verification.
VerifyPermissionsAllowed(main_rfh);
}

// Verifies that permissions are not supported for file:/// with changed URL to
Expand Down
24 changes: 15 additions & 9 deletions chromecast/browser/cast_permission_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -152,22 +152,28 @@ void CastPermissionManager::ResetPermission(blink::PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) {}

void CastPermissionManager::RequestPermissionsFromCurrentDocument(
const std::vector<blink::PermissionType>& permissions,
content::RenderFrameHost* render_frame_host,
bool user_gesture,
base::OnceCallback<void(const std::vector<blink::mojom::PermissionStatus>&)>
callback) {
std::vector<blink::mojom::PermissionStatus> permission_statuses;
for (auto permission : permissions) {
permission_statuses.push_back(GetPermissionStatusInternal(
permission, render_frame_host,
render_frame_host->GetLastCommittedOrigin().GetURL()));
}
std::move(callback).Run(permission_statuses);
}

blink::mojom::PermissionStatus CastPermissionManager::GetPermissionStatus(
blink::PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) {
return GetPermissionStatusInternal(permission, requesting_origin);
}

blink::mojom::PermissionStatus
CastPermissionManager::GetPermissionStatusForFrame(
blink::PermissionType permission,
content::RenderFrameHost* render_frame_host,
const GURL& requesting_origin) {
return GetPermissionStatusInternal(permission, render_frame_host,
requesting_origin);
}

blink::mojom::PermissionStatus
CastPermissionManager::GetPermissionStatusForCurrentDocument(
blink::PermissionType permission,
Expand Down
11 changes: 7 additions & 4 deletions chromecast/browser/cast_permission_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,17 @@ class CastPermissionManager : public content::PermissionControllerDelegate {
void ResetPermission(blink::PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) override;
void RequestPermissionsFromCurrentDocument(
const std::vector<blink::PermissionType>& permissions,
content::RenderFrameHost* render_frame_host,
bool user_gesture,
base::OnceCallback<
void(const std::vector<blink::mojom::PermissionStatus>&)> callback)
override;
blink::mojom::PermissionStatus GetPermissionStatus(
blink::PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) override;
blink::mojom::PermissionStatus GetPermissionStatusForFrame(
blink::PermissionType permission,
content::RenderFrameHost* render_frame_host,
const GURL& requesting_origin) override;
blink::mojom::PermissionStatus GetPermissionStatusForCurrentDocument(
blink::PermissionType permission,
content::RenderFrameHost* render_frame_host) override;
Expand Down
1 change: 1 addition & 0 deletions components/permissions/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ include_rules = [
"+third_party/blink/public/common/permissions/permission_utils.h",
"+third_party/blink/public/common/web_preferences",
"+third_party/blink/public/mojom/bluetooth",
"+third_party/blink/public/mojom/permissions/permission_status.mojom.h",
"+third_party/blink/public/mojom/permissions_policy",
"+third_party/blink/public/mojom/quota",
"+third_party/sqlite",
Expand Down

0 comments on commit 27239ed

Please sign in to comment.