Fix permission chip UI in fullscreen mode

If browser is in fullscreen mode, the location bar isn't visible and
chip UI isn't visible too. This CLs implements using bubble UI in
fullscreen mode. It also fixes closing behaviour for permissions that
use bubble UI when chip feature is enabled.

Bug: 1148332
Change-Id: I7a519d0dc892f660c7ced39195338d60b4193d3d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532577
Commit-Queue: Olesia Marukhno <olesiamarukhno@google.com>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826958}

chrome/browser/ui/BUILD.gn

@@ -3889,6 +3889,7 @@ static_library("ui") {
       "views/permission_bubble/permission_prompt_bubble_view.h",
       "views/permission_bubble/permission_prompt_impl.cc",
       "views/permission_bubble/permission_prompt_impl.h",
+      "views/permission_bubble/permission_prompt_style.h",
       "views/profiles/avatar_toolbar_button.cc",
       "views/profiles/avatar_toolbar_button.h",
       "views/profiles/avatar_toolbar_button_delegate.cc",

chrome/browser/ui/views/bubble_anchor_util_views.cc

@@ -49,8 +49,9 @@ AnchorConfiguration GetPageInfoAnchorConfiguration(Browser* browser,
 
 AnchorConfiguration GetPermissionPromptBubbleAnchorConfiguration(
     Browser* browser) {
-  if (base::FeatureList::IsEnabled(permissions::features::kPermissionChip)) {
-    BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
+  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
+  if (base::FeatureList::IsEnabled(permissions::features::kPermissionChip) &&
+      browser_view->GetLocationBarView()->IsDrawn()) {
     return {browser_view->GetLocationBarView(),
             browser_view->GetLocationBarView()->permission_chip()->button(),
             views::BubbleBorder::TOP_LEFT};

chrome/browser/ui/views/location_bar/permission_chip.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/permission_bubble/permission_prompt_bubble_view.h"
+#include "chrome/browser/ui/views/permission_bubble/permission_prompt_style.h"
 #include "components/permissions/permission_request.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/vector_icons/vector_icons.h"
@@ -198,8 +199,8 @@ void PermissionChip::ChipButtonPressed() {
   // deactivation.
   DCHECK(!prompt_bubble_);
 
-  prompt_bubble_ =
-      new PermissionPromptBubbleView(browser_, delegate_, requested_time_);
+  prompt_bubble_ = new PermissionPromptBubbleView(
+      browser_, delegate_, requested_time_, PermissionPromptStyle::kChip);
   prompt_bubble_->Show();
   prompt_bubble_->GetWidget()->AddObserver(this);
   // Restart the timer after user clicks on the chip to open the bubble.

chrome/browser/ui/views/permission_bubble/permission_prompt_bubble_view.cc

@@ -20,7 +20,6 @@
 #include "chrome/browser/ui/views/title_origin_label.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
-#include "components/permissions/features.h"
 #include "components/permissions/permission_request.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/url_formatter/elide_url.h"
@@ -43,7 +42,8 @@
 PermissionPromptBubbleView::PermissionPromptBubbleView(
     Browser* browser,
     permissions::PermissionPrompt::Delegate* delegate,
-    base::TimeTicks permission_requested_time)
+    base::TimeTicks permission_requested_time,
+    PermissionPromptStyle prompt_style)
     : browser_(browser),
       delegate_(delegate),
       visible_requests_(GetVisibleRequests()),
@@ -67,10 +67,11 @@ PermissionPromptBubbleView::PermissionPromptBubbleView(
   SetCancelCallback(base::BindOnce(&PermissionPromptBubbleView::DenyPermission,
                                    base::Unretained(this)));
 
-  // If the permission chip feature is enabled, the chip is indicating the
-  // pending permission request and so the bubble can be opened and closed
-  // repeatedly.
-  if (!base::FeatureList::IsEnabled(permissions::features::kPermissionChip)) {
+  // If bubble hanging off the padlock icon, with no chip showing, it shouldn't
+  // close on deactivate and it should stick until user makes a decision.
+  // Otherwise, the chip is indicating the pending permission request and so the
+  // bubble can be opened and closed repeatedly.
+  if (prompt_style == PermissionPromptStyle::kBubbleOnly) {
     set_close_on_deactivate(false);
     DialogDelegate::SetCloseCallback(
         base::BindOnce(&PermissionPromptBubbleView::ClosingPermission,

chrome/browser/ui/views/permission_bubble/permission_prompt_bubble_view.h

@@ -7,6 +7,7 @@
 
 #include "base/macros.h"
 #include "base/strings/string16.h"
+#include "chrome/browser/ui/views/permission_bubble/permission_prompt_style.h"
 #include "components/permissions/permission_prompt.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
 
@@ -18,7 +19,8 @@ class PermissionPromptBubbleView : public views::BubbleDialogDelegateView {
  public:
   PermissionPromptBubbleView(Browser* browser,
                              permissions::PermissionPrompt::Delegate* delegate,
-                             base::TimeTicks permission_requested_time);
+                             base::TimeTicks permission_requested_time,
+                             PermissionPromptStyle prompt_style);
   ~PermissionPromptBubbleView() override;
 
   void Show();

chrome/browser/ui/views/permission_bubble/permission_prompt_bubble_view_unittest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/permission_bubble/permission_prompt_bubble_view.h"
+#include "chrome/browser/ui/views/permission_bubble/permission_prompt_style.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/views/chrome_views_test_base.h"
 #include "components/permissions/permission_util.h"
@@ -67,7 +68,8 @@ class TestDelegate : public permissions::PermissionPrompt::Delegate {
 TEST_F(PermissionPromptBubbleViewTest, AccessibleTitleMentionsPermissions) {
   TestDelegate delegate(GURL("https://test.origin"), {"foo", "bar"});
   auto bubble = std::make_unique<PermissionPromptBubbleView>(
-      nullptr, &delegate, base::TimeTicks::Now());
+      nullptr, &delegate, base::TimeTicks::Now(),
+      PermissionPromptStyle::kBubbleOnly);
 
   EXPECT_PRED_FORMAT2(::testing::IsSubstring, "foo",
                       base::UTF16ToUTF8(bubble->GetAccessibleWindowTitle()));
@@ -78,7 +80,8 @@ TEST_F(PermissionPromptBubbleViewTest, AccessibleTitleMentionsPermissions) {
 TEST_F(PermissionPromptBubbleViewTest, AccessibleTitleMentionsOrigin) {
   TestDelegate delegate(GURL("https://test.origin"), {"foo", "bar"});
   auto bubble = std::make_unique<PermissionPromptBubbleView>(
-      nullptr, &delegate, base::TimeTicks::Now());
+      nullptr, &delegate, base::TimeTicks::Now(),
+      PermissionPromptStyle::kBubbleOnly);
 
   // Note that the scheme is not usually included.
   EXPECT_PRED_FORMAT2(::testing::IsSubstring, "test.origin",
@@ -90,7 +93,8 @@ TEST_F(PermissionPromptBubbleViewTest,
   TestDelegate delegate(GURL("https://test.origin"),
                         {"foo", "bar", "baz", "quxx"});
   auto bubble = std::make_unique<PermissionPromptBubbleView>(
-      nullptr, &delegate, base::TimeTicks::Now());
+      nullptr, &delegate, base::TimeTicks::Now(),
+      PermissionPromptStyle::kBubbleOnly);
 
   const auto title = base::UTF16ToUTF8(bubble->GetAccessibleWindowTitle());
   EXPECT_PRED_FORMAT2(::testing::IsSubstring, "foo", title);
@@ -103,7 +107,8 @@ TEST_F(PermissionPromptBubbleViewTest,
        AccessibleTitleFileSchemeMentionsThisFile) {
   TestDelegate delegate(GURL("file:///tmp/index.html"), {"foo", "bar"});
   auto bubble = std::make_unique<PermissionPromptBubbleView>(
-      nullptr, &delegate, base::TimeTicks::Now());
+      nullptr, &delegate, base::TimeTicks::Now(),
+      PermissionPromptStyle::kBubbleOnly);
 
   EXPECT_PRED_FORMAT2(::testing::IsSubstring,
                       base::UTF16ToUTF8(l10n_util::GetStringUTF16(
@@ -117,7 +122,8 @@ TEST_F(PermissionPromptBubbleViewTest,
                          ContentSettingsType::MEDIASTREAM_CAMERA,
                          ContentSettingsType::CAMERA_PAN_TILT_ZOOM});
   auto bubble = std::make_unique<PermissionPromptBubbleView>(
-      nullptr, &delegate, base::TimeTicks::Now());
+      nullptr, &delegate, base::TimeTicks::Now(),
+      PermissionPromptStyle::kBubbleOnly);
 
   const auto title = base::UTF16ToUTF8(bubble->GetAccessibleWindowTitle());
   EXPECT_PRED_FORMAT2(::testing::IsSubstring, "AudioCapture", title);

chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc

@@ -7,7 +7,6 @@
 #include <memory>
 
 #include "chrome/browser/content_settings/chrome_content_settings_utils.h"
-#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/permission_bubble/permission_prompt.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
@@ -20,15 +19,6 @@
 #include "content/public/browser/web_contents.h"
 #include "ui/views/bubble/bubble_frame_view.h"
 
-enum class PermissionPromptImpl::PromptStyle {
-  // The permission prompt bubble is shown directly.
-  kBubble,
-  // The permission chip view in the location bar.
-  kChip,
-  // The prompt as an indicator in the right side of the omnibox.
-  kQuiet
-};
-
 std::unique_ptr<permissions::PermissionPrompt> CreatePermissionPrompt(
     content::WebContents* web_contents,
     permissions::PermissionPrompt::Delegate* delegate) {
@@ -45,30 +35,30 @@ std::unique_ptr<permissions::PermissionPrompt> CreatePermissionPrompt(
 PermissionPromptImpl::PermissionPromptImpl(Browser* browser,
                                            content::WebContents* web_contents,
                                            Delegate* delegate)
-    : prompt_bubble_(nullptr), web_contents_(web_contents) {
+    : prompt_bubble_(nullptr),
+      web_contents_(web_contents),
+      delegate_(delegate),
+      browser_(browser) {
   permissions::PermissionRequestManager* manager =
       permissions::PermissionRequestManager::FromWebContents(web_contents_);
   if (manager->ShouldCurrentRequestUseQuietUI()) {
-    prompt_style_ = PromptStyle::kQuiet;
+    prompt_style_ = PermissionPromptStyle::kQuiet;
     // Shows the prompt as an indicator in the right side of the omnibox.
     content_settings::UpdateLocationBarUiForWebContents(web_contents_);
   } else {
     LocationBarView* lbv = GetLocationBarView();
     std::vector<permissions::PermissionRequest*> requests =
         delegate->Requests();
+    const bool should_use_chip_ui = std::all_of(
+        requests.begin(), requests.end(),
+        [](auto* request) { return request->GetChipText().has_value(); });
     if (base::FeatureList::IsEnabled(permissions::features::kPermissionChip) &&
-        lbv && std::all_of(requests.begin(), requests.end(), [](auto* request) {
-          return request->GetChipText().has_value();
-        })) {
+        lbv && lbv->IsDrawn() && should_use_chip_ui) {
       permission_chip_ = lbv->permission_chip();
       permission_chip_->Show(delegate);
-      prompt_style_ = PromptStyle::kChip;
+      prompt_style_ = PermissionPromptStyle::kChip;
     } else {
-      prompt_bubble_ = new PermissionPromptBubbleView(browser, delegate,
-                                                      base::TimeTicks::Now());
-      prompt_bubble_->Show();
-      prompt_bubble_->GetWidget()->AddObserver(this);
-      prompt_style_ = PromptStyle::kBubble;
+      ShowBubble();
     }
   }
 }
@@ -83,31 +73,37 @@ PermissionPromptImpl::~PermissionPromptImpl() {
   if (prompt_bubble_)
     prompt_bubble_->GetWidget()->Close();
 
-  if (prompt_style_ == PromptStyle::kQuiet) {
+  if (prompt_style_ == PermissionPromptStyle::kQuiet) {
     // Hides the quiet prompt.
     content_settings::UpdateLocationBarUiForWebContents(web_contents_);
   }
 
-  if (prompt_style_ == PromptStyle::kChip && permission_chip_) {
+  if (prompt_style_ == PermissionPromptStyle::kChip && permission_chip_) {
     permission_chip_->Hide();
   }
   CHECK(!IsInObserverList());
 }
 
 void PermissionPromptImpl::UpdateAnchorPosition() {
+  // TODO(olesiamarukhno): Figure out if we need to switch UI styles (chip to
+  // bubble and vice versa) here.
   if (prompt_bubble_)
     prompt_bubble_->UpdateAnchorPosition();
 }
 
 LocationBarView* PermissionPromptImpl::GetLocationBarView() {
-  Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
-  if (!browser)
-    return nullptr;
-
-  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
+  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_);
   return browser_view ? browser_view->GetLocationBarView() : nullptr;
 }
 
+void PermissionPromptImpl::ShowBubble() {
+  prompt_style_ = PermissionPromptStyle::kBubbleOnly;
+  prompt_bubble_ = new PermissionPromptBubbleView(
+      browser_, delegate_, base::TimeTicks::Now(), prompt_style_);
+  prompt_bubble_->Show();
+  prompt_bubble_->GetWidget()->AddObserver(this);
+}
+
 permissions::PermissionPrompt::TabSwitchingBehavior
 PermissionPromptImpl::GetTabSwitchingBehavior() {
   return permissions::PermissionPrompt::TabSwitchingBehavior::
@@ -117,11 +113,11 @@ PermissionPromptImpl::GetTabSwitchingBehavior() {
 permissions::PermissionPromptDisposition
 PermissionPromptImpl::GetPromptDisposition() const {
   switch (prompt_style_) {
-    case PromptStyle::kBubble:
+    case PermissionPromptStyle::kBubbleOnly:
       return permissions::PermissionPromptDisposition::ANCHORED_BUBBLE;
-    case PromptStyle::kChip:
+    case PermissionPromptStyle::kChip:
       return permissions::PermissionPromptDisposition::LOCATION_BAR_LEFT_CHIP;
-    case PromptStyle::kQuiet: {
+    case PermissionPromptStyle::kQuiet: {
       permissions::PermissionRequestManager* manager =
           permissions::PermissionRequestManager::FromWebContents(web_contents_);
       return permissions::NotificationPermissionUiSelector::

chrome/browser/ui/views/permission_bubble/permission_prompt_impl.h

@@ -6,7 +6,9 @@
 #define CHROME_BROWSER_UI_VIEWS_PERMISSION_BUBBLE_PERMISSION_PROMPT_IMPL_H_
 
 #include "base/macros.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
+#include "chrome/browser/ui/views/permission_bubble/permission_prompt_style.h"
 #include "components/permissions/permission_prompt.h"
 
 class Browser;
@@ -41,10 +43,10 @@ class PermissionPromptImpl : public permissions::PermissionPrompt,
   void OnWidgetClosing(views::Widget* widget) override;
 
  private:
-  enum class PromptStyle;
-
   LocationBarView* GetLocationBarView();
 
+  void ShowBubble();
+
   // The popup bubble. Not owned by this class; it will delete itself when a
   // decision is made.
   PermissionPromptBubbleView* prompt_bubble_;
@@ -52,10 +54,14 @@ class PermissionPromptImpl : public permissions::PermissionPrompt,
   // The web contents whose location bar should show the quiet prompt.
   content::WebContents* web_contents_;
 
-  PromptStyle prompt_style_;
+  PermissionPromptStyle prompt_style_;
 
   PermissionChip* permission_chip_ = nullptr;
 
+  permissions::PermissionPrompt::Delegate* const delegate_;
+
+  Browser* const browser_;
+
   DISALLOW_COPY_AND_ASSIGN(PermissionPromptImpl);
 };
 

chrome/browser/ui/views/permission_bubble/permission_prompt_style.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_PERMISSION_BUBBLE_PERMISSION_PROMPT_STYLE_H_
+#define CHROME_BROWSER_UI_VIEWS_PERMISSION_BUBBLE_PERMISSION_PROMPT_STYLE_H_
+
+// Represents permission prompt UI styles on desktop.
+enum class PermissionPromptStyle {
+  // The permission prompt bubble is shown directly.
+  kBubbleOnly,
+  // The permission chip view in the location bar.
+  kChip,
+  // The prompt as an indicator in the right side of the omnibox.
+  kQuiet
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_PERMISSION_BUBBLE_PERMISSION_PROMPT_STYLE_H_