Commit 5a4f422d authored by Bret Sepulveda's avatar Bret Sepulveda Committed by Commit Bot

Delete PermissionRequest::GetEmbeddingOrigin.

PermissionRequest:GetEmbeddingOrigin was not implemented for all
permission types, which made it difficult to use anywhere except when
constructing the prompt strings. This patch moves that construction into
UI code where the strings are used, which also simplifies the plumbing
in PermissionPromptImpl. This patch also moves the Flash warning to use
the new codepath, which allows us to delete
PermissionRequest::GetMessageTextWarningFragment.

This has a minor visual impact on the permission prompt bubble on
Desktop. See the associated bug for screenshots.

Bug: 1110905
Change-Id: Icfeedd011d620e467a5dbbf75bea6875f4af9379
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2379729Reviewed-by: default avatarAndy Paicu <andypaicu@chromium.org>
Reviewed-by: default avatarOlesia Marukhno <olesiamarukhno@google.com>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806960}
parent 2d7a23f7
......@@ -206,7 +206,8 @@ class PermissionDialogTest
void SetUpOnMainThread() override {
// Skip super: It will install a mock permission UI factory, but for this
// test we want to show "real" UI.
ui_test_utils::NavigateToURL(browser(), GetUrl());
ui_test_utils::NavigateToURL(browser(),
GURL("https://toplevel.example.com"));
}
GURL GetUrl() { return GURL("https://example.com"); }
......@@ -261,7 +262,7 @@ permissions::PermissionRequest* PermissionDialogTest::MakePermissionRequest(
auto cleanup = [] {}; // Leave cleanup to test harness destructor.
owned_requests_.push_back(
std::make_unique<permissions::PermissionRequestImpl>(
GetUrl(), GetUrl(), permission, user_gesture, base::BindOnce(decided),
GetUrl(), permission, user_gesture, base::BindOnce(decided),
base::BindOnce(cleanup)));
return owned_requests_.back().get();
}
......@@ -280,6 +281,7 @@ void PermissionDialogTest::ShowUi(const std::string& name) {
{"camera", ContentSettingsType::MEDIASTREAM_CAMERA},
{"protocol_handlers", ContentSettingsType::PROTOCOL_HANDLERS},
{"midi", ContentSettingsType::MIDI_SYSEX},
{"storage_access", ContentSettingsType::STORAGE_ACCESS},
{kMultipleName, ContentSettingsType::DEFAULT}};
const auto* it = std::begin(kNameToType);
for (; it != std::end(kNameToType); ++it) {
......@@ -311,6 +313,7 @@ void PermissionDialogTest::ShowUi(const std::string& name) {
case ContentSettingsType::PROTECTED_MEDIA_IDENTIFIER: // ChromeOS only.
case ContentSettingsType::PPAPI_BROKER:
case ContentSettingsType::PLUGINS: // Flash.
case ContentSettingsType::STORAGE_ACCESS:
manager->AddRequest(source_frame, MakePermissionRequest(it->type));
break;
case ContentSettingsType::DEFAULT:
......@@ -999,6 +1002,11 @@ IN_PROC_BROWSER_TEST_F(PermissionDialogTest, InvokeUi_midi) {
ShowAndVerifyUi();
}
// Host wants to access storage from the site in which it's embedded.
IN_PROC_BROWSER_TEST_F(PermissionDialogTest, InvokeUi_storage_access) {
ShowAndVerifyUi();
}
// Shows a permissions bubble with multiple requests.
IN_PROC_BROWSER_TEST_F(PermissionDialogTest, InvokeUi_multiple) {
ShowAndVerifyUi();
......
......@@ -29,11 +29,13 @@ TestPermissionBubbleViewDelegate::Requests() {
return requests_;
}
PermissionBubbleBrowserTest::PermissionBubbleBrowserTest() {
GURL TestPermissionBubbleViewDelegate::GetEmbeddingOrigin() const {
return GURL("https://embedder.example.com");
}
PermissionBubbleBrowserTest::~PermissionBubbleBrowserTest() {
}
PermissionBubbleBrowserTest::PermissionBubbleBrowserTest() = default;
PermissionBubbleBrowserTest::~PermissionBubbleBrowserTest() = default;
void PermissionBubbleBrowserTest::SetUpOnMainThread() {
ExtensionBrowserTest::SetUpOnMainThread();
......
......@@ -34,6 +34,8 @@ class TestPermissionBubbleViewDelegate
const std::vector<permissions::PermissionRequest*>& Requests() override;
GURL GetEmbeddingOrigin() const override;
void Accept() override {}
void Deny() override {}
void Closing() override {}
......
......@@ -1362,7 +1362,7 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
// request bubble resulting in top chrome unhiding.
auto decided = [](ContentSetting) {};
permissions::PermissionRequestImpl permission_request(
url, url, ContentSettingsType::GEOLOCATION, true /* user_gesture */,
url, ContentSettingsType::GEOLOCATION, true /* user_gesture */,
base::BindOnce(decided), base::DoNothing() /* delete_callback */);
auto* permission_manager =
permissions::PermissionRequestManager::FromWebContents(active_contents);
......
......@@ -83,6 +83,27 @@ PermissionPromptBubbleView::PermissionPromptBubbleView(
for (permissions::PermissionRequest* request : visible_requests_)
AddPermissionRequestLine(request);
base::Optional<base::string16> extra_text = GetExtraText();
if (extra_text.has_value()) {
auto* extra_text_label =
AddChildView(std::make_unique<views::Label>(extra_text.value()));
extra_text_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
extra_text_label->SetMultiLine(true);
}
if (visible_requests_[0]->GetContentSettingsType() ==
ContentSettingsType::PLUGINS) {
auto learn_more_button = views::CreateVectorImageButton(this);
learn_more_button->SetFocusForPlatform();
learn_more_button->SetTooltipText(
l10n_util::GetStringUTF16(IDS_LEARN_MORE));
SkColor text_color = GetNativeTheme()->GetSystemColor(
ui::NativeTheme::kColorId_LabelEnabledColor);
views::SetImageFromVectorIcon(learn_more_button.get(),
vector_icons::kHelpOutlineIcon, text_color);
SetExtraView(std::move(learn_more_button));
}
}
PermissionPromptBubbleView::~PermissionPromptBubbleView() = default;
......@@ -161,40 +182,6 @@ void PermissionPromptBubbleView::AddPermissionRequestLine(
std::make_unique<views::Label>(request->GetMessageTextFragment()));
label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
label->SetMultiLine(true);
// Text that warns the user that flash will be deprecated. This text is only
// shown for flash and should be empty for all other permissions.
// TODO (crbug.com/1058401): Remove the warning text once flash is deprecated.
const auto warning_text = request->GetMessageTextWarningFragment();
if (warning_text.empty())
return;
// Should only be reached if the permission required is for flash.
DCHECK(request->GetContentSettingsType() == ContentSettingsType::PLUGINS);
auto* warning_line_container = AddChildView(std::make_unique<views::View>());
warning_line_container->SetLayoutManager(std::make_unique<views::BoxLayout>(
views::BoxLayout::Orientation::kHorizontal,
gfx::Insets(0, provider->GetDistanceMetric(
DISTANCE_SUBSECTION_HORIZONTAL_INDENT)),
provider->GetDistanceMetric(views::DISTANCE_RELATED_LABEL_HORIZONTAL)));
auto* warning_label = warning_line_container->AddChildView(
std::make_unique<views::Label>(std::move(warning_text)));
warning_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
warning_label->SetMultiLine(true);
auto learn_more_button = views::CreateVectorImageButton(this);
learn_more_button->SetFocusForPlatform();
learn_more_button->SetTooltipText(l10n_util::GetStringUTF16(IDS_LEARN_MORE));
SkColor text_color = GetNativeTheme()->GetSystemColor(
ui::NativeTheme::kColorId_LabelEnabledColor);
learn_more_button_ = learn_more_button.get();
views::SetImageFromVectorIcon(learn_more_button_,
vector_icons::kHelpOutlineIcon, text_color);
SetExtraView(std::move(learn_more_button));
}
void PermissionPromptBubbleView::UpdateAnchorPosition() {
......@@ -272,14 +259,14 @@ gfx::Size PermissionPromptBubbleView::CalculatePreferredSize() const {
void PermissionPromptBubbleView::ButtonPressed(views::Button* sender,
const ui::Event& event) {
if (sender == learn_more_button_)
chrome::AddSelectedTabWithURL(browser_,
GURL(chrome::kFlashDeprecationLearnMoreURL),
ui::PAGE_TRANSITION_LINK);
DCHECK_EQ(sender, GetExtraView());
chrome::AddSelectedTabWithURL(browser_,
GURL(chrome::kFlashDeprecationLearnMoreURL),
ui::PAGE_TRANSITION_LINK);
}
PermissionPromptBubbleView::DisplayNameOrOrigin
PermissionPromptBubbleView::GetDisplayNameOrOrigin() {
PermissionPromptBubbleView::GetDisplayNameOrOrigin() const {
DCHECK(!visible_requests_.empty());
GURL origin_url = visible_requests_[0]->GetOrigin();
......@@ -303,6 +290,27 @@ PermissionPromptBubbleView::GetDisplayNameOrOrigin() {
true /* is_origin */};
}
base::Optional<base::string16> PermissionPromptBubbleView::GetExtraText()
const {
switch (visible_requests_[0]->GetContentSettingsType()) {
case ContentSettingsType::PLUGINS:
// TODO(crbug.com/1058401): Remove this warning text once flash is
// deprecated.
return l10n_util::GetStringUTF16(IDS_FLASH_PERMISSION_WARNING_FRAGMENT);
case ContentSettingsType::STORAGE_ACCESS:
return l10n_util::GetStringFUTF16(
IDS_STORAGE_ACCESS_PERMISSION_EXPLANATION,
url_formatter::FormatUrlForSecurityDisplay(
visible_requests_[0]->GetOrigin(),
url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC),
url_formatter::FormatUrlForSecurityDisplay(
delegate_->GetEmbeddingOrigin(),
url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC));
default:
return base::nullopt;
}
}
void PermissionPromptBubbleView::AcceptPermission() {
RecordDecision();
delegate_->Accept();
......
......@@ -13,10 +13,6 @@
class Browser;
namespace views {
class ImageButton;
}
// Bubble that prompts the user to grant or deny a permission request from a
// website.
class PermissionPromptBubbleView : public views::ButtonListener,
......@@ -61,7 +57,10 @@ class PermissionPromptBubbleView : public views::ButtonListener,
// Returns the origin to be displayed in the permission prompt. May return
// a non-origin, e.g. extension URLs use the name of the extension.
DisplayNameOrOrigin GetDisplayNameOrOrigin();
DisplayNameOrOrigin GetDisplayNameOrOrigin() const;
// Get extra information to display for the permission, if any.
base::Optional<base::string16> GetExtraText() const;
// Record UMA Permissions.Prompt.TimeToDecision metric.
void RecordDecision();
......@@ -75,8 +74,6 @@ class PermissionPromptBubbleView : public views::ButtonListener,
// The requesting domain's name or origin.
const DisplayNameOrOrigin name_or_origin_;
views::ImageButton* learn_more_button_ = nullptr;
base::TimeTicks permission_requested_time_;
DISALLOW_COPY_AND_ASSIGN(PermissionPromptBubbleView);
......
......@@ -27,6 +27,7 @@ class TestDelegate : public permissions::PermissionPrompt::Delegate {
std::back_inserter(raw_requests_),
[](auto& req) { return req.get(); });
}
TestDelegate(const GURL& origin, const std::vector<std::string> names) {
std::transform(
names.begin(), names.end(), std::back_inserter(requests_),
......@@ -43,6 +44,10 @@ class TestDelegate : public permissions::PermissionPrompt::Delegate {
return raw_requests_;
}
GURL GetEmbeddingOrigin() const override {
return GURL("https://embedder.example.com");
}
void Accept() override {}
void Deny() override {}
void Closing() override {}
......@@ -112,4 +117,5 @@ TEST_F(PermissionPromptBubbleViewTest,
EXPECT_PRED_FORMAT2(::testing::IsSubstring, "CameraPanTiltZoom", title);
EXPECT_PRED_FORMAT2(::testing::IsNotSubstring, "VideoCapture", title);
}
} // namespace
......@@ -26,8 +26,8 @@ class TestPermissionRequestOwner {
bool user_gesture = true;
auto decided = [](ContentSetting) {};
request_ = std::make_unique<permissions::PermissionRequestImpl>(
GURL("https://embedder_example.test"), GURL("https://example.com"), type,
user_gesture, base::BindOnce(decided),
GURL("https://example.com"), type, user_gesture,
base::BindOnce(decided),
base::BindOnce(&TestPermissionRequestOwner::DeleteThis,
base::Unretained(this)));
}
......
......@@ -140,8 +140,21 @@ int PermissionPromptAndroid::GetIconId() const {
base::string16 PermissionPromptAndroid::GetMessageText() const {
const std::vector<permissions::PermissionRequest*>& requests =
delegate_->Requests();
if (requests.size() == 1)
return requests[0]->GetMessageText();
if (requests.size() == 1) {
if (requests[0]->GetContentSettingsType() ==
ContentSettingsType::STORAGE_ACCESS) {
return l10n_util::GetStringFUTF16(
IDS_STORAGE_ACCESS_INFOBAR_TEXT,
url_formatter::FormatUrlForSecurityDisplay(
requests[0]->GetOrigin(),
url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC),
url_formatter::FormatUrlForSecurityDisplay(
delegate_->GetEmbeddingOrigin(),
url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC));
} else {
return requests[0]->GetMessageText();
}
}
CheckValidRequestGroup(requests);
if (IsValidARCameraAccessRequestGroup(requests)) {
return l10n_util::GetStringFUTF16(
......
......@@ -370,8 +370,7 @@ void PermissionContextBase::DecidePermission(
std::unique_ptr<PermissionRequest> request_ptr =
std::make_unique<PermissionRequestImpl>(
embedding_origin, requesting_origin, content_settings_type_,
user_gesture,
requesting_origin, content_settings_type_, user_gesture,
base::BindOnce(&PermissionContextBase::PermissionDecided,
weak_factory_.GetWeakPtr(), id, requesting_origin,
embedding_origin, std::move(callback)),
......
......@@ -10,6 +10,7 @@
#include "base/callback.h"
#include "base/strings/string16.h"
#include "url/gurl.h"
namespace content {
class WebContents;
......@@ -48,6 +49,10 @@ class PermissionPrompt {
// deleted upon navigation and so on.
virtual const std::vector<PermissionRequest*>& Requests() = 0;
// Get the top-level origin currently displayed in the address bar
// associated with the requests.
virtual GURL GetEmbeddingOrigin() const = 0;
virtual void Accept() = 0;
virtual void Deny() = 0;
virtual void Closing() = 0;
......
......@@ -24,15 +24,6 @@ base::string16 PermissionRequest::GetChipText() const {
}
#endif
base::string16 PermissionRequest::GetMessageTextWarningFragment() const {
return base::string16();
}
GURL PermissionRequest::GetEmbeddingOrigin() const {
NOTREACHED();
return GURL();
}
#if defined(OS_ANDROID)
base::string16 PermissionRequest::GetQuietTitleText() const {
return base::string16();
......
......@@ -122,13 +122,6 @@ class PermissionRequest {
// be displayed next to an image and indicate the user grants the permission.
virtual base::string16 GetMessageTextFragment() const = 0;
// Returns a warning prompt text related to this permission.
virtual base::string16 GetMessageTextWarningFragment() const;
// Get the top-level origin currently displayed in the address bar associated
// with this request.
virtual GURL GetEmbeddingOrigin() const;
// Get the origin on whose behalf this permission request is being made.
virtual GURL GetOrigin() const = 0;
......
......@@ -23,14 +23,12 @@
namespace permissions {
PermissionRequestImpl::PermissionRequestImpl(
const GURL& embedding_origin,
const GURL& request_origin,
ContentSettingsType content_settings_type,
bool has_gesture,
PermissionDecidedCallback permission_decided_callback,
base::OnceClosure delete_callback)
: embedding_origin_(embedding_origin),
request_origin_(request_origin),
: request_origin_(request_origin),
content_settings_type_(content_settings_type),
has_gesture_(has_gesture),
permission_decided_callback_(std::move(permission_decided_callback)),
......@@ -151,14 +149,6 @@ base::string16 PermissionRequestImpl::GetMessageText() const {
case ContentSettingsType::AR:
message_id = IDS_AR_INFOBAR_TEXT;
break;
case ContentSettingsType::STORAGE_ACCESS:
return l10n_util::GetStringFUTF16(
IDS_STORAGE_ACCESS_INFOBAR_TEXT,
url_formatter::FormatUrlForSecurityDisplay(
GetOrigin(), url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC),
url_formatter::FormatUrlForSecurityDisplay(
GetEmbeddingOrigin(),
url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC));
default:
NOTREACHED();
return base::string16();
......@@ -237,13 +227,8 @@ base::string16 PermissionRequestImpl::GetMessageTextFragment() const {
message_id = IDS_AR_PERMISSION_FRAGMENT;
break;
case ContentSettingsType::STORAGE_ACCESS:
return l10n_util::GetStringFUTF16(
IDS_STORAGE_ACCESS_PERMISSION_FRAGMENT,
url_formatter::FormatUrlForSecurityDisplay(
GetOrigin(), url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC),
url_formatter::FormatUrlForSecurityDisplay(
GetEmbeddingOrigin(),
url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC));
message_id = IDS_STORAGE_ACCESS_PERMISSION_FRAGMENT;
break;
case ContentSettingsType::WINDOW_PLACEMENT:
message_id = IDS_WINDOW_PLACEMENT_PERMISSION_FRAGMENT;
break;
......@@ -293,16 +278,6 @@ base::string16 PermissionRequestImpl::GetChipText() const {
}
#endif
base::string16 PermissionRequestImpl::GetMessageTextWarningFragment() const {
if (content_settings_type_ == ContentSettingsType::PLUGINS)
return l10n_util::GetStringUTF16(IDS_FLASH_PERMISSION_WARNING_FRAGMENT);
return base::string16();
}
GURL PermissionRequestImpl::GetEmbeddingOrigin() const {
return embedding_origin_;
}
GURL PermissionRequestImpl::GetOrigin() const {
return request_origin_;
}
......
......@@ -24,8 +24,7 @@ class PermissionRequestImpl : public PermissionRequest {
public:
using PermissionDecidedCallback = base::OnceCallback<void(ContentSetting)>;
PermissionRequestImpl(const GURL& embedding_origin,
const GURL& request_origin,
PermissionRequestImpl(const GURL& request_origin,
ContentSettingsType content_settings_type,
bool has_gesture,
PermissionDecidedCallback permission_decided_callback,
......@@ -45,8 +44,6 @@ class PermissionRequestImpl : public PermissionRequest {
base::string16 GetChipText() const override;
#endif
base::string16 GetMessageTextFragment() const override;
base::string16 GetMessageTextWarningFragment() const override;
GURL GetEmbeddingOrigin() const override;
GURL GetOrigin() const override;
void PermissionGranted() override;
void PermissionDenied() override;
......@@ -56,7 +53,6 @@ class PermissionRequestImpl : public PermissionRequest {
PermissionRequestGestureType GetGestureType() const override;
ContentSettingsType GetContentSettingsType() const override;
GURL embedding_origin_;
GURL request_origin_;
ContentSettingsType content_settings_type_;
bool has_gesture_;
......
......@@ -338,6 +338,10 @@ const std::vector<PermissionRequest*>& PermissionRequestManager::Requests() {
return requests_;
}
GURL PermissionRequestManager::GetEmbeddingOrigin() const {
return web_contents()->GetLastCommittedURL().GetOrigin();
}
void PermissionRequestManager::Accept() {
if (deleting_bubble_)
return;
......
......@@ -130,6 +130,7 @@ class PermissionRequestManager
// PermissionPrompt::Delegate:
const std::vector<PermissionRequest*>& Requests() override;
GURL GetEmbeddingOrigin() const override;
void Accept() override;
void Deny() override;
void Closing() override;
......
......@@ -39,7 +39,11 @@ MockPermissionPrompt::MockPermissionPrompt(MockPermissionPromptFactory* factory,
// The actual prompt will call these, so test they're sane.
EXPECT_FALSE(request->GetMessageTextFragment().empty());
#if defined(OS_ANDROID)
EXPECT_FALSE(request->GetMessageText().empty());
// For STORAGE_ACCESS, the prompt itself calculates the message text.
if (request->GetContentSettingsType() !=
ContentSettingsType::STORAGE_ACCESS) {
EXPECT_FALSE(request->GetMessageText().empty());
}
EXPECT_NE(0, request->GetIconId());
#else
EXPECT_FALSE(request->GetIconId().is_empty());
......
......@@ -108,9 +108,10 @@
<message name="IDS_AR_PERMISSION_FRAGMENT" desc="Permission request shown if the user is visiting a site that wants to use AR. Follows a prompt: 'This site would like to:">
Create a 3D map of your surroundings and track camera position
</message>
<message name="IDS_STORAGE_ACCESS_PERMISSION_FRAGMENT" desc="Permission request shown if the user is visiting a site needs access to its data while it is embedded into another site. Follows a prompt: 'This site would like to:">
<message name="IDS_STORAGE_ACCESS_PERMISSION_FRAGMENT" desc="Permission request shown if the user is visiting a site needs access to its data while it is embedded into another site. Follows a prompt: 'This site would like to:'">
Access cookies and site data.
</message>
<message name="IDS_STORAGE_ACCESS_PERMISSION_EXPLANATION" desc="Explanation of the permission request shown if the user is visiting a site needs access to its data while it is embedded into another site. Follows this text: 'This site would like to: Access cookies and site data.'">
Do you want to allow <ph name="EMBEDDED_URL">$1<ex>news.site</ex></ph> to use cookies and site data on <ph name="TOP_LEVEL_URL">$2<ex>content_domain.site</ex></ph>?
This will otherwise be blocked by your privacy settings. This will allow the content you interacted with to work correctly, but may allow <ph name="EMBEDDED_URL">$1<ex>news.site</ex></ph> to track your activity.
......
10641252f274f657fb248e37ca77aa729196d120
\ No newline at end of file
10641252f274f657fb248e37ca77aa729196d120
\ No newline at end of file
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment