Implement fullscreen info bubble on Win and Mac

chrome/browser/ui/views/fullscreen_exit_bubble_views.cc

Index: chrome/browser/ui/views/fullscreen_exit_bubble_views.cc
diff --git a/chrome/browser/ui/views/fullscreen_exit_bubble_views.cc b/chrome/browser/ui/views/fullscreen_exit_bubble_views.cc
index 239dbf4966522d31adf79ccc18c828d01f2fbbff..6c0b7b1b9b2eb6b365a812ea39a9d7abe6200f2f 100644
--- a/chrome/browser/ui/views/fullscreen_exit_bubble_views.cc
+++ b/chrome/browser/ui/views/fullscreen_exit_bubble_views.cc
@@ -7,6 +7,7 @@
 #include "base/message_loop.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/ui/views/bubble/bubble.h"
 #include "grit/generated_resources.h"
 #include "ui/base/animation/slide_animation.h"
 #include "ui/base/keycodes/keyboard_codes.h"
@@ -14,6 +15,8 @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas_skia.h"
 #include "ui/gfx/screen.h"
+#include "views/bubble/bubble_border.h"
+#include "views/controls/button/text_button.h"
 #include "views/controls/link.h"
 #include "views/widget/widget.h"
 
@@ -23,28 +26,56 @@
 
 // FullscreenExitView ----------------------------------------------------------
 
-class FullscreenExitBubbleViews::FullscreenExitView : public views::View {
+class FullscreenExitBubbleViews::FullscreenExitView : public views::View,
+    public views::ButtonListener {

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Must be aligned with other parent class declaration.  You'll probably need
to move the ": public views::View" part down a line and indent it 4.

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

  public:
   FullscreenExitView(FullscreenExitBubbleViews* bubble,
-                     const std::wstring& accelerator);
+                     const std::wstring& accelerator,
+                     const GURL& url,
+                     bool ask_permission);
   virtual ~FullscreenExitView();
 
   // views::View
   virtual gfx::Size GetPreferredSize();
 
+  // views::ButtonListener

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Can this be private?

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+  virtual void ButtonPressed(views::Button* sender, const views::Event& event);
+
+  virtual void HideButtons();

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Is this really virtual?  Seems like no?  Also needs a comment about what it
does (or more precisely why).

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+
  private:
+  string16 GetMessage(const GURL& url);
+  // Space between the site info label and the buttons / link.

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Blank line above this

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+  static const int kMiddlePaddingPx = 30;
+
   // views::View
   virtual void Layout();
-  virtual void OnPaint(gfx::Canvas* canvas);
+  // virtual void OnPaint(gfx::Canvas* canvas);

[Peter Kasting, 2011/10/11 23:08:32]

Don't comment out declarations -- remove it entirely.

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+
+  FullscreenExitBubbleViews* bubble_;
 
   // Clickable hint text to show in the bubble.
   views::Link link_;
+  views::Label message_label_;
+  views::NativeTextButton* accept_button_;
+  views::NativeTextButton* deny_button_;
+
+  bool show_buttons_;
 };
 
 FullscreenExitBubbleViews::FullscreenExitView::FullscreenExitView(
     FullscreenExitBubbleViews* bubble,
-    const std::wstring& accelerator) {
+    const std::wstring& accelerator,
+    const GURL& url,
+    bool ask_permission)
+    : bubble_(bubble),
+      accept_button_(NULL),
+      deny_button_(NULL),
+      show_buttons_(ask_permission) {
+  set_focusable(false);
   link_.set_parent_owned(false);
+  link_.set_collapse_when_hidden(false);
+  link_.set_focusable(false);
 #if !defined(OS_CHROMEOS)
   link_.SetText(
       l10n_util::GetStringFUTF16(IDS_EXIT_FULLSCREEN_MODE,
@@ -54,59 +85,138 @@ FullscreenExitBubbleViews::FullscreenExitView::FullscreenExitView(
 #endif
   link_.set_listener(bubble);
   link_.SetFont(ResourceBundle::GetSharedInstance().GetFont(
-      ResourceBundle::LargeFont));
-  link_.SetNormalColor(SK_ColorWHITE);
-  link_.SetHighlightedColor(SK_ColorWHITE);
+      ResourceBundle::MediumFont));
+  link_.SetNormalColor(SK_ColorBLACK);

[Peter Kasting, 2011/10/11 23:08:32]

There are potential accessibility issues here.  You're not explicitly setting
the colors of the labels, just the link.  That means you should probably set the
link to use the same colors as the message label instead of assuming it should
be black.

http://codereview.chromium.org/8221027/ will be modifying this code and the code
that you need to call in order to implement my suggestion above.  Once that
lands you'll need to explicitly give the link and the label the appropriate
background color.  The best way for you to accomplish that and my other
suggestions is for you to move the set_background() call below to the top of
this constructor, then do something like:

  link_. ...(set up other stuff)...
  link_.SetBackgroundColor(background()->get_color());
  link_.SetNormalColor(message_label_.enabled_color());
  link_.SetPressedColor(message_label_.enabled_color());

  message_label_. ...(set up other stuff)...
  message_label_.SetBackgroundColor(background()->get_color());

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Cool. I'll make this change once your patch lands.

[Peter Kasting, 2011/10/12 23:09:21]

This has now landed.

+  link_.SetHighlightedColor(SK_ColorBLACK);
+
+  message_label_.set_parent_owned(false);
+  message_label_.SetText(
+    GetMessage(url));
+  message_label_.SetFont(ResourceBundle::GetSharedInstance().GetFont(
+      ResourceBundle::MediumFont));
+  AddChildView(&message_label_);
   AddChildView(&link_);
+
+  views::BubbleBorder* bubble_border =

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Instead of manually creating a bubble border, can we just use the bubble
class outright?  That would simplify this file.  (See also below.)

[koz (OOO until 15th September), 2011/10/12 05:38:23]

There were a few reasons why we abandoned our attempts to use Bubble here:
- Bubble animates differently to how we want to here - it fades but we slide
- Bubble auto-cleans itself up when it is hidden, but we want to be able to
slide the view in and out.
- Bubble auto-hides when it loses focus (we did get around this by adding a flag
to Bubble)

It became quite tricky when we tried to instantiate new Bubbles whenever the
base class (FullscreenExitBubble) called Show() / Hide(), as the base class also
needs to measure the size of the exit bubble when it is in the hidden state to
implement the auto-hide logic.

+      new views::BubbleBorder(views::BubbleBorder::NONE);
+  bubble_border->set_background_color(Bubble::kBackgroundColor);
+  set_background(new views::BubbleBackground(bubble_border));
+  set_border(bubble_border);
+
+  accept_button_ = new views::NativeTextButton(this,
+      UTF16ToWide(l10n_util::GetStringUTF16(IDS_FULLSCREEN_INFOBAR_ALLOW)));
+  AddChildView(accept_button_);
+
+  deny_button_ = new views::NativeTextButton(this,
+      UTF16ToWide(l10n_util::GetStringUTF16(IDS_FULLSCREEN_INFOBAR_DENY)));
+  AddChildView(deny_button_);
+  accept_button_->set_focusable(false);
+  deny_button_->set_focusable(false);
+  link_.SetVisible(false);
+  if (!show_buttons_) {

[Peter Kasting, 2011/10/11 23:08:32]

Nit: No {}

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+    HideButtons();
+  }
+}
+
+string16 FullscreenExitBubbleViews::FullscreenExitView::GetMessage(
+    const GURL& url) {
+    UTF16ToWide(

[Peter Kasting, 2011/10/11 23:08:32]

This line is erroneous.

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+  if (url.empty()) {
+    return l10n_util::GetStringUTF16(
+        IDS_FULLSCREEN_INFOBAR_USER_ENTERED_FULLSCREEN);
+  } else if (url.SchemeIsFile()) {

[Peter Kasting, 2011/10/11 23:08:32]

Nit: No else after return (2 places)

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+    return l10n_util::GetStringUTF16(IDS_FULLSCREEN_INFOBAR_FILE_PAGE_NAME);
+  } else {
+    return
+        l10n_util::GetStringFUTF16(IDS_FULLSCREEN_INFOBAR_REQUEST_PERMISSION,
+          UTF8ToUTF16(url.host()));
+  }
 }
 
 FullscreenExitBubbleViews::FullscreenExitView::~FullscreenExitView() {
 }
 
+void FullscreenExitBubbleViews::FullscreenExitView::ButtonPressed(
+    views::Button* sender, const views::Event& event) {
+  if (sender == accept_button_)
+    bubble_->OnAcceptFullscreen();
+  else
+    bubble_->OnCancelFullscreen();
+}
+
+void FullscreenExitBubbleViews::FullscreenExitView::HideButtons() {
+  show_buttons_ = false;
+  accept_button_->SetVisible(false);
+  deny_button_->SetVisible(false);
+  link_.SetVisible(true);
+}
+
 gfx::Size FullscreenExitBubbleViews::FullscreenExitView::GetPreferredSize() {
-  gfx::Size preferred_size(link_.GetPreferredSize());
-  preferred_size.Enlarge(kPaddingPx * 2, kPaddingPx * 2);
-  return preferred_size;
+  gfx::Size link_preferred_size(link_.GetPreferredSize());
+  gfx::Size site_info_preferred_size(message_label_.GetPreferredSize());
+  gfx::Size accept_preferred_size(accept_button_->GetPreferredSize());
+  gfx::Size deny_preferred_size(deny_button_->GetPreferredSize());
+  gfx::Insets insets(GetInsets());
+
+  int buttons_width = accept_preferred_size.width() + kPaddingPx +
+      deny_preferred_size.width();
+  int button_box_width = std::max(buttons_width, link_preferred_size.width());
+  int width = kPaddingPx + site_info_preferred_size.width() + kMiddlePaddingPx +
+      button_box_width + kPaddingPx;
+
+  gfx::Size result(width + insets.width(),
+      kPaddingPx * 2 + accept_preferred_size.height() + insets.height());
+  return result;
 }
 
 void FullscreenExitBubbleViews::FullscreenExitView::Layout() {
   gfx::Size link_preferred_size(link_.GetPreferredSize());

[Peter Kasting, 2011/10/11 23:08:32]

Did you consider using a layout manager like GridLayout to take care of your
layout and sizing functions?

[koz (OOO until 15th September), 2011/10/12 05:38:23]

I'm not sure if that would work here as we need to make button_box_width the
larger of the two buttons and the link behind it. Also, hiding the buttons and
showing the link shouldn't cause the bubble to change size.

-  link_.SetBounds(kPaddingPx,
-                  height() - kPaddingPx - link_preferred_size.height(),
-                  link_preferred_size.width(), link_preferred_size.height());
-}
+  gfx::Size site_info_preferred_size(message_label_.GetPreferredSize());
+  gfx::Size accept_preferred_size(accept_button_->GetPreferredSize());
+  gfx::Size deny_preferred_size(deny_button_->GetPreferredSize());
+  gfx::Insets insets(GetInsets());
+
+  int inner_height = height() - insets.height();
+  int button_box_x = insets.left() + kPaddingPx +
+      site_info_preferred_size.width() + kMiddlePaddingPx;
+  int site_info_y = insets.top() +
+      (inner_height - site_info_preferred_size.height()) / 2;
+  int link_x = width() - insets.right() - kPaddingPx -
+      link_preferred_size.width();
+  int link_y = insets.top() + (inner_height - link_preferred_size.height()) / 2;
 
-void FullscreenExitBubbleViews::FullscreenExitView::OnPaint(
-    gfx::Canvas* canvas) {
-  // Create a round-bottomed rect to fill the whole View.
-  SkRect rect;
-  SkScalar padding = SkIntToScalar(kPaddingPx);
-  // The "-padding" top coordinate ensures that the rect is always tall enough
-  // to contain the complete rounded corner radius.  If we set this to 0, as the
-  // popup slides offscreen (in reality, squishes to 0 height), the corners will
-  // flatten out as the height becomes less than the corner radius.
-  rect.set(0, -padding, SkIntToScalar(width()), SkIntToScalar(height()));
-  SkScalar rad[8] = { 0, 0, 0, 0, padding, padding, padding, padding };
-  SkPath path;
-  path.addRoundRect(rect, rad, SkPath::kCW_Direction);
-
-  // Fill it black.
-  SkPaint paint;
-  paint.setStyle(SkPaint::kFill_Style);
-  paint.setFlags(SkPaint::kAntiAlias_Flag);
-  paint.setColor(SK_ColorBLACK);
-  canvas->GetSkCanvas()->drawPath(path, paint);
+  message_label_.SetBounds(insets.left() + kPaddingPx,
+                       site_info_y,
+                       site_info_preferred_size.width(),
+                       site_info_preferred_size.height());
+  link_.SetBounds(link_x,

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Wrapping here is odd

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+                  link_y,
+                  link_preferred_size.width(), link_preferred_size.height());
+  if (show_buttons_) {
+    accept_button_->SetBounds(button_box_x,
+                              insets.top() + kPaddingPx,
+                              accept_preferred_size.width(),
+                              accept_preferred_size.height());
+    deny_button_->SetBounds(
+        button_box_x + accept_preferred_size.width() + kPaddingPx,
+        insets.top() + kPaddingPx,
+        deny_preferred_size.width(),
+        deny_preferred_size.height());
+  }
 }
 
 // FullscreenExitBubbleViews ---------------------------------------------------
 
 FullscreenExitBubbleViews::FullscreenExitBubbleViews(
     views::Widget* frame,

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Move up to previous line and align other args even

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

-    CommandUpdater::CommandUpdaterDelegate* delegate)
-    : FullscreenExitBubble(delegate),
+    Browser* browser,
+    const GURL& url,
+    bool ask_permission)
+    : FullscreenExitBubble(browser),
       root_view_(frame->GetRootView()),
       popup_(NULL),
-      size_animation_(new ui::SlideAnimation(this)) {
+      size_animation_(new ui::SlideAnimation(this)),
+      url_(url) {
   size_animation_->Reset(1);
 
   // Create the contents view.
@@ -114,7 +224,8 @@ FullscreenExitBubbleViews::FullscreenExitBubbleViews(
   bool got_accelerator = frame->GetAccelerator(IDC_FULLSCREEN, &accelerator);
   DCHECK(got_accelerator);
   view_ = new FullscreenExitView(
-      this, UTF16ToWideHack(accelerator.GetShortcutText()));
+      this, UTF16ToWideHack(accelerator.GetShortcutText()), url,

[Peter Kasting, 2011/10/11 23:08:32]

Nit: I think you can fit this all on 2 lines if you move the first arg up to the
previous line

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+      ask_permission);
 
   // Initialize the popup.
   popup_ = new views::Widget;
@@ -125,11 +236,14 @@ FullscreenExitBubbleViews::FullscreenExitBubbleViews(
   params.parent = frame->GetNativeView();
   params.bounds = GetPopupRect(false);
   popup_->Init(params);
+  gfx::Size size = GetPopupRect(true).size();
   popup_->SetContentsView(view_);
-  popup_->SetOpacity(static_cast<unsigned char>(0xff * kOpacity));

[Peter Kasting, 2011/10/11 23:08:32]

If you're removing this you should kill the kOpacity constant entirely. 
Furthermore, the popup doesn't need to be transparent, and in fact we could
maybe (?) get by without a separate popup at all and just use a completely
standard bubble.

[koz (OOO until 15th September), 2011/10/12 05:38:23]

I've removed the constant (thanks), but as I talked about above the standard
bubble doesn't seem to be suitable for this UI.

+  popup_->GetRootView()->SetLayoutManager(NULL);

[Peter Kasting, 2011/10/11 23:08:32]

Why is this needed?

[koz (OOO until 15th September), 2011/10/12 05:38:23]

I've added a comment.

+  view_->SetBounds(0, 0, size.width(), size.height());
   popup_->Show();  // This does not activate the popup.
 
-  StartWatchingMouse();
+  if (!ask_permission)
+    StartWatchingMouse();
 }
 
 FullscreenExitBubbleViews::~FullscreenExitBubbleViews() {
@@ -151,6 +265,16 @@ void FullscreenExitBubbleViews::LinkClicked(
   ToggleFullscreen();
 }
 
+void FullscreenExitBubbleViews::OnAcceptFullscreen() {
+  AcceptFullscreen(url_);
+  view_->HideButtons();
+  StartWatchingMouse();
+}
+
+void FullscreenExitBubbleViews::OnCancelFullscreen() {
+  CancelFullscreen();
+}
+
 void FullscreenExitBubbleViews::AnimationProgressed(
     const ui::Animation* animation) {
   gfx::Rect popup_rect(GetPopupRect(false));
@@ -158,9 +282,11 @@ void FullscreenExitBubbleViews::AnimationProgressed(
     popup_->Hide();
   } else {
     popup_->SetBounds(popup_rect);
+    view_->SetY(popup_rect.height() - view_->height());
     popup_->Show();
   }
 }
+
 void FullscreenExitBubbleViews::AnimationEnded(
     const ui::Animation* animation) {
   AnimationProgressed(animation);
@@ -198,16 +324,29 @@ gfx::Point FullscreenExitBubbleViews::GetCursorScreenPoint() {
 gfx::Rect FullscreenExitBubbleViews::GetPopupRect(
     bool ignore_animation_state) const {
   gfx::Size size(view_->GetPreferredSize());
-  if (!ignore_animation_state) {
-    size.set_height(static_cast<int>(static_cast<double>(size.height()) *
-        size_animation_->GetCurrentValue()));
-  }
   // NOTE: don't use the bounds of the root_view_. On linux changing window
   // size is async. Instead we use the size of the screen.
   gfx::Rect screen_bounds = gfx::Screen::GetMonitorAreaNearestWindow(
       root_view_->GetWidget()->GetNativeView());
   gfx::Point origin(screen_bounds.x() +
                     (screen_bounds.width() - size.width()) / 2,
-                    screen_bounds.y());
+                    kPopupTopPx + screen_bounds.y());
+  if (!ignore_animation_state) {
+    int total_height = size.height() + kPopupTopPx;
+    int animation_height = static_cast<int>(

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Use size_animation_.CurrentValueBetween().

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Done.

+        static_cast<double>(total_height) *
+        (1.0f - size_animation_->GetCurrentValue()));
+    int offset_delta, height_delta;
+    if (animation_height <= kPopupTopPx) {

[Peter Kasting, 2011/10/11 23:08:32]

Nit: Simpler:

  int y_offset = std::min(animation_height, kPopupTopPx);
  size.set_height(size.height() - animation_height +   y_offset);
  origin.set_y(origin.y() - y_offset);

You can probably get even simpler by making animation_height go 0->total_height
instead of total_height->0, starting |origin| at the screen y, and reversing the
signs of various calculations above.

However, I'm concerned with this -- it seems like it means the bubble will slide
to the top of the screen and then compress down until it disappears instead of
sliding offscreen entirely, which would presumably look better (and the reverse
on showing rather than hiding).  Plus I don't know if the bubble border draws
correctly at very small heights.

Shouldn't instead this just animate the bubble on/offscreen by moving the y
coordinate?

[koz (OOO until 15th September), 2011/10/12 05:38:23]

Yes, that would be the best way to do it, but we were concerned that a negative
y coordinate would put the popup outside the browser (eg: multiple monitor
environments) - will it not do that?

Also, because the border is on the contents view and we clear the layout manager
for the popup, the border won't change size while the popup shrinks.

[Peter Kasting, 2011/10/12 07:38:40]

Test and see.  Grab two monitors and tell Windows that one is positioned above
the other, then change the patch to do something like this and see what happens.

+      offset_delta = animation_height;
+      height_delta = 0;
+    } else {
+      offset_delta = kPopupTopPx;
+      height_delta = animation_height - kPopupTopPx;
+    }
+
+    size.set_height(size.height() - height_delta);
+    origin.set_y(origin.y() - offset_delta);
+  }
   return gfx::Rect(origin, size);
 }