Center permission bubble if location bar is hidden in MacOS.

chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm

Index: chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm
diff --git a/chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm b/chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm
new file mode 100644
index 0000000000000000000000000000000000000000..c5f1deff2a49d85ef5cde6f4a21b35c66e4d8ba2
--- /dev/null
+++ b/chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browsertest.mm
@@ -0,0 +1,297 @@
+// Copyright 2015 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.
+
+#include "base/command_line.h"
+#include "base/stl_util.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/first_run/first_run.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_iterator.h"
+#import "chrome/browser/ui/cocoa/browser_window_controller.h"
+#import "chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h"
+#include "chrome/browser/ui/startup/startup_browser_creator_impl.h"
+#include "chrome/browser/ui/startup/startup_types.h"
+#include "chrome/browser/ui/website_settings/mock_permission_bubble_request.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/grit/generated_resources.h"
+#include "extensions/browser/extension_registry.h"
+#include "net/dns/mock_host_resolver.h"
+#import "ui/base/cocoa/fullscreen_window_manager.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/l10n/l10n_util_mac.h"
+
+// To be used when getting the anchor with a location bar. The bubble should be
+// in the top-right corner.
+const int kTolerance = 200;

[groby-ooo-7-16, 2015/04/02 22:21:13]

I don't think 200 pixels is an acceptable tolerance. Why so high?

[hcarmona, 2015/04/04 01:04:23]

Made tolerance smaller and moved my expected point closer to the lock icon.
Is there a constant that I can use?

+
+class TestPermissionBubbleCocoa : public PermissionBubbleCocoa {
+ public:
+  TestPermissionBubbleCocoa(NSWindow* parent_window)
+      : PermissionBubbleCocoa(parent_window) {}
+
+  // Returns the base class value for testing |HasLocationBar|.
+  bool HasLocationBar() override {
+    return PermissionBubbleCocoa::HasLocationBar();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestPermissionBubbleCocoa);
+};
+
+class MockPermissionBubbleCocoa : public PermissionBubbleCocoa {

[groby-ooo-7-16, 2015/04/02 22:21:12]

FWIW: You can just add a mock class via gmock.h

class MockBubble : public CocoaBubble {
 public:
   MockBubble(...) {...}
   MOCK_METHOD0(HasLocationBar,bool());
};

And then where it's used:

ON_CALL(mockInstance, HasLocationBar).WillByDefault(Return(true));  // replace
with false if necessary.

Less code, more flavor ;)

[hcarmona, 2015/04/04 01:04:22]

Awesome! Done.

+ public:
+  MockPermissionBubbleCocoa(NSWindow* parent_window)
+      : PermissionBubbleCocoa(parent_window), location_bar_present_(false) {}
+
+  // Sets the delegate value for testing |GetAnchorPoint|.
+  void set_location_bar_present(bool value) { location_bar_present_ = value; }
+
+ protected:
+  bool location_bar_present_;
+
+  // Override to mock location bar in tests.
+  bool HasLocationBar() override { return location_bar_present_; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockPermissionBubbleCocoa);
+};
+
+class TestPermissionBubbleViewDelegate : public PermissionBubbleView::Delegate {

[groby-ooo-7-16, 2015/04/02 22:21:12]

Since this does exactly squat, gmock to the rescue - scrap the class, and then
use

testing::NiceMock<PermissionBubbleView::Delegate> delegate_;

in the test fixture.

[hcarmona, 2015/04/04 01:04:22]

Done.

+ public:
+  TestPermissionBubbleViewDelegate() : PermissionBubbleView::Delegate() {}
+
+  void ToggleAccept(int, bool) override {}
+  void Accept() override {}
+  void Deny() override {}
+  void Closing() override {}
+  void SetView(PermissionBubbleView* view) override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestPermissionBubbleViewDelegate);
+};
+
+// Inherit from ExtensionBrowserTest instead of InProcessBrowserTest because
+// testing app mode requires extensions.
+class PermissionBubbleCocoaBrowsertest : public ExtensionBrowserTest {
+ public:
+  PermissionBubbleCocoaBrowsertest()
+      : ExtensionBrowserTest(), controller_(nullptr) {}
+
+  void SetUpOnMainThread() override {
+    ExtensionBrowserTest::SetUpOnMainThread();
+
+    // Add a single permission request
+    MockPermissionBubbleRequest* request = new MockPermissionBubbleRequest(
+        "Request 1", l10n_util::GetStringUTF8(IDS_PERMISSION_ALLOW),
+        l10n_util::GetStringUTF8(IDS_PERMISSION_DENY));
+    requests_.push_back(request);
+  }
+
+  void TearDownOnMainThread() override {
+    if (browser()->tab_strip_model())
+      browser()->tab_strip_model()->CloseSelectedTabs();
+
+    if (controller_)
+      [controller_ close];
+
+    STLDeleteElements(&requests_);

[groby-ooo-7-16, 2015/04/02 22:21:12]

Use a ScopedVector instead.

[hcarmona, 2015/04/04 01:04:22]

Done.

+    ExtensionBrowserTest::TearDownOnMainThread();
+  }
+
+  virtual PermissionBubbleCocoa* CreatePermissionBubble(
+      NSWindow* parent_window) {
+    return new TestPermissionBubbleCocoa(parent_window);
+  }
+
+  bool HasLocationBar() {
+    return static_cast<TestPermissionBubbleCocoa*>(permission_bubble_.get())
+        ->HasLocationBar();
+  }
+
+  void ShowPermissionBubble(Browser* cur_browser) {
+    controller_ = [[BrowserWindowController alloc] initWithBrowser:cur_browser

[groby-ooo-7-16, 2015/04/02 22:21:13]

Why create one?

This sequence should yield the BWC for the current browser (since BrowserTests
always have a browser, they also have a BWC ready-made)

    NSWindow* window = browser()->window()->GetNativeWindow();
    BrowserWindowController* bwc =
        [BrowserWindowController browserWindowControllerForWindow:window];

No need to worry about closing that way.

Actually, you don't even need the BWC. Just use the NSWindow from the first call
as parent window. 

[hcarmona, 2015/04/04 01:04:22]

Done.

+                                                     takeOwnership:NO];
+    [controller_ showWindow:nil];
+
+    permission_bubble_.reset(CreatePermissionBubble([controller_ window]));
+    permission_delegate_.reset(new TestPermissionBubbleViewDelegate());

[groby-ooo-7-16, 2015/04/02 22:21:12]

The mock already takes care of this, but: Why do you new all the things? Unless
you need to dynamically create, prefer static allocation.

[hcarmona, 2015/04/04 01:04:22]

Done.

+    permission_bubble_->SetDelegate(permission_delegate_.get());
+
+    permission_bubble_->Show(requests_, accept_states_);
+  }
+
+ protected:
+  BrowserWindowController* controller_;
+  scoped_ptr<PermissionBubbleCocoa> permission_bubble_;
+  scoped_ptr<TestPermissionBubbleViewDelegate> permission_delegate_;
+  std::vector<PermissionBubbleRequest*> requests_;
+  std::vector<bool> accept_states_;
+};
+
+// Default Window Test /////////////////////////////////////////////////////////
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaBrowsertest,
+                       HasLocationBarByDefault) {
+  ShowPermissionBubble(browser());
+  EXPECT_TRUE(HasLocationBar());
+}
+
+// Fullscreen Test /////////////////////////////////////////////////////////////
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaBrowsertest,
+                       FullscreenHasLocationBar) {
+  ShowPermissionBubble(browser());
+
+  base::scoped_nsobject<FullscreenWindowManager> manager(
+      [[FullscreenWindowManager alloc] initWithWindow:[controller_ window]
+                                        desiredScreen:[NSScreen mainScreen]]);
+  EXPECT_TRUE(HasLocationBar());
+  [manager enterFullscreenMode];
+  EXPECT_TRUE(HasLocationBar());
+  [manager exitFullscreenMode];
+  EXPECT_TRUE(HasLocationBar());
+}
+
+// App Test ////////////////////////////////////////////////////////////////////
+
+class PermissionBubbleCocoaAppTest : public PermissionBubbleCocoaBrowsertest {
+ public:
+  PermissionBubbleCocoaAppTest()
+      : PermissionBubbleCocoaBrowsertest(), app_browser_(nullptr) {}
+
+  void TearDownOnMainThread() override {
+    if (app_browser_)
+      app_browser_->tab_strip_model()->CloseSelectedTabs();
+
+    PermissionBubbleCocoaBrowsertest::TearDownOnMainThread();
+  }
+
+  // Only one app can be opened per test.
+  bool OpenExtensionAppWindow(const extensions::Extension* extension) {

[groby-ooo-7-16, 2015/04/02 22:21:13]

Question: Can this code be shared in a util place? I feel we do that in several
tests?

[hcarmona, 2015/04/04 01:04:23]

Would it make sense to move it to the ExtensionBrowserTest or should it be its
own thing?

+    // On MacOS, it's not possible to enter app mode with the new bookmark apps
+    // enabled.

[hcarmona, 2015/04/02 01:30:14]

I'm currently looking into this. Spoke with benwells@, he mentioned that this is
a bug, but I can't repro on canary. I'm not sure if this is caused by my change
yet.

[groby-ooo-7-16, 2015/04/02 22:21:12]

Well, then let's not land this until we know we're not causing the issue.

Also, even if we're not: Any way to opt out of the bookmark apps being enabled
via command line switch?

[hcarmona, 2015/04/04 01:04:22]

Issue is not related to this change, created bug 473394.
New code is agnostic to bookmark apps because we specify the container.

+    if (extensions::util::IsNewBookmarkAppsEnabled())
+      return false;
+
+    base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
+    command_line.AppendSwitchASCII(switches::kAppId, extension->id());
+
+    chrome::startup::IsFirstRun first_run =

[groby-ooo-7-16, 2015/04/02 22:21:13]

Why do we care about first_run status? Just launch it with IS_NOT_FIRST_RUN?

Or do we want to see a different behavior in the first run case?

[hcarmona, 2015/04/04 01:04:22]

Code no longer needed in this test.

+        first_run::IsChromeFirstRun() ? chrome::startup::IS_FIRST_RUN
+                                      : chrome::startup::IS_NOT_FIRST_RUN;
+    StartupBrowserCreatorImpl launch(base::FilePath(), command_line, first_run);
+
+    if (!launch.OpenApplicationWindow(browser()->profile(), NULL))
+      return false;
+
+    unsigned int browser_count = chrome::GetBrowserCount(
+        browser()->profile(), browser()->host_desktop_type());
+
+    if (browser_count != 2)

[groby-ooo-7-16, 2015/04/02 22:21:12]

Please ASSERT here - that way, we know what exactly went wrong. 

[hcarmona, 2015/04/04 01:04:22]

Code changed so we get the browser and don't need to guess.

+      return false;
+
+    for (chrome::BrowserIterator it; !it.done(); it.Next()) {
+      if (*it != browser()) {
+        app_browser_ = *it;
+        return true;
+      }
+    }
+
+    NOTREACHED();
+    return false;
+  }
+
+  Browser* app_browser() { return app_browser_; }
+
+ private:
+  Browser* app_browser_;
+};
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaAppTest, AppHasNoLocationBar) {
+  auto extension = LoadExtension(test_data_dir_.AppendASCII("packaged_app/"));
+
+  ASSERT_TRUE(extension);
+  ASSERT_TRUE(OpenExtensionAppWindow(extension));
+
+  ShowPermissionBubble(app_browser());
+
+  EXPECT_TRUE(app_browser()->is_app());
+  EXPECT_FALSE(HasLocationBar());
+}
+
+// Kiosk Test //////////////////////////////////////////////////////////////////
+
+class PermissionBubbleCocoaKioskTest : public PermissionBubbleCocoaBrowsertest {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    PermissionBubbleCocoaBrowsertest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kKioskMode);
+  }
+};
+
+// http://crbug.com/470724
+// Kiosk mode on Mac has a location bar but it shouldn't.
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaKioskTest,
+                       DISABLED_KioskHasNoLocationBar) {
+  ShowPermissionBubble(browser());
+  EXPECT_FALSE(HasLocationBar());
+}
+
+// Anchor Position Tests ///////////////////////////////////////////////////////
+
+class PermissionBubbleCocoaAnchorBrowsertest
+    : public PermissionBubbleCocoaBrowsertest {
+ protected:
+  PermissionBubbleCocoa* CreatePermissionBubble(
+      NSWindow* parent_window) override {
+    return new MockPermissionBubbleCocoa(parent_window);
+  }
+
+  void SetLocationBarPresent(bool present) {
+    static_cast<MockPermissionBubbleCocoa*>(permission_bubble_.get())
+        ->set_location_bar_present(present);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaAnchorBrowsertest,
+                       AnchorPositionWithLocationBar) {
+  ShowPermissionBubble(browser());
+  SetLocationBarPresent(true);
+
+  NSPoint anchor = permission_bubble_->GetAnchorPoint();
+  NSWindow* window = [controller_ window];
+  NSRect frame = [window frame];
+
+  // Expected anchor location will be top left when there's a location bar.
+  NSPoint expected = NSMakePoint(0, frame.size.height);

[groby-ooo-7-16, 2015/04/02 22:21:13]

You want NSMinX(frame), NSMaxY(frame) (it's not guaranteed the frame starts at
0,0)

[hcarmona, 2015/04/04 01:04:22]

Done.

+  expected = [window convertBaseToScreen:expected];
+
+  // Anchor should be close to the left of the screen.
+  EXPECT_GE(expected.x + kTolerance, anchor.x);
+  EXPECT_LE(expected.x, anchor.x);
+
+  // Anchor should be close to the top of the screen.
+  EXPECT_GE(expected.y, anchor.y);
+  EXPECT_LE(expected.y - kTolerance, anchor.y);

[groby-ooo-7-16, 2015/04/02 22:21:12]

That means exepcted.y should be within kTolerance of anchor.y, more or less,
correct? 

ASSERT_NEAR(expected.y, anchor.y, kTolerance)

[hcarmona, 2015/04/04 01:04:22]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaAnchorBrowsertest,
+                       AnchorPositionWithoutLocationBar) {
+  ShowPermissionBubble(browser());
+  SetLocationBarPresent(false);
+
+  NSPoint anchor = permission_bubble_->GetAnchorPoint();
+  NSWindow* window = [controller_ window];
+  NSRect frame = [window frame];
+
+  // Expected anchor location will be top center when there's no location bar.
+  NSPoint expected = NSMakePoint(frame.size.width / 2, frame.size.height);
+  expected = [window convertBaseToScreen:expected];
+
+  // Anchor should be centered.
+  EXPECT_EQ(expected.x, anchor.x);

[groby-ooo-7-16, 2015/04/02 22:21:12]

EXPECT_TRUE(NSEqualPoints(expected, anchor));

[hcarmona, 2015/04/04 01:04:23]

Done.

+  EXPECT_EQ(expected.y, anchor.y);
+}