Remove ScopedVector from search_engines.

Remove ScopedVector from search_engines.

(This also fixes a memory leak in the ios code.)

BUG=554289
Committed: https://crrev.com/e8828f24dc78cb5f894ea0bc40a652b2a569bd8d
Cr-Commit-Position: refs/heads/master@{#416299}

[Avi (use Gerrit), 2016-09-01 21:25: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 21:25:55]

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 21:41:58]

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

[commit-bot: I haz the power, 2016-09-01 21:41:58]

Dry run: Try jobs failed on following builders:
  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-09-01 21:43:32]

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:44:12]

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 22:03:15]

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

[commit-bot: I haz the power, 2016-09-01 22:03:21]

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 22:32:46]

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

[commit-bot: I haz the power, 2016-09-01 22:52:41]

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

[commit-bot: I haz the power, 2016-09-01 22:52:42]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-09-01 23:17:33]

LGTM

https://codereview.chromium.org/2307663002/diff/40001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/2307663002/diff/40001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1167: return
std::move(loaded_urls[default_search_index]);
Nit: Maybe

  return (default_search_index < loaded_urls.size())
             ? std::move(loaded_urls[default_search_index])
             : nullptr;

https://codereview.chromium.org/2307663002/diff/40001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1169: return
std::unique_ptr<TemplateURLData>();
Nit: return nullptr?

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

https://codereview.chromium.org/2307663002/diff/40001/components/search_engin...
components/search_engines/template_url_service.cc:1743: for (auto& url :
prepopulated_urls) {
Nit: const auto&?

[Avi (use Gerrit), 2016-09-01 23:26:42]

avi@chromium.org changed reviewers:
+ droger@chromium.org, rdevlin.cronin@chromium.org

[Avi (use Gerrit), 2016-09-01 23:26:43]

Devlin: extension change
David: one random ios file

https://codereview.chromium.org/2307663002/diff/40001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/2307663002/diff/40001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1167: return
std::move(loaded_urls[default_search_index]);
On 2016/09/01 23:17:32, Peter Kasting wrote:
> Nit: Maybe
> 
>   return (default_search_index < loaded_urls.size())
>              ? std::move(loaded_urls[default_search_index])
>              : nullptr;

Done.

https://codereview.chromium.org/2307663002/diff/40001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1169: return
std::unique_ptr<TemplateURLData>();
On 2016/09/01 23:17:32, Peter Kasting wrote:
> Nit: return nullptr?

Acknowledged.

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

https://codereview.chromium.org/2307663002/diff/40001/components/search_engin...
components/search_engines/template_url_service.cc:1743: for (auto& url :
prepopulated_urls) {
On 2016/09/01 23:17:32, Peter Kasting wrote:
> Nit: const auto&?

Done.

[Avi (use Gerrit), 2016-09-01 23:27:36]

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

[commit-bot: I haz the power, 2016-09-01 23:27:56]

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-02 00:21:51]

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

[commit-bot: I haz the power, 2016-09-02 00:21:51]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-09-02 00:49:09]

extensions lgtm

https://codereview.chromium.org/2307663002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc
(right):

https://codereview.chromium.org/2307663002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:56:
*data = *engine;
Total drive-by nit: it looks like there's no reason to do a copy here rather
than use the std::unique_ptr we already have.  Can you put:
// TODO(devlin): Use the std::unique_ptr instead of a copy here.
?

(It's probably out of scope for this change, but no reason to cause extra work!
:))

[Peter Kasting, 2016-09-02 01:06:25]

https://codereview.chromium.org/2307663002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc
(right):

https://codereview.chromium.org/2307663002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:56:
*data = *engine;
On 2016/09/02 00:49:09, Devlin wrote:
> Total drive-by nit: it looks like there's no reason to do a copy here rather
> than use the std::unique_ptr we already have.  Can you put:
> // TODO(devlin): Use the std::unique_ptr instead of a copy here.
> ?
> 
> (It's probably out of scope for this change, but no reason to cause extra
work!
> :))

Looks like just fixing now would be pretty simple.  We can change this function
to:

std::unique_ptr<TemplateURLData> GetPrepopulatedSearchProvider(
    PrefService* prefs,
    int prepopulated_id) {
  ...
  for (auto& engine : engines) {
    if (engine->prepopulate_id == prepopulated_id)
      return std::move(engine);
  }  // Note, this loop could optionally be replaced by find_if()
  return nullptr;
}

Then in ConvertSearchProvider():

  std::unique_ptr<TemplateURLData> data;
  if (search_provider.prepopulated_id) {
    data = GetPrepopulatedSearchProvider(prefs,
                                         *search_provider.prepopulated_id);
    if (!data) {
      VLOG(1) << ...;
    }
  }

  ... change "data." to "data->" here ...

[Avi (use Gerrit), 2016-09-02 01:08:32]

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

[Avi (use Gerrit), 2016-09-02 01:08:37]

devlin ptal

https://codereview.chromium.org/2307663002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc
(right):

https://codereview.chromium.org/2307663002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:56:
*data = *engine;
Fixed, ptal.

[commit-bot: I haz the power, 2016-09-02 01:08:48]

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

[Devlin, 2016-09-02 01:24:09]

extensions s-lgtm

https://codereview.chromium.org/2307663002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc
(right):

https://codereview.chromium.org/2307663002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:56:
*data = *engine;
On 2016/09/02 01:08:37, Avi wrote:
> Fixed, ptal.

Thanks!  I didn't want to volunteer you for more work than necessary as an
unrelated drive-by. :)

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

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

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

Dry run: This issue passed the CQ dry run.

[droger, 2016-09-02 08:34:26]

lgtm

https://codereview.chromium.org/2307663002/diff/80001/ios/chrome/browser/sear...
File ios/chrome/browser/search_engines/search_engines_util.cc (left):

https://codereview.chromium.org/2307663002/diff/80001/ios/chrome/browser/sear...
ios/chrome/browser/search_engines/search_engines_util.cc:57: it =
new_engines.weak_erase(it);
If I understand correctly this was a memory leak. Can you expand the description
of the CL to mention that this is also fixing a memory leak in addition to doing
the scoped vector conversion?

[Avi (use Gerrit), 2016-09-02 13:57:57]

Description was changed from

==========
Remove ScopedVector from search_engines.

BUG=554289
==========

to

==========
Remove ScopedVector from search_engines.

(This also fixes a memory leak in the ios code.)

BUG=554289
==========

[Avi (use Gerrit), 2016-09-02 14:39:38]

https://codereview.chromium.org/2307663002/diff/80001/ios/chrome/browser/sear...
File ios/chrome/browser/search_engines/search_engines_util.cc (left):

https://codereview.chromium.org/2307663002/diff/80001/ios/chrome/browser/sear...
ios/chrome/browser/search_engines/search_engines_util.cc:57: it =
new_engines.weak_erase(it);
On 2016/09/02 08:34:26, droger wrote:
> If I understand correctly this was a memory leak. Can you expand the
description
> of the CL to mention that this is also fixing a memory leak in addition to
doing
> the scoped vector conversion?

Yes, this was a memory leak; we were constructing based on an object and then
leaking it.

Sure.

[Avi (use Gerrit), 2016-09-02 18:00:37]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-09-02 18:00:38]

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/2307663002/#ps80001
(title: "devlin")

[commit-bot: I haz the power, 2016-09-02 18:01:12]

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 18:08:23]

Description was changed from

==========
Remove ScopedVector from search_engines.

(This also fixes a memory leak in the ios code.)

BUG=554289
==========

to

==========
Remove ScopedVector from search_engines.

(This also fixes a memory leak in the ios code.)

BUG=554289
==========

[commit-bot: I haz the power, 2016-09-02 18:08:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-02 18:10:58]

Description was changed from

==========
Remove ScopedVector from search_engines.

(This also fixes a memory leak in the ios code.)

BUG=554289
==========

to

==========
Remove ScopedVector from search_engines.

(This also fixes a memory leak in the ios code.)

BUG=554289

Committed: https://crrev.com/e8828f24dc78cb5f894ea0bc40a652b2a569bd8d
Cr-Commit-Position: refs/heads/master@{#416299}
==========

[commit-bot: I haz the power, 2016-09-02 18:10:59]

Patchset 5 (id:??) landed as
https://crrev.com/e8828f24dc78cb5f894ea0bc40a652b2a569bd8d
Cr-Commit-Position: refs/heads/master@{#416299}