Refactor search_engines tests to better reuse code

Refactor search_engines tests to better reuse code

I refactored search engines test code to better reuse recently created
util functions.

R=pkasting@chromium.org

Committed: https://crrev.com/7b30d405b26f91439db4826e53f99e5698b3a7ab
Cr-Commit-Position: refs/heads/master@{#435171}

[Peter Kasting, 2016-11-30 00:50:37]

LGTM!

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1672:
managed.input_encodings.push_back("UTF-32");
Nit: Can this just be:

  managed.input_encodings = {"UTF-16", "UTF-32"};

?  That seems a bit easier to read.

(For similarity, we could probably use the same kind of syntax with the
|alternate_urls| field below.)

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_test_util.h (right):

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_test_util.h:25: //
notification.
Nit: I wasn't clear which notification this was, then I saw the code didn't
explicitly fire a notification; that happens automatically due to lower layers.

Accordingly, I would just leave that detail out of this comment.  Layering-wise,
if we change that implementation detail lower down, we wouldn't want to fix the
comment here, and it doesn't seem like this function cares about enforcing that
that happens.

(Same comment below)

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_test_util.h:26: void
SetManagedDefaultSearchPreferences(const TemplateURLData& managed_tdata,
Nit: "tdata" is an unusual spelling for this (it kinda mirrors "turl", which is
common, although in retrospect I'm not sure I ever should have started using
that).  Maybe "data" or "managed_data" or "turl_data"?

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_test_util.h:27: bool enabled,
Nit: Do you need to comment on what, precisely, this enables?

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:142:
result->input_encodings.push_back("UTF-16");
Nit: See earlier comments

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:872: new
TemplateURL(*managed));
Nit: Try to avoid raw new (use MakeUnique here); feel free to use auto to reduce
redundancy.  (Multiple places)

[Alexander Yashkin, 2016-11-30 06:01:20]

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1672:
managed.input_encodings.push_back("UTF-32");
On 2016/11/30 at 00:50:37, Peter Kasting wrote:
> Nit: Can this just be:
> 
>   managed.input_encodings = {"UTF-16", "UTF-32"};
> 
> ?  That seems a bit easier to read.
> 
> (For similarity, we could probably use the same kind of syntax with the
|alternate_urls| field below.)

Done, cool new syntax, short and clear.

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_test_util.h (right):

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_test_util.h:25: //
notification.
On 2016/11/30 at 00:50:37, Peter Kasting wrote:
> Nit: I wasn't clear which notification this was, then I saw the code didn't
explicitly fire a notification; that happens automatically due to lower layers.
> 
> Accordingly, I would just leave that detail out of this comment. 
Layering-wise, if we change that implementation detail lower down, we wouldn't
want to fix the comment here, and it doesn't seem like this function cares about
enforcing that that happens.
> 
> (Same comment below)

Done

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_test_util.h:26: void
SetManagedDefaultSearchPreferences(const TemplateURLData& managed_tdata,
On 2016/11/30 at 00:50:37, Peter Kasting wrote:
> Nit: "tdata" is an unusual spelling for this (it kinda mirrors "turl", which
is common, although in retrospect I'm not sure I ever should have started using
that).  Maybe "data" or "managed_data" or "turl_data"?

Done

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_test_util.h:27: bool enabled,
On 2016/11/30 at 00:50:37, Peter Kasting wrote:
> Nit: Do you need to comment on what, precisely, this enables?

Added comment:
// enabled arg enables/disables use of managed engine by DefaultSearchManager.

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:142:
result->input_encodings.push_back("UTF-16");
On 2016/11/30 at 00:50:37, Peter Kasting wrote:
> Nit: See earlier comments

Done

https://codereview.chromium.org/2541493002/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:872: new
TemplateURL(*managed));
On 2016/11/30 at 00:50:37, Peter Kasting wrote:
> Nit: Try to avoid raw new (use MakeUnique here); feel free to use auto to
reduce redundancy.  (Multiple places)

Done

[Alexander Yashkin, 2016-11-30 06:01:29]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-11-30 06:01:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2541493002/#ps20001
(title: "Fixed after review")

[commit-bot: I haz the power, 2016-11-30 06:02:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 08:34:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480485689310820,
"parent_rev": "73bb7091486a1b2ca27d97008da0a88326c67bb0", "commit_rev":
"1760e41702eb74ef131f302abd9d6de012cbe04c"}

[commit-bot: I haz the power, 2016-11-30 08:35:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-30 08:37:12]

Description was changed from

==========
Refactor search_engines tests to better reuse code

I refactored search engines test code to better reuse recently created
util functions.

R=pkasting@chromium.org
==========

to

==========
Refactor search_engines tests to better reuse code

I refactored search engines test code to better reuse recently created
util functions.

R=pkasting@chromium.org

Committed: https://crrev.com/7b30d405b26f91439db4826e53f99e5698b3a7ab
Cr-Commit-Position: refs/heads/master@{#435171}
==========

[commit-bot: I haz the power, 2016-11-30 08:37:13]

Patchset 2 (id:??) landed as
https://crrev.com/7b30d405b26f91439db4826e53f99e5698b3a7ab
Cr-Commit-Position: refs/heads/master@{#435171}