Alternate NTP: Move bookmark bar to bottom

chrome/browser/ui/search/search.cc

 // Copyright (c) 2012 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/search/search.h"
 
 #include "base/command_line.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 
+namespace {
+
+static bool s_is_instant_extended_api_enabled_for_testing = false;

[dhollowa, 2012/12/04 01:24:49]

Let's not do this.

Prefer to add:

  CommandLine* command_line = CommandLine::ForCurrentProcess();
  command_line->AppendSwitch(switches::kEnableInstantExtendedAPI);

in the tests themselves.

[sail, 2012/12/04 01:41:12]

Unfotunately that's not enough since the function tests more than the command
line switch. 

For example, this test just skips testing all together if setting the command
line doesn't work:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/policy/policy_...

I think my way is much cleaner.

[dhollowa, 2012/12/04 01:56:16]

If you sync, you'll find that it is enough now.  I got rid of the channel check.

But, maybe you're right...  could you convert those other tests over to use your
new |EnableInstantExtendedAPIForTesting()| call too please?

On 2012/12/04 01:41:12, sail wrote:

[sail, 2012/12/04 04:26:30]

Done.

I had one big oversight in my previous code. Since this flag is a static it
doesn't get reset between tests. To fix this I changed the function to take a
bool so that tests could clear the static at the end of the test.

It's a little more ugly now but it works. Let me know if you'd rather have me go
back to using the command line switch instead.

[dhollowa, 2012/12/04 05:17:53]

Gosh yes, that is pretty ugly.  Not to rat-hole here but how about we get rid of
the boolean argument to EnableInstantExtendedAPIForTesting and just have it set
the command line flag.  This gives us the improvement of keeping a clear API for
testing so if IsInstantExtendedAPIEnabled changes it will be localized to this
file how to tweak the tests too.  That's an improvement, but we can keep the
tests simpler with the single call.

[sail, 2012/12/04 18:00:07]

Done.
Good idea!

+
+}
+
 namespace chrome {
 namespace search {
 
 bool IsInstantExtendedAPIEnabled(const Profile* profile) {
+  if (s_is_instant_extended_api_enabled_for_testing)
+    return true;
+
   return VersionInfo::GetChannel() <= VersionInfo::CHANNEL_DEV &&
       !profile->IsOffTheRecord() &&
       CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableInstantExtendedAPI);
 }
 
+void EnableInstantExtendedAPIForTesting() {
+  s_is_instant_extended_api_enabled_for_testing = true;
+}
+
 }  // namespace search
 }  // namespace chrome