Remove use of stl_util in search_engines.

Remove use of stl_util in search_engines.

BUG=555865
Committed: https://crrev.com/8a64b715b5358377162509aa064242a266f37bdb
Cr-Commit-Position: refs/heads/master@{#416291}

[Avi (use Gerrit), 2016-08-30 03:24:43]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 03:45:53]

Dry run: 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-08-30 04:13:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 04:13:19]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Avi (use Gerrit), 2016-08-30 15:09:37]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 15:09:54]

Dry run: 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-08-30 15:20:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 15:20:39]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Avi (use Gerrit), 2016-08-30 16:08:12]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

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

Dry run: 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-08-30 16:14:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 16:14:57]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Avi (use Gerrit), 2016-08-30 16:18:57]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 16:19:13]

Dry run: 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-08-30 17:10:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 17:10:40]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-08-30 17:17:17]

avi@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-08-31 04:12:58]

Thanks very much for doing this work!  This is a definite improvement to the
TemplateURLService interface.

I have one big suggestion in template_url_service.h that would affect the rest
of the CL (and make a number of my other comments. e.g. about using WrapUnique
in more places, moot), so I would read and, if you agree, implement that before
anything else.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/android/...
File
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/android/...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:73:
// Takes ownership of |template_url|.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:319: // Takes
ownership.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/importer...
File chrome/browser/importer/profile_writer.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/importer...
chrome/browser/importer/profile_writer.cc:337: model->Add(base::WrapUnique(*i));
 // Takes ownership.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:686:
// The model takes ownership of |template_url|.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/i...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/i...
chrome/browser/search/instant_service_unittest.cc:136: // Takes ownership of
|template_url|.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/i...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/i...
chrome/browser/search/instant_unittest_base.cc:66:
template_url_service_->Add(std::move(template_url_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/s...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:78: // Takes ownership of
|template_url|.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:659: // |template_url_service| takes
ownership of |template_url_|.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:524:
std::unique_ptr<TemplateURL> local_turl_ptr = CreateTestTemplateURL(
Nit: It's not a big deal, but in places like this, I'd have expected the "_ptr"
to go on the other variable:

  std::unique_ptr<TemplateURL> local_turl = ...;
  TemplateURL* local_turl_ptr = local_turl.get();

An argument could be made for each direction, I suppose.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:749:
model()->Add(std::move(turl2_ptr));
Nit: Can just call Add() on the result of CreateTestTemplateURL directly and
omit the move() (as you have done below).

The argument against this is that without naming this "turl2", the meaning of
the subsequent "turl2_older" variable is unclear, but I think it's still
sufficiently clear.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_unittest.cc:1089:
std::unique_ptr<TemplateURL> cloned_url(
Nit: () -> =

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_unittest.cc:1149:
std::unique_ptr<TemplateURL> cloned_url(
Nit: () -> =

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:282:
model->Add(std::move(template_url_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:62: // Takes
ownership of |template_url|.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router_unittest.cc:114: // Takes ownership
of |template_url|.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/edit_search_engine_controller.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/edit_search_engine_controller.cc:108: //
TemplateURLService takes ownership of template_url_.
Nit: Comment is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc:250:
util()->model()->Add(std::move(turl_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/template_url_table_model.cc:272:
template_url_service_->Add(std::move(turl_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:309:
turl_model->Add(std::move(default_turl_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:376:
turl_model->Add(std::move(default_turl_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:407:
turl_model->Add(std::move(keyword_turl_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:417:
turl_model->Add(std::move(keyword_turl_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:426:
turl_model->Add(std::move(keyword_turl_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/keyword_provider_unittest.cc:435:
client_->GetTemplateURLService()->Add(std::move(template_url_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/scored_history_match_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/scored_history_match_unittest.cc:318:
template_url_service->Add(std::move(template_url_ptr));
Nit: Just use the old code and add a WrapUnique() call.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/shortcuts_backend_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/shortcuts_backend_unittest.cc:110:
template_url_service_->Add(std::move(template_url_ptr));
Nit: Just use the old code and add a WrapUnique() call.  (Leave the redundant
comment off though.)

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:209: for (auto&
handler : handlers_) {
Nit: Seems like this could be const auto&.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:319: for (auto&
handler : handlers_) {
Nit: Seems like this could be const auto&.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/search_host_to_urls_map.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/search_host_to_urls_map.cc:81: for (auto& turl :
template_urls)
Nit: Seems like this could be const auto&.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/search_host_to_urls_map_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/search_host_to_urls_map_unittest.cc:91:
std::map<size_t, bool> found_urls;
Wow, this function is a mess.

Seems like everything from this point on could just be:

  for (const auto& url : template_urls_)
    EXPECT_NE(urls->end(), urls->find(url.get()));

...and it'd perform better to boot.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:416: return (!loaded_ &&
initial_default_search_provider_.get() &&
Nit: Can this .get() be removed?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:427: return (!loaded_ &&
initial_default_search_provider_.get() &&
Nit: Can this .get() be removed?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:442: : nullptr;
Nit: Simpler:

  return (initial_dsp &&
          (initial_dsp->GenerateSearchURL(search_terms_data()).host() == host))
             ? initial_dsp
             : nullptr;

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:471:
DCHECK(template_url.get());
Nit: I'm surprised this is necessary.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:482:
template_url->extension_info_->wants_to_be_default_engine;
Nit: Might make sense to modify AddNoNotify() to return a raw pointer instead of
a bool as well.  Then this could be:

  TemplateURL* template_url_ptr = AddNoNotify(std::move(template_url), true);
  if (template_url_ptr) {
    if (template_url->extension_info_->wants_to_be_default_engine)
      ...

This would also allow implementing Add() as:

  TemplateURL* template_url_ptr = AddNoNotify(std::move(template_url), true);
  if (template_url_ptr)
    NotifyObservers();
  return template_url_ptr;

...which is a little simpler than what would need to be done to make it return a
raw pointer without touching AddNoNotify().

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:562:
std::unique_ptr<TemplateURL> turl = base::MakeUnique<TemplateURL>(data);
Nit: Can just inline below.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:901: for (auto& turl :
template_urls_) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1462: for (auto& turl :
template_urls_) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1563: bool HasValidID(const
std::unique_ptr<TemplateURL>& t_url) {
Nit: Perhaps this should just be inlined below as a lambda.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1648: for (auto& turl :
template_urls_) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1832: for (auto& turl :
template_urls_) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1899: : nullptr);
Nit: Maybe

    initial_default_search_provider_ =
        data ? base::MakeUnique<TemplateURL>(*data) : nullptr;

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2020:
(Contains(&template_urls_, existing_keyword_turl))) {
Nit: Remove extra parens

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2126: TemplateURL*
template_url = (*i).get();
Nit: i->get()

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2424: for (auto& turl :
template_urls_) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2435: for (auto& turl :
template_urls_) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2446: for (auto& turl :
template_urls_) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2456: for (auto& turl :
template_urls_) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/template_url_service.h (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.h:172: bool
Add(std::unique_ptr<TemplateURL> template_url);
I have an idea for a signature change here that will simplify most of the other
changes in this CL.

What if we change this declaration to the following:

  // Adds |template_url| to this model.  Returns a raw pointer to |template_url|
  // if the addition succeeded, or null on failure.  (Many callers need still
  // need a raw pointer to the TemplateURL so they can access it later.)
  TemplateURL* Add(std::unique_ptr<TemplateURL> template_url);

Now code that does this:

    TemplateURL* template_url = new TemplateURL(data);
    template_url_service->Add(base::WrapUnique(template_url));
    ...use |template_url|...

...can become:

    TemplateURL* template_url =
        template_url_service->Add(base::MakeUnique<TemplateURL>(data));

And code that does this:

    std::unique_ptr<TemplateURL> turl_ptr = CreateTURL();
    TemplateURL* turl = turl.get();
    model->Add(std::move(turl_ptr));
    ...use |template_url|...

...can become:

    TemplateURL* turl = model->Add(CreateTURL());

One could argue this is not only shorter and more readable but also safer, since
most of the existing callers don't check the return value of Add() before using
the raw pointer, which would point to deleted memory if the Add() failed; with
the new pattern, the returned pointer value is null, so things would fail in a
more obvious way.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.h:567: // reference, and delete
it when the TemplateURL is removed.
Nit: Second sentence here is now redundant.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/template_url_service_util_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service_util_unittest.cc:89: for (auto
itr = local_turls.begin(); itr != local_turls.end(); ++itr) {
Nit: Might as well use range-based for.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/util.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:204: for (auto removed_engine :
actions.removed_engines) {
Nit: const auto* (requires FindTemplateURL() signature change below)

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:219: for (auto edited_engine :
actions.edited_engines) {
Nit: const auto&?

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:220: TemplateURLData& data =
edited_engine.second;
Nit: const TemplateURLData&

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:230: for (const auto& added_engine :
actions.added_engines) {
Nit: {} unnecessary

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:385: TemplateURL* url) {
Nit: const TemplateURL*

[Avi (use Gerrit), 2016-09-01 00:32:56]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 00:33:33]

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

[Avi (use Gerrit), 2016-09-01 00:34:28]

An excellent API suggestion; redid the CL based on it.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/android/...
File
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/android/...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:73:
// Takes ownership of |template_url|.
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_unittest.cc:319: // Takes
ownership.
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/importer...
File chrome/browser/importer/profile_writer.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/importer...
chrome/browser/importer/profile_writer.cc:337: model->Add(base::WrapUnique(*i));
 // Takes ownership.
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:686:
// The model takes ownership of |template_url|.
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/i...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/i...
chrome/browser/search/instant_service_unittest.cc:136: // Takes ownership of
|template_url|.
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/i...
File chrome/browser/search/instant_unittest_base.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/i...
chrome/browser/search/instant_unittest_base.cc:66:
template_url_service_->Add(std::move(template_url_ptr));
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Fixed.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/s...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:78: // Takes ownership of
|template_url|.
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search/s...
chrome/browser/search/search_unittest.cc:659: // |template_url_service| takes
ownership of |template_url_|.
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:524:
std::unique_ptr<TemplateURL> local_turl_ptr = CreateTestTemplateURL(
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: It's not a big deal, but in places like this, I'd have expected the
"_ptr"
> to go on the other variable:
> 
>   std::unique_ptr<TemplateURL> local_turl = ...;
>   TemplateURL* local_turl_ptr = local_turl.get();
> 
> An argument could be made for each direction, I suppose.

Fixed.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:749:
model()->Add(std::move(turl2_ptr));
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: Can just call Add() on the result of CreateTestTemplateURL directly and
> omit the move() (as you have done below).
> 
> The argument against this is that without naming this "turl2", the meaning of
> the subsequent "turl2_older" variable is unclear, but I think it's still
> sufficiently clear.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_unittest.cc:1089:
std::unique_ptr<TemplateURL> cloned_url(
On 2016/08/31 04:12:55, Peter Kasting wrote:
> Nit: () -> =

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_unittest.cc:1149:
std::unique_ptr<TemplateURL> cloned_url(
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: () -> =

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:282:
model->Add(std::move(template_url_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

fixed

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:62: // Takes
ownership of |template_url|.
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router_unittest.cc:114: // Takes ownership
of |template_url|.
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/edit_search_engine_controller.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/edit_search_engine_controller.cc:108: //
TemplateURLService takes ownership of template_url_.
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Comment is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc:250:
util()->model()->Add(std::move(turl_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Done.

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/chrome/browser/ui/searc...
chrome/browser/ui/search_engines/template_url_table_model.cc:272:
template_url_service_->Add(std::move(turl_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

fixed

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:309:
turl_model->Add(std::move(default_turl_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:376:
turl_model->Add(std::move(default_turl_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:407:
turl_model->Add(std::move(keyword_turl_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:417:
turl_model->Add(std::move(keyword_turl_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_provider_unittest.cc:426:
turl_model->Add(std::move(keyword_turl_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/keyword_provider_unittest.cc:435:
client_->GetTemplateURLService()->Add(std::move(template_url_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/scored_history_match_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/scored_history_match_unittest.cc:318:
template_url_service->Add(std::move(template_url_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/shortcuts_backend_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/omnibox/brow...
components/omnibox/browser/shortcuts_backend_unittest.cc:110:
template_url_service_->Add(std::move(template_url_ptr));
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Just use the old code and add a WrapUnique() call.  (Leave the redundant
> comment off though.)

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:209: for (auto&
handler : handlers_) {
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Seems like this could be const auto&.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:319: for (auto&
handler : handlers_) {
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Seems like this could be const auto&.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/search_host_to_urls_map_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/search_host_to_urls_map_unittest.cc:91:
std::map<size_t, bool> found_urls;
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Wow, this function is a mess.
> 
> Seems like everything from this point on could just be:
> 
>   for (const auto& url : template_urls_)
>     EXPECT_NE(urls->end(), urls->find(url.get()));
> 
> ...and it'd perform better to boot.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:416: return (!loaded_ &&
initial_default_search_provider_.get() &&
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Can this .get() be removed?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:427: return (!loaded_ &&
initial_default_search_provider_.get() &&
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: Can this .get() be removed?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:442: : nullptr;
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return (initial_dsp &&
>           (initial_dsp->GenerateSearchURL(search_terms_data()).host() ==
host))
>              ? initial_dsp
>              : nullptr;

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:471:
DCHECK(template_url.get());
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: I'm surprised this is necessary.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:482:
template_url->extension_info_->wants_to_be_default_engine;
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: Might make sense to modify AddNoNotify() to return a raw pointer instead
of
> a bool as well.  Then this could be:
> 
>   TemplateURL* template_url_ptr = AddNoNotify(std::move(template_url), true);
>   if (template_url_ptr) {
>     if (template_url->extension_info_->wants_to_be_default_engine)
>       ...
> 
> This would also allow implementing Add() as:
> 
>   TemplateURL* template_url_ptr = AddNoNotify(std::move(template_url), true);
>   if (template_url_ptr)
>     NotifyObservers();
>   return template_url_ptr;
> 
> ...which is a little simpler than what would need to be done to make it return
a
> raw pointer without touching AddNoNotify().

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:562:
std::unique_ptr<TemplateURL> turl = base::MakeUnique<TemplateURL>(data);
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: Can just inline below.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:901: for (auto& turl :
template_urls_) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1462: for (auto& turl :
template_urls_) {
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1563: bool HasValidID(const
std::unique_ptr<TemplateURL>& t_url) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: Perhaps this should just be inlined below as a lambda.

C++11!

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1648: for (auto& turl :
template_urls_) {
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1832: for (auto& turl :
template_urls_) {
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:1899: : nullptr);
On 2016/08/31 04:12:56, Peter Kasting wrote:
> Nit: Maybe
> 
>     initial_default_search_provider_ =
>         data ? base::MakeUnique<TemplateURL>(*data) : nullptr;

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2020:
(Contains(&template_urls_, existing_keyword_turl))) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: Remove extra parens

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2126: TemplateURL*
template_url = (*i).get();
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: i->get()

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2424: for (auto& turl :
template_urls_) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2435: for (auto& turl :
template_urls_) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2446: for (auto& turl :
template_urls_) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:2456: for (auto& turl :
template_urls_) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/template_url_service.h (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.h:172: bool
Add(std::unique_ptr<TemplateURL> template_url);
On 2016/08/31 04:12:57, Peter Kasting wrote:
> I have an idea for a signature change here that will simplify most of the
other
> changes in this CL.
> 
> What if we change this declaration to the following:
> 
>   // Adds |template_url| to this model.  Returns a raw pointer to
|template_url|
>   // if the addition succeeded, or null on failure.  (Many callers need still
>   // need a raw pointer to the TemplateURL so they can access it later.)
>   TemplateURL* Add(std::unique_ptr<TemplateURL> template_url);
> 
> Now code that does this:
> 
>     TemplateURL* template_url = new TemplateURL(data);
>     template_url_service->Add(base::WrapUnique(template_url));
>     ...use |template_url|...
> 
> ...can become:
> 
>     TemplateURL* template_url =
>         template_url_service->Add(base::MakeUnique<TemplateURL>(data));
> 
> And code that does this:
> 
>     std::unique_ptr<TemplateURL> turl_ptr = CreateTURL();
>     TemplateURL* turl = turl.get();
>     model->Add(std::move(turl_ptr));
>     ...use |template_url|...
> 
> ...can become:
> 
>     TemplateURL* turl = model->Add(CreateTURL());
> 
> One could argue this is not only shorter and more readable but also safer,
since
> most of the existing callers don't check the return value of Add() before
using
> the raw pointer, which would point to deleted memory if the Add() failed; with
> the new pattern, the returned pointer value is null, so things would fail in a
> more obvious way.

Pretty clever. Let's give it a try.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service.h:567: // reference, and delete
it when the TemplateURL is removed.
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: Second sentence here is now redundant.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/template_url_service_util_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/template_url_service_util_unittest.cc:89: for (auto
itr = local_turls.begin(); itr != local_turls.end(); ++itr) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: Might as well use range-based for.

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
File components/search_engines/util.cc (right):

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:204: for (auto removed_engine :
actions.removed_engines) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const auto* (requires FindTemplateURL() signature change below)

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:219: for (auto edited_engine :
actions.edited_engines) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const auto&?

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:220: TemplateURLData& data =
edited_engine.second;
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const TemplateURLData&

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:230: for (const auto& added_engine :
actions.added_engines) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: {} unnecessary

Done.

https://codereview.chromium.org/2290503003/diff/60001/components/search_engin...
components/search_engines/util.cc:385: TemplateURL* url) {
On 2016/08/31 04:12:57, Peter Kasting wrote:
> Nit: const TemplateURL*

Done.

[commit-bot: I haz the power, 2016-09-01 00:36:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 00:36:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2016-09-01 01:24:41]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 01:25:09]

Dry run: 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-09-01 02:43:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 02:43:57]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-09-01 08:21:39]

LGTM

You are awesome!  http://youtu.be/StTqXEQ2l-Y

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/autocom...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/autocom...
chrome/browser/autocomplete/search_provider_unittest.cc:289:
turl_model->Add(base::WrapUnique(default_t_url_));
Nit: These two lines could become:

  default_t_url_ = turl_model->Add(base::MakeUnique<TemplateURL>(data));

Also applies to the other changes in this file.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/importe...
File chrome/browser/importer/profile_writer.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/importe...
chrome/browser/importer/profile_writer.cc:345: model->Add(base::WrapUnique(*i));
Extra super bonus nit that you don't have to do: If this function took an
OwningTemplateURLVector, then I think we could basically just do
"model->Add(std::move(*i))" here and eliminate the next line.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2139:
default_turl.get(), "http://wrong.url.com?q={searchTerms}", "default");
Nit: Could be inlined into the call below, though I don't know if this would
make it less readable.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2169:
"http://new.wrong.url.com?q={searchTerms}", "default");
Nit: Same comment.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_unittest.cc:1424:
model()->AddExtensionControlledTURL(std::move(ext_dse_ptr),
Nit: Does it make sense for this Add() variant to also return a raw pointer?

We may still end up wanting to de-inline the creation of these objects for
readability, I dunno.  But it'd affect at least the four places here and below
that call this function.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1117:
template_url_service->Add(base::WrapUnique(template_url));
Nit: Can be

  TemplateURL* template_url =
      template_url_service->Add(base::MakeUnique<TemplateURL>(data));

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/sear...
File chrome/browser/ui/search_engines/edit_search_engine_controller.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/sear...
chrome/browser/ui/search_engines/edit_search_engine_controller.cc:108:
template_url_service->AddWithOverrides(base::WrapUnique(template_url_),
Urgh, apparently sometimes this class owns this raw pointer and sometimes it
doesn't.  Pretty sure cleaning that up is beyond the scope of this CL...

https://codereview.chromium.org/2290503003/diff/100001/components/omnibox/bro...
File components/omnibox/browser/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/omnibox/bro...
components/omnibox/browser/history_url_provider_unittest.cc:911:
template_url_service->Add(base::WrapUnique(template_url));
Nit: Can be

  TemplateURL* template_url =
      template_url_service->Add(base::MakeUnique<TemplateURL>(data));

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/default_search_policy_handler.cc:308: return true;
Waste of time: I looked for a way to do this sort of thing more
"<algorithm>ically" and came up with the following expression that is just as
long and harder to read:

  return std::find_if(
             handlers_.begin(), handlers_.end(),
             [policies, errors](const auto& handler) {
               return !handler->CheckPolicySettings(policies, errors); }) !=
         handlers_.end();

This would probably be a bad change.  But ya know, I put several minutes into
it.

We really need versions of the standard algorithms that just take the container,
or some other way of copying some of the simplicity of range-based for and not
having to write "handlers_.begin(), handlers_.end(),".

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/search_host_to_urls_map_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/search_host_to_urls_map_unittest.cc:55:
ASSERT_EQ(&new_t_url, provider_map_->GetTemplateURLForHost(new_host));
Nit: Honestly, most of the ASSERTs in this file should really be EXPECTs, but
whatever

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/template_url_service.cc:484: if
(template_url_ptr->extension_info_->wants_to_be_default_engine) {
Nit: {} unnecessary

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/template_url_service.cc:1571: urls->begin(),
urls->end(), [](const std::unique_ptr<TemplateURL>& turl) {
Nit: Feel free to use const auto& here if you think it would be more readable (I
have no strong opinion)

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/template_url_service_util_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/template_url_service_util_unittest.cc:75:
local_turls.push_back(std::move(default_turl_ptr));
Nit: Shorter:

  local_turls.push_back(CreatePrepopulateTemplateURL(2, "winner5", 12));
  TemplateURL* default_turl = local_turls.back().get();

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/util.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/util.cc:204: for (const auto& removed_engine :
actions.removed_engines) {
Nit: I think this should have been const auto* instead of const auto&.

[Avi (use Gerrit), 2016-09-01 15:14:16]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 15:14:43]

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

[Avi (use Gerrit), 2016-09-01 15:14:47]

Question: are the fixups outside of search_engines routine enough to be
TBR-able?

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/autocom...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/autocom...
chrome/browser/autocomplete/search_provider_unittest.cc:289:
turl_model->Add(base::WrapUnique(default_t_url_));
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: These two lines could become:
> 
>   default_t_url_ = turl_model->Add(base::MakeUnique<TemplateURL>(data));
> 
> Also applies to the other changes in this file.

Done.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/importe...
File chrome/browser/importer/profile_writer.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/importe...
chrome/browser/importer/profile_writer.cc:345: model->Add(base::WrapUnique(*i));
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Extra super bonus nit that you don't have to do: If this function took an
> OwningTemplateURLVector, then I think we could basically just do
> "model->Add(std::move(*i))" here and eliminate the next line.

A pass to remove ScopedVector is next, but I figured one big sweeping change at
a time. :)

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2139:
default_turl.get(), "http://wrong.url.com?q={searchTerms}", "default");
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: Could be inlined into the call below, though I don't know if this would
> make it less readable.

Hmm.... I'll pass here. For a one-param function, sure, but when you get to two,
that feels like a bit much.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2169:
"http://new.wrong.url.com?q={searchTerms}", "default");
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: Same comment.

Acknowledged.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_unittest.cc:1424:
model()->AddExtensionControlledTURL(std::move(ext_dse_ptr),
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: Does it make sense for this Add() variant to also return a raw pointer?
> 
> We may still end up wanting to de-inline the creation of these objects for
> readability, I dunno.  But it'd affect at least the four places here and below
> that call this function.

Might as well for completeness's sake.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1117:
template_url_service->Add(base::WrapUnique(template_url));
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: Can be
> 
>   TemplateURL* template_url =
>       template_url_service->Add(base::MakeUnique<TemplateURL>(data));

Done.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/sear...
File chrome/browser/ui/search_engines/edit_search_engine_controller.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/sear...
chrome/browser/ui/search_engines/edit_search_engine_controller.cc:108:
template_url_service->AddWithOverrides(base::WrapUnique(template_url_),
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Urgh, apparently sometimes this class owns this raw pointer and sometimes it
> doesn't.  Pretty sure cleaning that up is beyond the scope of this CL...

Yep, this is super sloppy. I'm going to pass since this CL is ugly enough. If
you want to take a spare moment, I'll let you have fun with it.

https://codereview.chromium.org/2290503003/diff/100001/components/omnibox/bro...
File components/omnibox/browser/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/omnibox/bro...
components/omnibox/browser/history_url_provider_unittest.cc:911:
template_url_service->Add(base::WrapUnique(template_url));
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: Can be
> 
>   TemplateURL* template_url =
>       template_url_service->Add(base::MakeUnique<TemplateURL>(data));

Done.

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/default_search_policy_handler.cc:308: return true;
On 2016/09/01 08:21:39, Peter Kasting wrote:
> We really need versions of the standard algorithms that just take the
container,
> or some other way of copying some of the simplicity of range-based for and not
> having to write "handlers_.begin(), handlers_.end(),".

Won't the "ranges" work in the C++ committee eventually end up helping here?

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/search_host_to_urls_map_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/search_host_to_urls_map_unittest.cc:55:
ASSERT_EQ(&new_t_url, provider_map_->GetTemplateURLForHost(new_host));
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: Honestly, most of the ASSERTs in this file should really be EXPECTs, but
> whatever

Yes, and I'm going to let that slide.

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/template_url_service.cc:484: if
(template_url_ptr->extension_info_->wants_to_be_default_engine) {
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: {} unnecessary

Yes, but I stared at the code without them and with nested if()s like this it
seemed clearer. Leaving them in as readability prerogative :)

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/template_url_service.cc:1571: urls->begin(),
urls->end(), [](const std::unique_ptr<TemplateURL>& turl) {
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: Feel free to use const auto& here if you think it would be more readable
(I
> have no strong opinion)

As the lambda parameter? That's a C++14 feature, to give lambdas generic
parameters.

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/template_url_service_util_unittest.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/template_url_service_util_unittest.cc:75:
local_turls.push_back(std::move(default_turl_ptr));
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: Shorter:
> 
>   local_turls.push_back(CreatePrepopulateTemplateURL(2, "winner5", 12));
>   TemplateURL* default_turl = local_turls.back().get();

Done.

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/util.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/util.cc:204: for (const auto& removed_engine :
actions.removed_engines) {
On 2016/09/01 08:21:39, Peter Kasting wrote:
> Nit: I think this should have been const auto* instead of const auto&.

Done.

[commit-bot: I haz the power, 2016-09-01 15:21:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 15:21:43]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Avi (use Gerrit), 2016-09-01 15:33:08]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 15:33:22]

Dry run: 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-09-01 15:42:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 15:42:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-09-01 19:41:18]

avi@chromium.org changed reviewers:
+ droger@chromium.org, thakis@chromium.org

[Avi (use Gerrit), 2016-09-01 19:41:19]

Nico: chrome/browser generic changes
David: one random ios file

[Avi (use Gerrit), 2016-09-01 20:19:08]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 20:19:47]

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

[Peter Kasting, 2016-09-01 20:25:05]

Probably best to go ahead and get OWNER signoff for those other changes rather
than TBR, but it's borderline.

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/sear...
File chrome/browser/ui/search_engines/edit_search_engine_controller.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/chrome/browser/ui/sear...
chrome/browser/ui/search_engines/edit_search_engine_controller.cc:108:
template_url_service->AddWithOverrides(base::WrapUnique(template_url_),
On 2016/09/01 15:14:47, Avi wrote:
> On 2016/09/01 08:21:39, Peter Kasting wrote:
> > Urgh, apparently sometimes this class owns this raw pointer and sometimes it
> > doesn't.  Pretty sure cleaning that up is beyond the scope of this CL...
> 
> Yep, this is super sloppy. I'm going to pass since this CL is ugly enough. If
> you want to take a spare moment, I'll let you have fun with it.

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=643358 .

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/default_search_policy_handler.cc:308: return true;
On 2016/09/01 15:14:47, Avi wrote:
> On 2016/09/01 08:21:39, Peter Kasting wrote:
> > We really need versions of the standard algorithms that just take the
> container,
> > or some other way of copying some of the simplicity of range-based for and
not
> > having to write "handlers_.begin(), handlers_.end(),".
> 
> Won't the "ranges" work in the C++ committee eventually end up helping here?

Maybe.  Haven't been following closely enough to know what their preferred
syntax is.

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/template_url_service.cc:484: if
(template_url_ptr->extension_info_->wants_to_be_default_engine) {
On 2016/09/01 15:14:47, Avi wrote:
> On 2016/09/01 08:21:39, Peter Kasting wrote:
> > Nit: {} unnecessary
> 
> Yes, but I stared at the code without them and with nested if()s like this it
> seemed clearer. Leaving them in as readability prerogative :)

Fine with me.

https://codereview.chromium.org/2290503003/diff/100001/components/search_engi...
components/search_engines/template_url_service.cc:1571: urls->begin(),
urls->end(), [](const std::unique_ptr<TemplateURL>& turl) {
On 2016/09/01 15:14:47, Avi wrote:
> On 2016/09/01 08:21:39, Peter Kasting wrote:
> > Nit: Feel free to use const auto& here if you think it would be more
readable
> (I
> > have no strong opinion)
> 
> As the lambda parameter? That's a C++14 feature, to give lambdas generic
> parameters.

Huh!  The More You Know

[commit-bot: I haz the power, 2016-09-01 20:53:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 20:53:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Avi (use Gerrit), 2016-09-01 21:40:05]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 21:40:52]

Dry run: 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-09-01 22:33:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 22:33:48]

Dry run: This issue passed the CQ dry run.

[droger, 2016-09-02 11:45:17]

On 2016/09/01 19:41:19, Avi wrote:
> David: one random ios file
lgtm

[Nico, 2016-09-02 16:22:22]

chrome/ lgtm

Nice change!

https://codereview.chromium.org/2290503003/diff/180001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/180001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:353:
std::unique_ptr<TemplateURL> turl) const {
(unrelated to your change, this feels like a somewhat strange api. If it only
wants a TemplateURL to pass on as reference, why does it require the template
url to be on the heap just so it can delete it?)

[Avi (use Gerrit), 2016-09-02 17:25:27]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-09-02 17:25:28]

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/2290503003/#ps180001
(title: "fix broken test")

[Avi (use Gerrit), 2016-09-02 17:25:38]

https://codereview.chromium.org/2290503003/diff/180001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/180001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:353:
std::unique_ptr<TemplateURL> turl) const {
On 2016/09/02 16:22:22, Nico wrote:
> (unrelated to your change, this feels like a somewhat strange api. If it only
> wants a TemplateURL to pass on as reference, why does it require the template
> url to be on the heap just so it can delete it?)

So it can reuse CreateTestTemplateURL ?

[commit-bot: I haz the power, 2016-09-02 17:25:55]

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-09-02 17:30:22]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-02 17:32:24]

Description was changed from

==========
Remove use of stl_util in search_engines.

BUG=555865
==========

to

==========
Remove use of stl_util in search_engines.

BUG=555865

Committed: https://crrev.com/8a64b715b5358377162509aa064242a266f37bdb
Cr-Commit-Position: refs/heads/master@{#416291}
==========

[commit-bot: I haz the power, 2016-09-02 17:32:26]

Patchset 10 (id:??) landed as
https://crrev.com/8a64b715b5358377162509aa064242a266f37bdb
Cr-Commit-Position: refs/heads/master@{#416291}

[Nico, 2016-09-02 17:34:49]

https://codereview.chromium.org/2290503003/diff/180001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/180001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:353:
std::unique_ptr<TemplateURL> turl) const {
On 2016/09/02 17:25:38, Avi wrote:
> On 2016/09/02 16:22:22, Nico wrote:
> > (unrelated to your change, this feels like a somewhat strange api. If it
only
> > wants a TemplateURL to pass on as reference, why does it require the
template
> > url to be on the heap just so it can delete it?)
> 
> So it can reuse CreateTestTemplateURL ?

Sure, but that function is local to this file and as far as I can tell nothing
else requires it to produce results on the heap. (Didn't look super carefully
though.)

[Peter Kasting, 2016-09-02 19:22:22]

https://codereview.chromium.org/2290503003/diff/180001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2290503003/diff/180001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:353:
std::unique_ptr<TemplateURL> turl) const {
On 2016/09/02 17:34:49, Nico (away until Tuesday) wrote:
> On 2016/09/02 17:25:38, Avi wrote:
> > On 2016/09/02 16:22:22, Nico wrote:
> > > (unrelated to your change, this feels like a somewhat strange api. If it
> only
> > > wants a TemplateURL to pass on as reference, why does it require the
> template
> > > url to be on the heap just so it can delete it?)
> > 
> > So it can reuse CreateTestTemplateURL ?
> 
> Sure, but that function is local to this file and as far as I can tell nothing
> else requires it to produce results on the heap. (Didn't look super carefully
> though.)

Yeah, this probably should have taken a const TemplateURL& instead, and callers
added a '*'.  I didn't peer closely enough at this.