Store default search provider data in dictionary pref.

chrome/browser/search_engines/default_search_manager.cc

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

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

fix date, remove (c)

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/search_engines/default_search_manager.h"
+
+#include <algorithm>
+#include <utility>
+
+#include "base/compiler_specific.h"
+#include "base/i18n/case_conversion.h"
+#include "base/prefs/pref_service.h"
+#include "base/stl_util.h"
+#include "base/strings/string_number_conversions.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/profiles/profile.h"
+#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_keys.h"
+#include "chrome/browser/search_engines/util.h"
+#include "chrome/common/pref_names.h"
+#include "components/user_prefs/pref_registry_syncable.h"
+
+namespace {
+// A dictionary to hold all data related to the Default Search Engine.

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

blank line before

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

Done.

+// Eventually, this should replace all the data stored in the
+// default_search_provider.* prefs.
+const char kDefaultSearchProviderData[] =
+    "default_search_provider.template_url_data";
+}  // namespace
+
+DefaultSearchManager::DefaultSearchManager(PrefService* pref_service)
+    : pref_service_(pref_service) {
+}
+
+DefaultSearchManager::~DefaultSearchManager() {
+}
+
+// static
+void DefaultSearchManager::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(
+      kDefaultSearchProviderData,
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+}
+
+bool DefaultSearchManager::ReadFromPrefService(TemplateURLData* data) {
+  if (!pref_service_) {
+    return false;

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

Should this even be allowed by the constructor?

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

I agree; the constructor should DCHECK this and the rest of the code should just
rely on it being set.

Combined with your comment below, this means this function can return a
TemplateURLData object directly instead of via outparam, since the bool will no
longer be needed.

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

Agreed. DCHECK in ctor.

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

Done.

+  }

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

rm {}

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

Done.

+  const base::DictionaryValue* url_dict =
+      pref_service_->GetDictionary(kDefaultSearchProviderData);
+  if (!url_dict) {
+    return false;

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

This is not possible; PrefService always returns a dictionary (the default one
at registration -- an empty dict in this case -- if no other is available).

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

Right. I guess you will have to treat an empty dictionary as undefined.

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

Done.

+  }
+
+  std::string search_url;
+  base::string16 keyword;

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

Nit: Reverse declaration order to match usage order below

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

Done.

+

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

Nit: Eliminate this blank line and the next two

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

Done.

+  url_dict->GetString(default_search::kKeyword, &keyword);
+  url_dict->GetString(default_search::kURL, &search_url);
+
+  DCHECK(!keyword.empty());
+  DCHECK(!search_url.empty());

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

Couldn't these DCHECKs fail if the user has mucked with their preferences file?

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

Settings hardening should catch this, and reset the entire DSE pref if any part
of it gets mucked with, but w/o settings hardening the scenario you described
could occur and result in weird behavior. I'll add code to handle it (but keep
the DCHECKs, as this scenario should be an outlier).

+
+  data->SetKeyword(keyword);
+  data->SetURL(search_url);
+
+  std::string id;
+  url_dict->GetString(default_search::kID, &id);
+  base::StringToInt64(id, &data->id);

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

see PrefService::GetInt64

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

She's working with a dict, not a PrefService, here.

[gab, 2014/04/23 20:56:21]

Duh, of course, this is annoying. DictionaryValue should be augmented to support
this IMO, but that's another story... (this is not the first piece of code doing
this custom logic for int64s).

[Peter Kasting, 2014/04/23 20:57:04]

Then how about someone writes that change now, and checks it in, and we use it
here?

[gab, 2014/04/23 21:01:17]

Sounds good. I'm on it.

[gab, 2014/04/23 23:20:34]

CL up @ https://codereview.chromium.org/254473002/

[gab, 2014/04/24 14:50:03]

My CL isn't as simple as I originally thought and nor does it seem to have
consensus among reviewers.

Please do not block your CL on it. We can fix this later if I can get to land my
new proposed API.

+  url_dict->GetString(default_search::kShortName, &data->short_name);
+  url_dict->GetInteger(default_search::kPrepopulateID, &data->prepopulate_id);
+  url_dict->GetString(default_search::kSyncGUID, &data->sync_guid);
+
+  url_dict->GetString(default_search::kSuggestionsURL, &data->suggestions_url);
+  url_dict->GetString(default_search::kInstantURL, &data->instant_url);
+  url_dict->GetString(default_search::kImageURL, &data->image_url);
+  url_dict->GetString(default_search::kNewTabURL, &data->new_tab_url);
+
+  std::string favicon_url;
+  std::string originating_url;
+  url_dict->GetString(default_search::kFaviconURL, &favicon_url);
+  url_dict->GetString(default_search::kOriginatingURL, &originating_url);
+  data->favicon_url = GURL(favicon_url);
+  data->originating_url = GURL(originating_url);
+
+  url_dict->GetString(default_search::kSearchURLPostParams,
+                      &data->search_url_post_params);
+  url_dict->GetString(default_search::kSuggestionsURLPostParams,
+                      &data->suggestions_url_post_params);
+  url_dict->GetString(default_search::kInstantURLPostParams,
+                      &data->instant_url_post_params);
+  url_dict->GetString(default_search::kImageURLPostParams,
+                      &data->image_url_post_params);
+
+  url_dict->GetBoolean(default_search::kSafeForAutoReplace,
+                       &data->safe_for_autoreplace);
+
+  double date_created = 0.0;
+  double last_modified = 0.0;
+  url_dict->GetDouble(default_search::kDateCreated, &date_created);
+  url_dict->GetDouble(default_search::kLastModified, &last_modified);
+  data->date_created = base::Time::FromInternalValue(date_created);
+  data->last_modified = base::Time::FromInternalValue(last_modified);
+
+  url_dict->GetInteger(default_search::kUsageCount, &data->usage_count);
+
+  const base::ListValue* alternate_urls = new base::ListValue;
+  url_dict->GetList(default_search::kAlternateURLs, &alternate_urls);
+  data->alternate_urls.clear();
+  for (size_t i = 0; i < alternate_urls->GetSize(); ++i) {

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

Use ListValue::const_iterator here.

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

Done.

+    std::string alternate_url;
+    if (alternate_urls->GetString(i, &alternate_url))
+      data->alternate_urls.push_back(alternate_url);
+  }
+
+  const base::ListValue* encodings = new base::ListValue;
+  url_dict->GetList(default_search::kInputEncodings, &encodings);
+  data->input_encodings.clear();
+  for (size_t i = 0; i < encodings->GetSize(); ++i) {

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

ListValue::const_iterator here as well.

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

Done.

+    std::string encoding;
+    if (encodings->GetString(i, &encoding))
+      data->input_encodings.push_back(encoding);
+  }
+
+  url_dict->GetString(default_search::kSearchTermsReplacementKey,
+                      &data->search_terms_replacement_key);
+
+  url_dict->GetBoolean(default_search::kCreatedByPolicy,
+                       &data->created_by_policy);
+
+  data->show_in_default_list = true;
+
+  return true;
+}
+
+void DefaultSearchManager::SaveToPrefService(const TemplateURLData& data) {
+  if (!pref_service_)
+    return;
+
+  base::DictionaryValue* url_dict = new base::DictionaryValue;
+  url_dict->SetString(default_search::kID, base::Int64ToString(data.id));

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

USe PrefService::SetInt64()

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

not a PrefService..

+  url_dict->SetString(default_search::kShortName, data.short_name);
+  url_dict->SetString(default_search::kKeyword, data.keyword());
+  url_dict->SetInteger(default_search::kPrepopulateID, data.prepopulate_id);
+  url_dict->SetString(default_search::kSyncGUID, data.sync_guid);
+
+  url_dict->SetString(default_search::kURL, data.url());
+  url_dict->SetString(default_search::kSuggestionsURL, data.suggestions_url);
+  url_dict->SetString(default_search::kInstantURL, data.instant_url);
+  url_dict->SetString(default_search::kImageURL, data.image_url);
+  url_dict->SetString(default_search::kNewTabURL, data.new_tab_url);
+  url_dict->SetString(default_search::kFaviconURL, data.favicon_url.spec());
+  url_dict->SetString(default_search::kOriginatingURL,
+                      data.originating_url.spec());
+
+  url_dict->SetString(default_search::kSearchURLPostParams,
+                      data.search_url_post_params);
+  url_dict->SetString(default_search::kSuggestionsURLPostParams,
+                      data.suggestions_url_post_params);
+  url_dict->SetString(default_search::kInstantURLPostParams,
+                      data.instant_url_post_params);
+  url_dict->SetString(default_search::kImageURLPostParams,
+                      data.image_url_post_params);
+
+  url_dict->SetBoolean(default_search::kSafeForAutoReplace,
+                       data.safe_for_autoreplace);
+
+  url_dict->SetDouble(default_search::kDateCreated,
+                      data.date_created.ToInternalValue());
+  url_dict->SetDouble(default_search::kLastModified,
+                      data.last_modified.ToInternalValue());
+  url_dict->SetInteger(default_search::kUsageCount, data.usage_count);
+
+  base::ListValue* alternate_urls = new base::ListValue;
+  for (size_t i = 0; i < data.alternate_urls.size(); ++i)

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

std::vector<std::string>::const_iterator

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

Done.

+    alternate_urls->AppendString(data.alternate_urls[i]);
+  url_dict->Set(default_search::kAlternateURLs, alternate_urls);
+
+  base::ListValue* encodings = new base::ListValue;
+  for (size_t i = 0; i < data.input_encodings.size(); ++i)

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

std::vector<std::string>::const_iterator

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

Done.

+    encodings->AppendString(data.input_encodings[i]);
+  url_dict->Set(default_search::kInputEncodings, encodings);
+
+  url_dict->SetString(default_search::kSearchTermsReplacementKey,
+                      data.search_terms_replacement_key);
+
+  url_dict->SetBoolean(default_search::kCreatedByPolicy,
+                       data.created_by_policy);
+
+  pref_service_->Set(kDefaultSearchProviderData, *url_dict);
+}