While trying to enable webview sign-in by default, I found a bunch of issues

chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc

+// Copyright 2013 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 "chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate.h"
+
+#include "base/basictypes.h"
+#include "base/command_line.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/test_extension_service.h"
+#include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/browser/ui/bookmarks/bookmark_bubble_delegate.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/singleton_tabs.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/test/test_utils.h"
+#include "ui/events/event_constants.h"
+#include "ui/gfx/range/range.h"
+
+class BookmarkBubbleSignInDelegateTest : public InProcessBrowserTest {
+ public:
+  BookmarkBubbleSignInDelegateTest() {}
+
+  Profile* profile() { return browser()->profile(); }
+
+  void ReplaceBlank(Browser* browser);
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BookmarkBubbleSignInDelegateTest);
+};
+
+void BookmarkBubbleSignInDelegateTest::ReplaceBlank(Browser* browser) {
+  // The default browser created for tests start with one tab open on

[sky, 2015/01/06 15:48:42]

Move description above declaration.

[Roger Tawa OOO till Jul 10th, 2015/01/06 22:34:59]

Done.

+  // about:blank.  The sign-in page is a singleton that will
+  // replace this tab.  This function replaces about:blank with another URL
+  // so that the sign in page goes into a new tab.
+  chrome::NavigateParams params(
+      chrome::GetSingletonTabNavigateParams(browser, GURL("chrome:version")));
+  params.path_behavior = chrome::NavigateParams::IGNORE_AND_NAVIGATE;
+  chrome::ShowSingletonTabOverwritingNTP(browser, params);

[sky, 2015/01/06 15:48:42]

Are you sure you don't need to wait for the navigation here?

[Roger Tawa OOO till Jul 10th, 2015/01/06 22:34:59]

Does not seem like it is needed.  Even though chrome:version will load
asynchronously, the current URL of the tab is updated to chrome:version, so the
sign in page does not consider it about:blank.

+}
+
+IN_PROC_BROWSER_TEST_F(BookmarkBubbleSignInDelegateTest, OnSignInLinkClicked) {
+  ReplaceBlank(browser());
+  int starting_tab_count = browser()->tab_strip_model()->count();
+
+  scoped_ptr<BookmarkBubbleDelegate> delegate;

[sky, 2015/01/06 15:48:42]

Is there a reason you need to wrap in scoped_ptr instead of declaration
BookmarkBubbleSignInDelegate on the stack?

[Roger Tawa OOO till Jul 10th, 2015/01/06 22:34:59]

The dtor of BookmarkBubbleSignInDelegate is private, so it cannot be
instantiated on the stack.

+  delegate.reset(new BookmarkBubbleSignInDelegate(browser()));
+
+  delegate->OnSignInLinkClicked();
+
+  // A new tab should have been opened and the browser should be visible.
+  content::RunAllPendingInMessageLoop();

[sky, 2015/01/06 15:48:42]

Why do you need the RunAllPending in all of these?

[Roger Tawa OOO till Jul 10th, 2015/01/06 22:34:59]

Not needed.  All removed except line 118.

+  EXPECT_EQ(starting_tab_count + 1, browser()->tab_strip_model()->count());
+}
+
+IN_PROC_BROWSER_TEST_F(BookmarkBubbleSignInDelegateTest,
+                       OnSignInLinkClickedReusesBlank) {
+  int starting_tab_count = browser()->tab_strip_model()->count();
+
+  scoped_ptr<BookmarkBubbleDelegate> delegate;
+  delegate.reset(new BookmarkBubbleSignInDelegate(browser()));
+
+  delegate->OnSignInLinkClicked();
+
+  // A new tab should have been opened and the browser should be visible.
+  content::RunAllPendingInMessageLoop();
+  EXPECT_EQ(starting_tab_count, browser()->tab_strip_model()->count());
+}
+
+IN_PROC_BROWSER_TEST_F(BookmarkBubbleSignInDelegateTest,
+                       OnSignInLinkClickedIncognito) {
+  ReplaceBlank(browser());
+  Browser* incognito_browser = CreateIncognitoBrowser();
+
+  int starting_tab_count_normal = browser()->tab_strip_model()->count();
+  int starting_tab_count_incognito =
+      incognito_browser->tab_strip_model()->count();
+
+  scoped_ptr<BookmarkBubbleDelegate> delegate;
+  delegate.reset(new BookmarkBubbleSignInDelegate(incognito_browser));
+
+  delegate->OnSignInLinkClicked();
+
+  content::RunAllPendingInMessageLoop();
+
+  // A new tab should have been opened in the normal browser, which should be
+  // visible.
+  int tab_count_normal = browser()->tab_strip_model()->count();
+  EXPECT_EQ(starting_tab_count_normal + 1, tab_count_normal);
+
+  // No effect is expected on the incognito browser.
+  int tab_count_incognito = incognito_browser->tab_strip_model()->count();
+  EXPECT_EQ(starting_tab_count_incognito, tab_count_incognito);
+}
+
+// Verifies that the sign in page can be loaded in a different browser
+// if the provided browser is invalidated.
+IN_PROC_BROWSER_TEST_F(BookmarkBubbleSignInDelegateTest, BrowserRemoved) {
+  // Create an extra browser.
+  Browser* extra_browser = CreateBrowser(profile());
+  ReplaceBlank(extra_browser);
+
+  int starting_tab_count = extra_browser->tab_strip_model()->count();
+
+  scoped_ptr<BookmarkBubbleDelegate> delegate;
+  delegate.reset(new BookmarkBubbleSignInDelegate(browser()));
+
+  BrowserList::SetLastActive(extra_browser);
+
+  browser()->tab_strip_model()->CloseAllTabs();
+  content::RunAllPendingInMessageLoop();
+
+  delegate->OnSignInLinkClicked();
+
+  // A new tab should have been opened in the extra browser, which should be
+  // visible.
+  content::RunAllPendingInMessageLoop();
+  int tab_count = extra_browser->tab_strip_model()->count();
+  EXPECT_EQ(starting_tab_count + 1, tab_count);
+}