Add a shortcut to open the Apps page from the bookmark bar.

chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[tfarina, 2013/02/26 19:27:51]

no (c)

[tfarina, 2013/02/26 19:27:51]

btw we already have bookmark_bar_view_test.cc!

Yeah, I plan to rename it to _unittest.cc, but never get around to do it yet.

[MAD, 2013/02/26 20:55:53]

Done.

[MAD, 2013/02/26 20:55:53]

But the other one is in interactive_ui_tests, you want to convert it as a
unittest?

[tfarina, 2013/02/26 21:14:21]

Ah, in that case I'd rename to _uitest.cc

[MAD, 2013/02/26 21:20:06]

Do you want me to do it in this CL, or you'll do it later?

[tfarina, 2013/02/26 22:34:02]

No, leave it to me. I'll consider doing it in another CL.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search_engines/template_url_service.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/ui/search/search.h"
+#include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"

[tfarina, 2013/02/26 19:27:51]

make this the first include.

[Alexei Svitkine (slow), 2013/02/26 19:32:30]

No, that's incorrect - against the style guide in fact. Test files aren't
special in this regard, you only have a first include if there's a .h file with
the same name (e.g. if there was a bookmarks_bar_view_unittest.h).

Yes, some tests break that, but we've slowly been moving away from that.

[tfarina, 2013/02/26 19:35:26]

No, actually we have been moving the other way around. At least that is the way
I'm used and reviewers haven't complained about doing it.

[Alexei Svitkine (slow), 2013/02/26 19:48:24]

Well, all the tests I've reviewed recently didn't use this convention.

I haven't seen a discussion about it and as it stands, it's contrary to the
style guide. This suggests that it's the wrong direction. (i.e. we want to
follow the style guide as close as possible)

The reason that we include the corresponding header file first in the impl file
is to ensure that the header can compile on its own and doesn't have a hidden
dependency against some previous include. (The origin of this came from google3
when they made this transition per this motivation.)

Considering that for any test file, there's already a corresponding impl file
that does include the header first, there's no reason to mandate this in the
test file too, since that's already tested.

So I don't see a good reason to diverge from the style guide here.

[MAD, 2013/02/26 20:55:53]

Done.

+#include "chrome/common/pref_names.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+
+typedef BrowserWithTestWindowTest BookmarkBarViewTest;
+

[tfarina, 2013/02/26 19:27:51]

can you wrap in unnamed namespace?

[MAD, 2013/02/26 20:55:53]

If I do so, I can't set the test as friend of the bookmark bar view and can't
access the private method. Would you rather I make the method public?

[tfarina, 2013/02/26 21:14:21]

No, private is better, leave that way so ;)

+// Verify that the apps shortcut is never visible without instant extended.
+TEST_F(BookmarkBarViewTest, NoAppsShortcutWithoutInstantExtended) {
+  BookmarkBarView bookmark_bar_view(browser(), NULL);
+  bookmark_bar_view.set_owned_by_client();
+  EXPECT_FALSE(bookmark_bar_view.IsAppsShortcutVisible());
+  browser()->profile()->GetPrefs()->SetBoolean(
+      prefs::kShowAppsShortcutInBookmarkBar, true);
+  EXPECT_FALSE(bookmark_bar_view.IsAppsShortcutVisible());
+}
+
+class BookmarkBarViewInstantExtendedTest : public BrowserWithTestWindowTest {
+ public:
+  BookmarkBarViewInstantExtendedTest() {
+    chrome::search::EnableInstantExtendedAPIForTesting();
+  }
+
+ protected:
+  virtual TestingProfile* CreateProfile() OVERRIDE {
+    TestingProfile* profile = BrowserWithTestWindowTest::CreateProfile();
+    // TemplateURLService is normally NULL during testing. Instant extended
+    // needs this service so set a custom factory function.
+    TemplateURLServiceFactory::GetInstance()->SetTestingFactory(
+        profile, &BookmarkBarViewInstantExtendedTest::CreateTemplateURLService);
+    return profile;
+  }
+
+ private:
+  static ProfileKeyedService* CreateTemplateURLService(Profile* profile) {
+    return new TemplateURLService(profile);
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkBarViewInstantExtendedTest);
+};
+
+// Verify that in instant extended mode the visibility of the apps shortcut
+// button properly follows the pref value.
+TEST_F(BookmarkBarViewInstantExtendedTest, AppsShortcutVisibility) {
+  BookmarkBarView bookmark_bar_view(browser(), NULL);
+  bookmark_bar_view.set_owned_by_client();
+  browser()->profile()->GetPrefs()->SetBoolean(
+      prefs::kShowAppsShortcutInBookmarkBar, false);
+  EXPECT_FALSE(bookmark_bar_view.IsAppsShortcutVisible());
+  browser()->profile()->GetPrefs()->SetBoolean(
+      prefs::kShowAppsShortcutInBookmarkBar, true);
+  EXPECT_TRUE(bookmark_bar_view.IsAppsShortcutVisible());

[Alexei Svitkine (slow), 2013/02/26 19:29:51]

Nit: Set the bool to false again to make sure it can transition from true ->
false as well.

[MAD, 2013/02/26 20:55:53]

Done.

+}