Some refactorings to facilitate a larger change to TemplateURLService.

Some refactorings to facilitate a larger change to TemplateURLService.

BUG=365762
R=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269310

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269723

[erikwright (departed), 2014-05-08 01:27:37]

pkasting: PTAL. A few things that can go before the big CL.

https://codereview.chromium.org/270533007/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/default_search_manager.cc (right):

https://codereview.chromium.org/270533007/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_manager.cc:102:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
This was accidentally committed as SYNCABLE.

[Peter Kasting, 2014-05-08 17:36:43]

LGTM

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/default_search_pref_migration.cc (right):

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_pref_migration.cc:26: pref_service,
&legacy_dse_from_prefs, &legacy_is_managed);
Nit: It would be nice to cleanup this function to return a scoped_ptr<> instead
of taking it as an outparam (the second arg).

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_pref_migration.cc:29: return
legacy_dse_from_prefs.Pass();
Nit: Shorter:

  return legacy_is_managed ?
      scoped_ptr<TemplateURLData>() : legacy_dse_from_prefs.Pass();

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:270:
DefaultSearchManager::ObserverCallback()) {
Nit: Can this be moved onto the previous line?  (2 places)

[erikwright (departed), 2014-05-08 18:24:20]

Addressed comments.

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/default_search_pref_migration.cc (right):

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_pref_migration.cc:26: pref_service,
&legacy_dse_from_prefs, &legacy_is_managed);
On 2014/05/08 17:36:43, Peter Kasting wrote:
> Nit: It would be nice to cleanup this function to return a scoped_ptr<>
instead
> of taking it as an outparam (the second arg).

It returns a bool at the moment, which indicates a succesful/failed load. A
successful load can still produce a NULL TemplateURL.

It's used in that format in TemplateURLService as of this CL, but will move here
before it's all over, at which time it should be as you say.

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_pref_migration.cc:29: return
legacy_dse_from_prefs.Pass();
On 2014/05/08 17:36:43, Peter Kasting wrote:
> Nit: Shorter:
> 
>   return legacy_is_managed ?
>       scoped_ptr<TemplateURLData>() : legacy_dse_from_prefs.Pass();

Done.

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/270533007/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:270:
DefaultSearchManager::ObserverCallback()) {
On 2014/05/08 17:36:43, Peter Kasting wrote:
> Nit: Can this be moved onto the previous line?  (2 places)

Done.

[erikwright (departed), 2014-05-08 18:24:25]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-05-08 18:27:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/270533007/120001

[commit-bot: I haz the power, 2014-05-08 23:01:38]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[erikwright (departed), 2014-05-09 15:22:46]

Committed patchset #8 manually as r269310 (presubmit successful).

[Alpha Left Google, 2014-05-10 20:00:47]

A revert of this CL has been created in
https://codereview.chromium.org/274323002/ by hclam@chromium.org.

The reason for reverting is: Use after free detected with this test:
EditSearchEngineControllerTest.EditTemplateURL

Failure bot:
http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%....

[Alpha Left Google, 2014-05-10 21:13:27]

A revert of this CL has been created in
https://codereview.chromium.org/279063002/ by hclam@chromium.org.

The reason for reverting is: See memory failure: bot:
http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%....

[erikwright (departed), 2014-05-11 17:46:33]

Fixed test failure in https://codereview.chromium.org/276063003.

[erikwright (departed), 2014-05-11 17:46:41]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-05-11 17:46:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/270533007/140001

[commit-bot: I haz the power, 2014-05-11 23:23:07]

Change committed as 269723