Create TemplateUrlData to base::Dictionary utility functions

components/search_engines/template_url_data_util.cc

Index: components/search_engines/template_url_data_util.cc
diff --git a/components/search_engines/template_url_data_util.cc b/components/search_engines/template_url_data_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7550d32cb93d52f08a11f40123293dd89acedde2
--- /dev/null
+++ b/components/search_engines/template_url_data_util.cc
@@ -0,0 +1,289 @@
+// Copyright 2016 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 "components/search_engines/template_url_data_util.h"
+
+#include <string>
+
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "components/search_engines/default_search_manager.h"
+#include "components/search_engines/prepopulated_engines.h"
+#include "components/search_engines/template_url_data.h"
+#include "url/gurl.h"
+
+std::unique_ptr<TemplateURLData> TemplateURLDataFromDictionary(
+    const base::DictionaryValue& dict) {
+

[Peter Kasting, 2016/11/20 07:57:50]

Nit: No blank line

[Alexander Yashkin, 2016/11/20 20:24:05]

Fixed

+  std::string search_url;

[Peter Kasting, 2016/11/20 07:57:50]

Is everything from here on just taken verbatim from the other files, or do I
need to actually review anything?  If it's pure copy-and-paste I won't bother
looking.

[Alexander Yashkin, 2016/11/20 20:24:05]

I have added check for short_name field to be required, following comments in
TemplateURLData:
// Private so we can enforce using the setters and thus enforce that these
// fields are never empty.
Yet I am not sure that this change does not break something.


Also I have added serialization/deserialization of contextual_search_url field
to TemplateURLDataFromDictionary/TemplateURLDataToDictionary which I believe was
forgotten.
It was deserialized from override engines pref but not from default_search pref.
I don't think such behaviour was intentional.

Other code is just copy/pasted.

[Peter Kasting, 2016/11/21 02:17:07]

Good catch!  Yes, that would have resulted in a DCHECK failure at runtime. 
Better to catch and handle it here.
Probably just never noticed since I doubt anyone has tried to use it.  Adding
that seems like a good change.

Thanks for the summary!

+  base::string16 keyword;
+  base::string16 short_name;
+  dict.GetString(DefaultSearchManager::kURL, &search_url);
+  dict.GetString(DefaultSearchManager::kKeyword, &keyword);
+  dict.GetString(DefaultSearchManager::kShortName, &short_name);
+  // Check required TemplateURLData fields first.
+  if (search_url.empty() || keyword.empty() || short_name.empty())
+    return std::unique_ptr<TemplateURLData>();
+
+  auto result = base::MakeUnique<TemplateURLData>();
+  result->SetKeyword(keyword);
+  result->SetURL(search_url);
+
+  std::string id;
+  dict.GetString(DefaultSearchManager::kID, &id);
+  base::StringToInt64(id, &result->id);
+
+  result->SetShortName(short_name);
+  dict.GetInteger(DefaultSearchManager::kPrepopulateID,
+                  &result->prepopulate_id);
+  dict.GetString(DefaultSearchManager::kSyncGUID,
+                 &result->sync_guid);
+  dict.GetString(DefaultSearchManager::kSuggestionsURL,
+                 &result->suggestions_url);
+
+  dict.GetString(DefaultSearchManager::kInstantURL,
+                 &result->instant_url);
+  dict.GetString(DefaultSearchManager::kImageURL, &result->image_url);
+  dict.GetString(DefaultSearchManager::kNewTabURL, &result->new_tab_url);
+  dict.GetString(DefaultSearchManager::kContextualSearchURL,
+                 &result->contextual_search_url);
+  std::string favicon_url;
+  std::string originating_url;
+  dict.GetString(DefaultSearchManager::kFaviconURL, &favicon_url);
+  dict.GetString(DefaultSearchManager::kOriginatingURL, &originating_url);
+  result->favicon_url = GURL(favicon_url);
+  result->originating_url = GURL(originating_url);
+
+  dict.GetString(DefaultSearchManager::kSearchURLPostParams,
+                 &result->search_url_post_params);
+  dict.GetString(DefaultSearchManager::kSuggestionsURLPostParams,
+                 &result->suggestions_url_post_params);
+  dict.GetString(DefaultSearchManager::kInstantURLPostParams,
+                 &result->instant_url_post_params);
+  dict.GetString(DefaultSearchManager::kImageURLPostParams,
+                 &result->image_url_post_params);
+  dict.GetBoolean(DefaultSearchManager::kSafeForAutoReplace,
+                  &result->safe_for_autoreplace);
+
+  std::string date_created_str;
+  std::string last_modified_str;
+  dict.GetString(DefaultSearchManager::kDateCreated, &date_created_str);
+  dict.GetString(DefaultSearchManager::kLastModified, &last_modified_str);
+
+  int64_t date_created = 0;
+  if (base::StringToInt64(date_created_str, &date_created)) {
+    result->date_created = base::Time::FromInternalValue(date_created);
+  }
+
+  int64_t last_modified = 0;
+  if (base::StringToInt64(date_created_str, &last_modified)) {
+    result->last_modified = base::Time::FromInternalValue(last_modified);
+  }
+
+  dict.GetInteger(DefaultSearchManager::kUsageCount, &result->usage_count);
+
+  const base::ListValue* alternate_urls = nullptr;
+  if (dict.GetList(DefaultSearchManager::kAlternateURLs, &alternate_urls)) {
+    for (const auto& it : *alternate_urls) {
+      std::string alternate_url;
+      if (it->GetAsString(&alternate_url))
+        result->alternate_urls.push_back(std::move(alternate_url));
+    }
+  }
+
+  const base::ListValue* encodings = nullptr;
+  if (dict.GetList(DefaultSearchManager::kInputEncodings, &encodings)) {
+    for (const auto& it : *encodings) {
+      std::string encoding;
+      if (it->GetAsString(&encoding))
+        result->input_encodings.push_back(std::move(encoding));
+    }
+  }
+
+  dict.GetString(DefaultSearchManager::kSearchTermsReplacementKey,
+                 &result->search_terms_replacement_key);
+  dict.GetBoolean(DefaultSearchManager::kCreatedByPolicy,
+                  &result->created_by_policy);
+  return result;
+}
+
+std::unique_ptr<base::DictionaryValue> TemplateURLDataToDictionary(
+    const TemplateURLData& data) {
+  auto url_dict = base::MakeUnique<base::DictionaryValue>();
+  url_dict->SetString(DefaultSearchManager::kID, base::Int64ToString(data.id));
+  url_dict->SetString(DefaultSearchManager::kShortName, data.short_name());
+  url_dict->SetString(DefaultSearchManager::kKeyword, data.keyword());
+  url_dict->SetInteger(DefaultSearchManager::kPrepopulateID,
+                       data.prepopulate_id);
+  url_dict->SetString(DefaultSearchManager::kSyncGUID, data.sync_guid);
+
+  url_dict->SetString(DefaultSearchManager::kURL, data.url());
+  url_dict->SetString(DefaultSearchManager::kSuggestionsURL,
+                      data.suggestions_url);
+  url_dict->SetString(DefaultSearchManager::kInstantURL, data.instant_url);
+  url_dict->SetString(DefaultSearchManager::kImageURL, data.image_url);
+  url_dict->SetString(DefaultSearchManager::kNewTabURL, data.new_tab_url);
+  url_dict->SetString(DefaultSearchManager::kContextualSearchURL,
+                      data.contextual_search_url);
+  url_dict->SetString(DefaultSearchManager::kFaviconURL,
+                      data.favicon_url.spec());
+  url_dict->SetString(DefaultSearchManager::kOriginatingURL,
+                      data.originating_url.spec());
+
+  url_dict->SetString(DefaultSearchManager::kSearchURLPostParams,
+                      data.search_url_post_params);
+  url_dict->SetString(DefaultSearchManager::kSuggestionsURLPostParams,
+                      data.suggestions_url_post_params);
+  url_dict->SetString(DefaultSearchManager::kInstantURLPostParams,
+                      data.instant_url_post_params);
+  url_dict->SetString(DefaultSearchManager::kImageURLPostParams,
+                      data.image_url_post_params);
+
+  url_dict->SetBoolean(DefaultSearchManager::kSafeForAutoReplace,
+                       data.safe_for_autoreplace);
+
+  url_dict->SetString(DefaultSearchManager::kDateCreated,
+                      base::Int64ToString(data.date_created.ToInternalValue()));
+  url_dict->SetString(DefaultSearchManager::kLastModified,
+                      base::Int64ToString(
+                          data.last_modified.ToInternalValue()));
+  url_dict->SetInteger(DefaultSearchManager::kUsageCount, data.usage_count);
+
+  auto alternate_urls = base::MakeUnique<base::ListValue>();
+  for (const auto& alternate_url : data.alternate_urls)
+    alternate_urls->AppendString(alternate_url);
+
+  url_dict->Set(DefaultSearchManager::kAlternateURLs,
+                std::move(alternate_urls));
+
+  auto encodings = base::MakeUnique<base::ListValue>();
+  for (const auto& input_encoding : data.input_encodings)
+    encodings->AppendString(input_encoding);
+  url_dict->Set(DefaultSearchManager::kInputEncodings, std::move(encodings));
+
+  url_dict->SetString(DefaultSearchManager::kSearchTermsReplacementKey,
+                      data.search_terms_replacement_key);
+  url_dict->SetBoolean(DefaultSearchManager::kCreatedByPolicy,
+                       data.created_by_policy);
+  return url_dict;
+}
+
+std::unique_ptr<TemplateURLData> MakePrepopulatedTemplateURLData(

[Peter Kasting, 2016/11/20 07:57:50]

I think this should just be an additional constructor on TemplateURLData.

[Alexander Yashkin, 2016/11/20 20:24:05]

What I don't like about an idea making it a constructor:
This function takes in arguments not all fields of TemplateURLData but only
those that exist in PrepopulatedEngine struct. It also initializes fields
safe_for_autoreplace, date_created, last_modified with values that are
appropriate for TemplateURLData created from PrepopulatedEngine struct but not
from other sources. Its name reflects this specifics -
MakePrepopulatedTemplateURLData.
 
Alternative would be creating universal constructor for TemplateURLData that
must get all fields in arguments - huge list of more than 20 values.
And it would be used from same places only to create TemplateURLData for
prepopulated engines - from override preference or from internal list of
PrepopulatedEngine structs.

[Peter Kasting, 2016/11/21 02:17:07]

It seems OK, and probably even preferable, that |date_created| and
|last_modified| be set to Time::Now() instead of Time() for these engines.  If
these are going to be considered "prepopulated" engines, then they'll be created
once during first run, and setting their creation and modification dates to that
time is reasonable.

If we changed these, the only fields whose values would be explicitly
initialized differently from the current null constructor are
show_in_default_list, which is gone as of
https://chromium.googlesource.com/chromium/src/+/cf06dd01d2516e1509acd1e99b20...
, and safe_for_autoreplace.  There are several ways to deal with that:

(1) Change the null constructor to also set this field to true, and then change
callers of that constructor so that the ones which wanted it true anyway do
nothing, and the ones which wanted it false set that explicitly.
(2) Set this field to false in this constructor, but set it true explicitly in
the callers here.
(3) Just have these two fields differ, and add a comment on the constructor
declaration a la:

// Creates a TemplateURLData suitable for prepopulated engines.  Note that
unlike in the default constructor, |safe_for_autoreplace| will be set to true.
TemplateURLData(...);

(4) Take this field's value as an argument to the constructor.

I'd be OK with any of these routes, personally.

Would doing any of these be satisfactory to you?

The other option, as you mentioned, is to just take all the fields in the
constructor argument list.  We're already passing almost everything anyway, so
this wouldn't be hugely worse, and I'd be OK with that too, although it doesn't
seem like we need the flexibility; but maybe some test code could be simplified
using such a constructor?

+    const base::string16& name,
+    const base::string16& keyword,
+    const base::StringPiece& search_url,

[Peter Kasting, 2016/11/21 02:17:07]

Nit: Since most of these are being used as strings below, and in some cases
passed in as strings as well, I'd change them to be string&s instead of
StringPiece&s, to avoid converting back and forth in some callers.

+    const base::StringPiece& suggest_url,
+    const base::StringPiece& instant_url,
+    const base::StringPiece& image_url,
+    const base::StringPiece& new_tab_url,
+    const base::StringPiece& contextual_search_url,
+    const base::StringPiece& search_url_post_params,
+    const base::StringPiece& suggest_url_post_params,
+    const base::StringPiece& instant_url_post_params,
+    const base::StringPiece& image_url_post_params,
+    const base::StringPiece& favicon_url,
+    const base::StringPiece& encoding,
+    const base::ListValue& alternate_urls,
+    const base::StringPiece& search_terms_replacement_key,
+    int prepopulate_id) {
+  std::unique_ptr<TemplateURLData> data(new TemplateURLData);
+
+  data->SetShortName(name);
+  data->SetKeyword(keyword);
+  data->SetURL(search_url.as_string());
+  data->suggestions_url = suggest_url.as_string();
+  data->instant_url = instant_url.as_string();
+  data->image_url = image_url.as_string();
+  data->new_tab_url = new_tab_url.as_string();
+  data->contextual_search_url = contextual_search_url.as_string();
+  data->search_url_post_params = search_url_post_params.as_string();
+  data->suggestions_url_post_params = suggest_url_post_params.as_string();
+  data->instant_url_post_params = instant_url_post_params.as_string();
+  data->image_url_post_params = image_url_post_params.as_string();
+  data->favicon_url = GURL(favicon_url);
+  data->show_in_default_list = true;
+  data->safe_for_autoreplace = true;
+  data->input_encodings.push_back(encoding.as_string());
+  data->date_created = base::Time();
+  data->last_modified = base::Time();
+  data->prepopulate_id = prepopulate_id;
+  for (size_t i = 0; i < alternate_urls.GetSize(); ++i) {
+    std::string alternate_url;
+    alternate_urls.GetString(i, &alternate_url);
+    DCHECK(!alternate_url.empty());
+    data->alternate_urls.push_back(alternate_url);
+  }
+  data->search_terms_replacement_key = search_terms_replacement_key.as_string();
+  return data;
+}
+
+std::unique_ptr<TemplateURLData> TemplateURLDataFromPrepopulatedEngine(
+    const TemplateURLPrepopulateData::PrepopulatedEngine& engine) {
+  base::ListValue alternate_urls;
+  if (engine.alternate_urls) {
+    for (size_t i = 0; i < engine.alternate_urls_size; ++i)
+      alternate_urls.AppendString(std::string(engine.alternate_urls[i]));
+  }
+
+  return MakePrepopulatedTemplateURLData(
+      base::WideToUTF16(engine.name), base::WideToUTF16(engine.keyword),
+      engine.search_url, engine.suggest_url, engine.instant_url,
+      engine.image_url, engine.new_tab_url, engine.contextual_search_url,
+      engine.search_url_post_params, engine.suggest_url_post_params,
+      engine.instant_url_post_params, engine.image_url_post_params,
+      engine.favicon_url, engine.encoding, alternate_urls,
+      engine.search_terms_replacement_key, engine.id);
+}
+
+std::unique_ptr<TemplateURLData> TemplateURLDataFromOverrideDictionary(
+    const base::DictionaryValue& engine) {
+  base::string16 name;
+  base::string16 keyword;
+  std::string search_url;
+  std::string favicon_url;
+  std::string encoding;
+  int id = -1;
+  // The following fields are required for each search engine configuration.
+  if (engine.GetString("name", &name) && !name.empty() &&
+      engine.GetString("keyword", &keyword) && !keyword.empty() &&
+      engine.GetString("search_url", &search_url) && !search_url.empty() &&
+      engine.GetString("favicon_url", &favicon_url) && !favicon_url.empty() &&
+      engine.GetString("encoding", &encoding) && !encoding.empty() &&
+      engine.GetInteger("id", &id)) {
+    // These fields are optional.
+    std::string suggest_url;
+    std::string instant_url;
+    std::string image_url;
+    std::string new_tab_url;
+    std::string contextual_search_url;
+    std::string search_url_post_params;
+    std::string suggest_url_post_params;
+    std::string instant_url_post_params;
+    std::string image_url_post_params;
+    base::ListValue empty_list;
+    const base::ListValue* alternate_urls = &empty_list;
+    std::string search_terms_replacement_key;
+    engine.GetString("suggest_url", &suggest_url);
+    engine.GetString("instant_url", &instant_url);
+    engine.GetString("image_url", &image_url);
+    engine.GetString("new_tab_url", &new_tab_url);
+    engine.GetString("contextual_search_url", &contextual_search_url);
+    engine.GetString("search_url_post_params", &search_url_post_params);
+    engine.GetString("suggest_url_post_params", &suggest_url_post_params);
+    engine.GetString("instant_url_post_params", &instant_url_post_params);
+    engine.GetString("image_url_post_params", &image_url_post_params);
+    engine.GetList("alternate_urls", &alternate_urls);
+    engine.GetString("search_terms_replacement_key",
+        &search_terms_replacement_key);
+    return MakePrepopulatedTemplateURLData(
+        name, keyword, search_url, suggest_url, instant_url, image_url,
+        new_tab_url, contextual_search_url, search_url_post_params,
+        suggest_url_post_params, instant_url_post_params,
+        image_url_post_params, favicon_url, encoding, *alternate_urls,
+        search_terms_replacement_key, id);
+  }
+  return std::unique_ptr<TemplateURLData>();
+}