Center permission bubble if location bar is hidden in MacOS.

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

+// 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

[groby-ooo-7-16, 2015/04/01 00:19:14]

General Q: How much of this testing framework (for Kiosk mode etc.) can be
shared with Views?

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

Most of these classes look like they can be shared with views if I refactor the
cocoa specifics into a delegate. Where would be a good place for the shared
code?

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

These classes are now split so only the tests are cocoa specific :-)

Setup should work for views

+// in the top-right corner.
+const int kTolerance = 200;
+
+class TestPermissionBubbleCocoa: public PermissionBubbleCocoa {

[groby-ooo-7-16, 2015/04/01 00:19:14]

Space in front of ':'.

Does git cl format not catch that?

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

Had not run git cl format. I've fixed the formatting issues.

+ public:
+  TestPermissionBubbleCocoa(NSWindow* parent_window):

[groby-ooo-7-16, 2015/04/01 00:19:14]

space in front of ':'

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

Done.

+    PermissionBubbleCocoa(parent_window),
+    location_bar_present_(false) {}
+
+  // Returns the base class value for testing |HasLocationBar|.

[groby-ooo-7-16, 2015/04/01 00:19:14]

It seems you either want to mock, or you don't, but never both - consider having
two test fixtures, both for clarity and to avoid naming awkwardness.

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

Done.

+  bool BaseHasLocationBar() { return PermissionBubbleCocoa::HasLocationBar(); }
+
+  // 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_; }

[groby-ooo-7-16, 2015/04/01 00:19:14]

private:
  DISALLOW_COPY_AND_ASSIGN...

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

Done.

+};
+
+class TestPermissionBubbleViewDelegate: public PermissionBubbleView::Delegate {
+  void ToggleAccept(int, bool) override {};
+  void Accept() override {};
+  void Deny() override {};
+  void Closing() override {};
+  void SetView(PermissionBubbleView* view) override {};
+};
+
+// Inherit from ExtensionBrowserTest instead of InProcessBrowserTest because
+// testing app mode requires extensions.
+class PermissionBubbleCocoaBrowsertest : public ExtensionBrowserTest {

[groby-ooo-7-16, 2015/04/01 00:19:14]

Huh. I had no idea ExtensionBrowserTest stuff lived in BrowserTest, too. Oh
well. 

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

I'm a bit confused about what I should do here...

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

Nothing. I would've just expected ExtensionBrowserTest files to have a different
suffix and was surprised. Ignore :)

+ 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_);
+    ExtensionBrowserTest::TearDownOnMainThread();
+  }
+
+  void ShowPermissionBubble(Browser* cur_browser) {
+    controller_ = [[BrowserWindowController alloc] initWithBrowser:cur_browser
+                                                     takeOwnership:NO];
+    [controller_ showWindow:nil];
+
+    permission_bubble_.reset(
+        new TestPermissionBubbleCocoa([controller_ window]));
+    permission_delegate_.reset(new TestPermissionBubbleViewDelegate());
+    permission_bubble_->SetDelegate(permission_delegate_.get());
+
+    permission_bubble_->Show(requests_, accept_states_);
+  }
+
+ protected:
+  BrowserWindowController* controller_;
+  scoped_ptr<TestPermissionBubbleCocoa> permission_bubble_;
+  scoped_ptr<TestPermissionBubbleViewDelegate> permission_delegate_;
+  std::vector<PermissionBubbleRequest*> requests_;
+  std::vector<bool> accept_states_;
+};
+
+// Normal Window Test //////////////////////////////////////////////////////////
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaBrowsertest, NormalHasLocationBar) {

[groby-ooo-7-16, 2015/04/01 00:19:14]

"Normal" is an unclear naming choice. "HasLocationBarByDefault", maybe?

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

Done.

+  ShowPermissionBubble(browser());
+  EXPECT_TRUE(permission_bubble_->BaseHasLocationBar());
+}
+
+// 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(permission_bubble_->BaseHasLocationBar());
+  [manager enterFullscreenMode];
+  EXPECT_TRUE(permission_bubble_->BaseHasLocationBar());
+  [manager exitFullscreenMode];
+  EXPECT_TRUE(permission_bubble_->BaseHasLocationBar());
+}
+
+// App Test ////////////////////////////////////////////////////////////////////
+
+class PermissionBubbleCocoaAppTest : public PermissionBubbleCocoaBrowsertest {
+ public:
+  PermissionBubbleCocoaAppTest():
+    PermissionBubbleCocoaBrowsertest() {}
+
+  // Returns the app extension aptly named "App Test".
+  const extensions::Extension* GetExtension() {

[groby-ooo-7-16, 2015/04/01 00:19:14]

GetTestExtension? Also, are we guaranteed to have that extension installed?

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

Refactored so this method is no longer needed. Also changed the app that's
loaded from "app" to "packaged_app" to avoid setting up a local test server.

+    extensions::ExtensionRegistry* registry =
+        extensions::ExtensionRegistry::Get(browser()->profile());
+    for (const scoped_refptr<const extensions::Extension>& extension :
+         registry->enabled_extensions()) {
+      if (extension->name() == "App Test")
+        return extension.get();
+    }
+    NOTREACHED();
+    return NULL;
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaAppTest, AppHasNoLocationBar) {
+  ASSERT_TRUE(test_server()->Start());
+
+  // There should be one tab to start with.
+  ASSERT_EQ(1, browser()->tab_strip_model()->count());
+
+  // Load an app.
+  host_resolver()->AddRule("www.example.com", "127.0.0.1");
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("app/")));
+  const extensions::Extension* extension_app = GetExtension();
+
+  base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
+  command_line.AppendSwitchASCII(switches::kAppId, extension_app->id());
+
+  chrome::startup::IsFirstRun first_run = first_run::IsChromeFirstRun() ?
+      chrome::startup::IS_FIRST_RUN : chrome::startup::IS_NOT_FIRST_RUN;
+  StartupBrowserCreatorImpl launch(base::FilePath(), command_line, first_run);
+
+  bool new_bookmark_apps_enabled = extensions::util::IsNewBookmarkAppsEnabled();
+
+  // If the new bookmark app flow is enabled, the app should open as a tab.
+  // Otherwise the app should open as an app window.
+  EXPECT_EQ(!new_bookmark_apps_enabled,
+            launch.OpenApplicationWindow(browser()->profile(), NULL));
+  EXPECT_EQ(new_bookmark_apps_enabled,
+            launch.OpenApplicationTab(browser()->profile()));
+
+  // Check that a the number of browsers and tabs is correct.
+  unsigned int expected_browsers = 1;
+  int expected_tabs = 1;
+  new_bookmark_apps_enabled ? expected_tabs++ : expected_browsers++;
+
+  EXPECT_EQ(expected_browsers,
+            chrome::GetBrowserCount(browser()->profile(),
+                                    browser()->host_desktop_type()));
+  EXPECT_EQ(expected_tabs, browser()->tab_strip_model()->count());
+
+  Browser* cur_browser = browser();
+  if (expected_browsers == 2) {
+    for (chrome::BrowserIterator it; !it.done(); it.Next()) {
+      if (*it == browser())
+        continue;
+      else
+        cur_browser = *it;
+    }
+
+    EXPECT_NE(browser(), cur_browser);
+  }
+
+  ShowPermissionBubble(cur_browser);

[groby-ooo-7-16, 2015/04/01 00:19:14]

Up until here, this seems to be a verbatim copy from browser_browsertest.cc -
please don't program via copy/paste.

1) Is that really _all_ required to put us into app mode?
2) Regardless of yes or no, it might be worth extracting into a helper function
- it looks like quite a few tests would like to force app mode.

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

No, not everything is necessary. Refactored test to remove anything not
required.
Good idea. Should that be a pre-requisite for this CL or should it be another CL
after this one?

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

Follow-up CL is fine, but please file a crbug  for it

[hcarmona, 2015/04/13 21:13:24]

The test that goes into app mode is now its own class in a shared location. Do
we need to refactor this further?

[groby-ooo-7-16, 2015/04/13 21:19:45]

Possibly, if others are using it. I wouldn't mind if you filed a follow-up bug.

+
+  // When bookmark apps are enabled, the browser that we have will not be an
+  // app: this means that it should have a location bar.
+  EXPECT_NE(new_bookmark_apps_enabled, cur_browser->is_app());

[groby-ooo-7-16, 2015/04/01 00:19:14]

So this doesn't quite know if it tests in app mode or not. After all,
new_bookmark_apps_enabled could be false or true here.

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

Updated test so it doesn't try to guess if app mode is possible. Test will fail
if no app mode.

+  EXPECT_EQ(new_bookmark_apps_enabled,
+            permission_bubble_->BaseHasLocationBar());

[groby-ooo-7-16, 2015/04/01 00:19:14]

Wait, the name says "AppHasNoLocationBar", but this test ensures that if
apps_enabled is true, we do have a location bar - one or the other is wrong.

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

Updated test so test matches the name.

+
+  if (cur_browser->tab_strip_model())
+    cur_browser->tab_strip_model()->CloseSelectedTabs();

[groby-ooo-7-16, 2015/04/01 00:19:14]

Why?

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

This should be in TearDownOnMainThread: moved there.
This is necessary because BrowserWindowController verifies that the
tab_strip_model is empty on windowWillClose.

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

It already was there, that's why I asked :)

+}
+
+// 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(permission_bubble_->BaseHasLocationBar());
+}
+
+// Anchor Position Tests ///////////////////////////////////////////////////////
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaBrowsertest,
+                       AnchorPositionWithLocationBar) {
+  ShowPermissionBubble(browser());
+  permission_bubble_->set_location_bar_present(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);
+  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);
+}
+
+IN_PROC_BROWSER_TEST_F(PermissionBubbleCocoaBrowsertest,
+                       AnchorPositionWithoutLocationBar) {
+  ShowPermissionBubble(browser());
+  permission_bubble_->set_location_bar_present(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);
+  EXPECT_EQ(expected.y, anchor.y);
+}