Store default search provider data in dictionary pref.

chrome/browser/search_engines/default_search_manager_unittest.cc

+// Copyright 2014 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/files/scoped_temp_dir.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
+#include "chrome/browser/search_engines/default_search_manager.h"
+#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/test/base/testing_pref_service_syncable.h"
+#include "components/user_prefs/pref_registry_syncable.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Checks that the two TemplateURLs are similar. It does not check the id, the
+// date_created or the last_modified time.  Neither pointer should be NULL.
+void ExpectSimilar(const TemplateURLData* expected,
+                   const TemplateURLData* actual) {
+  EXPECT_TRUE(expected != NULL);
+  EXPECT_TRUE(actual != NULL);
+
+  if (!actual || !expected)
+    return;

[gab, 2014/04/23 20:00:18]

optional: You can just use ASSERT_TRUE for the NULL checks above and not need
this extra logic here.

[Peter Kasting, 2014/04/23 20:41:06]

I would echo this except without the "optional" prefix :)

[erikwright (departed), 2014/04/23 20:48:53]

I asked her not to. Many developers are not aware that ASSERT does a return on
failure. As such, there's a risk of this later being replaced with an assert.

I believe ASSERT should only be used directly in test cases.

[Peter Kasting, 2014/04/23 20:56:25]

I'm confused by what this last sentence is saying.  Could you restate?

ASSERT_TRUE seems clearly better to me here.
That is very much not what tons of existing test code does.  We use ASSERT_
liberally in helper functions in test files.  And I approve of that usage.

[erikwright (departed), 2014/04/23 21:11:49]

It's not obvious that this is a return statement. So a developer could easily
look at this and say "Why are these ASSERTs and not EXPECTs? I'll fix that for
you."
 
The other issue with it is that many developers assume that ASSERT terminates a
test, without understanding how.

So they put ASSERT in a helper function in order to terminate when the test
should no longer continue. And then the test crashes because the caller keeps
going with invalid state.

Clearly I'm speaking very anecdotally here, both of these have definitely come
up in codereviews for me.

[Peter Kasting, 2014/04/23 21:15:47]

This means the developer fundamentally does not know there is any difference
between EXPECT and ASSERT.  I don't think we should code defensively against
such developers.  There may be a few (though I have never seen this confusion
happen), but that doesn't justify this fundamentally worse code.
This is a more reasonable concern, but I still don't think it is sufficient
justification to write the code here, especially when the test as written is not
subject to this problem, nor likely to become subject to it.

I'm going to remain with my original opinion: this needs to be changed to use
ASSERT before this code is landed.  If you still find this objectionable, I
would solve by inlining the code from ExpectSimilar() into its lone caller.

[erikwright (departed), 2014/04/23 21:21:05]

You're right that I'm conflating this with the second issue. The problem is
thinking that it terminates the test, not with thinking that the method will
continue.
My personal opinion remains that ASSERT should only be used directly in test
cases, but I definitely don't feel strong enough about it to ask for changes
here. As you say, it's common enough that one CL doesn't make a difference.

[Cait (Slow), 2014/04/23 23:26:55]

Erik had recommended against using ASSERTs in helper methods, for clarity:
"Don't use ASSERT in a helper function. Many developers do not immediately
understand that it returns on failure, so the fact that it _must_ be an ASSERT
(not an EXPECT) here is not self-evident.

Use two EXPECTs and an if (...) return; instead."

+
+  EXPECT_EQ(expected->short_name, actual->short_name);
+  EXPECT_EQ(expected->keyword(), actual->keyword());
+  EXPECT_EQ(expected->url(), actual->url());
+  EXPECT_EQ(expected->suggestions_url, actual->suggestions_url);
+  EXPECT_EQ(expected->favicon_url, actual->favicon_url);
+  EXPECT_EQ(expected->alternate_urls, actual->alternate_urls);
+  EXPECT_EQ(expected->show_in_default_list, actual->show_in_default_list);
+  EXPECT_EQ(expected->safe_for_autoreplace, actual->safe_for_autoreplace);
+  EXPECT_EQ(expected->input_encodings, actual->input_encodings);
+  EXPECT_EQ(expected->search_terms_replacement_key,
+            actual->search_terms_replacement_key);
+}
+
+}  // namespace
+
+class DefaultSearchManagerTest : public testing::Test {
+ public:
+  DefaultSearchManagerTest() {};
+
+  virtual void SetUp() OVERRIDE {
+    pref_service_.reset(new TestingPrefServiceSyncable);
+    DefaultSearchManager::RegisterProfilePrefs(pref_service_->registry());
+  }
+
+  virtual void TearDown() { pref_service_.reset(); }

[erikwright (departed), 2014/04/23 20:48:53]

Not necessary. There is one test instance per test case execution.

[Cait (Slow), 2014/04/23 23:26:55]

Done.

+
+  PrefService* Prefs() const { return pref_service_.get(); }

[Peter Kasting, 2014/04/23 20:41:06]

This must be non-const or else return a const pointer.

Nit: Name this getter "pref_service()".

[Cait (Slow), 2014/04/23 23:26:55]

Done.

+
+ private:
+  scoped_ptr<TestingPrefServiceSyncable> pref_service_;

[gab, 2014/04/23 20:00:18]

I think it's fine to leave this member in the protected section of your test
fixture and remove the Prefs() accessor method.

[Peter Kasting, 2014/04/23 20:41:06]

That would violate the Google style guide, which bans non-private class data
members and doesn't make an exception for tests.

+  DISALLOW_COPY_AND_ASSIGN(DefaultSearchManagerTest);
+};
+
+TEST_F(DefaultSearchManagerTest, PrefSaving) {
+  DefaultSearchManager manager(Prefs());
+  TemplateURLData data;
+  data.short_name = base::UTF8ToUTF16("name1");
+  data.SetKeyword(base::UTF8ToUTF16("key1"));
+  data.SetURL("http://foo1/{searchTerms}");
+  data.suggestions_url = "http://sugg1";
+  data.alternate_urls.push_back("http://foo1/alt");
+  data.favicon_url = GURL("http://icon1");
+  data.safe_for_autoreplace = true;
+  data.show_in_default_list = true;
+  base::SplitString("UTF-8;UTF-16", ';', &data.input_encodings);
+  data.date_created = base::Time();
+  data.last_modified = base::Time();
+
+  manager.SaveToPrefService(data);
+  TemplateURLData read_data;
+  manager.ReadFromPrefService(&read_data);
+  ExpectSimilar(&data, &read_data);
+}

[gab, 2014/04/23 20:00:18]

Also test reading with no prior writes (i.e. reading default values).

[Cait (Slow), 2014/04/23 23:26:55]

Done. For now this just amounts to an empty dictionary. Default value handling
is coming in a future CL.