Extract the processing of the web data service results processing in a standalone function.

Extract the processing of the web data service results processing in a standalone function.

BUG=38475
TEST=unit_test --gtest_filter=Temp*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57563

[sky, 2010-08-25 17:56:15]

http://codereview.chromium.org/3110038/diff/2001/3002
File chrome/browser/search_engines/template_url_model.cc (left):

http://codereview.chromium.org/3110038/diff/2001/3002#oldcode773
chrome/browser/search_engines/template_url_model.cc:773: Update(existing_url,
*t_url);
Seems like the new code doesn't update the db at this step.

It would be good to add a test case for this.

http://codereview.chromium.org/3110038/diff/2001/3002#oldcode779
chrome/browser/search_engines/template_url_model.cc:779:
SetDefaultSearchProvider(loaded_urls[i]);
This too doesn't look like it's happening in the new patch either, which means
the db might be out of sync.

A test case for this would be great.

http://codereview.chromium.org/3110038/diff/2001/3002#oldcode793
chrome/browser/search_engines/template_url_model.cc:793: Remove(template_url);
And it doesn't look like the new code updates the db here either.

It would be good to add a test case for this.

http://codereview.chromium.org/3110038/diff/2001/3005
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/3110038/diff/2001/3005#newcode17
chrome/browser/search_engines/util.cc:17: #include
"chrome/browser/pref_service.h"
this should be at line 15.

http://codereview.chromium.org/3110038/diff/2001/3005#newcode51
chrome/browser/search_engines/util.cc:51: service->RemoveKeyword(**i);
This should only be done when this function is called from the TemplateURLModel.
Perhaps a boolean parameter to GetSearchProvidersUsing...

http://codereview.chromium.org/3110038/diff/2001/3005#newcode182
chrome/browser/search_engines/util.cc:182: *new_resource_keyword_version = 0;
Can you set this to 0 early on in the function? I'm a bit worried about someone
adding an early return and missing it.

http://codereview.chromium.org/3110038/diff/2001/3006
File chrome/browser/search_engines/util.h (right):

http://codereview.chromium.org/3110038/diff/2001/3006#newcode31
chrome/browser/search_engines/util.h:31: void
GetSearchProvidersUsingKeywordResult(
Document the caller owners the TemplateURL* in template_urls and that
default_search_provider is contained in template_urls.

http://codereview.chromium.org/3110038/diff/2001/3006#newcode32
chrome/browser/search_engines/util.h:32: const WDTypedResult* result,
Can this be const WDTypeResult& ?

[levin, 2010-08-26 03:40:25]

In short all fixed.

http://codereview.chromium.org/3110038/diff/2001/3002
File chrome/browser/search_engines/template_url_model.cc (left):

http://codereview.chromium.org/3110038/diff/2001/3002#oldcode773
chrome/browser/search_engines/template_url_model.cc:773: Update(existing_url,
*t_url);
On 2010/08/25 17:56:15, sky wrote:
> Seems like the new code doesn't update the db at this step.
> 
> It would be good to add a test case for this.
Done.

TemplateURLModelTest.LoadUpdatesDefaultSearchUrl and
TemplateURLModelTest.LoadUpdatesSearchUrl

http://codereview.chromium.org/3110038/diff/2001/3002#oldcode779
chrome/browser/search_engines/template_url_model.cc:779:
SetDefaultSearchProvider(loaded_urls[i]);
On 2010/08/25 17:56:15, sky wrote:
> This too doesn't look like it's happening in the new patch either, which means
> the db might be out of sync.
> 
> A test case for this would be great.

Done.

TemplateURLModelTest.LoadSavesPrepopulatedDefaultSearchProvider

http://codereview.chromium.org/3110038/diff/2001/3002#oldcode793
chrome/browser/search_engines/template_url_model.cc:793: Remove(template_url);
On 2010/08/25 17:56:15, sky wrote:
> And it doesn't look like the new code updates the db here either.
> 
> It would be good to add a test case for this.

Done.

TemplateURLModelTest.LoadDeletesUnusedProvider

http://codereview.chromium.org/3110038/diff/2001/3005
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/3110038/diff/2001/3005#newcode17
chrome/browser/search_engines/util.cc:17: #include
"chrome/browser/pref_service.h"
On 2010/08/25 17:56:15, sky wrote:
> this should be at line 15.

Done.

http://codereview.chromium.org/3110038/diff/2001/3005#newcode51
chrome/browser/search_engines/util.cc:51: service->RemoveKeyword(**i);
On 2010/08/25 17:56:15, sky wrote:
> This should only be done when this function is called from the
TemplateURLModel.
> Perhaps a boolean parameter to GetSearchProvidersUsing...

I added a comment about when service should != NULL.

http://codereview.chromium.org/3110038/diff/2001/3005#newcode116
chrome/browser/search_engines/util.cc:116: *default_search_provider =
loaded_urls[i];
There is a bug fix here in next version of the patch, and it is verified by
TemplateURLModelTest.LoadUpdatesDefaultSearchUrl

http://codereview.chromium.org/3110038/diff/2001/3005#newcode182
chrome/browser/search_engines/util.cc:182: *new_resource_keyword_version = 0;
On 2010/08/25 17:56:15, sky wrote:
> Can you set this to 0 early on in the function? I'm a bit worried about
someone
> adding an early return and missing it.

Done.

http://codereview.chromium.org/3110038/diff/2001/3006
File chrome/browser/search_engines/util.h (right):

http://codereview.chromium.org/3110038/diff/2001/3006#newcode31
chrome/browser/search_engines/util.h:31: void
GetSearchProvidersUsingKeywordResult(
On 2010/08/25 17:56:15, sky wrote:
> Document the caller owners the TemplateURL* in template_urls and that
> default_search_provider is contained in template_urls.

Done.

http://codereview.chromium.org/3110038/diff/2001/3006#newcode32
chrome/browser/search_engines/util.h:32: const WDTypedResult* result,
On 2010/08/25 17:56:15, sky wrote:
> Can this be const WDTypeResult& ?

Done.

[levin, 2010-08-26 03:41:49]

And great catches. Thank you very much.

[sky, 2010-08-26 17:15:52]

http://codereview.chromium.org/3110038/diff/8/20002
File chrome/browser/search_engines/template_url_model.cc (right):

http://codereview.chromium.org/3110038/diff/8/20002#newcode510
chrome/browser/search_engines/template_url_model.cc:510:
default_search_provider_ = NULL;
Is it necessary to set default_search_provider_ to NULL here? If so, please add
a comment as to why.

http://codereview.chromium.org/3110038/diff/8/20004
File chrome/browser/search_engines/template_url_model_unittest.cc (right):

http://codereview.chromium.org/3110038/diff/8/20004#newcode808
chrome/browser/search_engines/template_url_model_unittest.cc:808:
ResetModel(true);
It doesn't seem like any of these tests trigger the merging code page now. Do
they?

http://codereview.chromium.org/3110038/diff/8/20004#newcode820
chrome/browser/search_engines/template_url_model_unittest.cc:820: // Make sure
that load routine doesn't delete
Please flow your comments to 80 chars.

http://codereview.chromium.org/3110038/diff/8/20005
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/3110038/diff/8/20005#newcode18
chrome/browser/search_engines/util.cc:18: 
Nuke this line (only one empty line between includes.

http://codereview.chromium.org/3110038/diff/8/20006
File chrome/browser/search_engines/util.h (right):

http://codereview.chromium.org/3110038/diff/8/20006#newcode34
chrome/browser/search_engines/util.h:34: // updated for things that happen to
results.
Nuke the 'for things to happen to results' as its a bit nebulous what that
means.

[levin, 2010-08-26 18:18:22]

All done as requested.

http://codereview.chromium.org/3110038/diff/8/20002
File chrome/browser/search_engines/template_url_model.cc (right):

http://codereview.chromium.org/3110038/diff/8/20002#newcode510
chrome/browser/search_engines/template_url_model.cc:510:
default_search_provider_ = NULL;
On 2010/08/26 17:15:52, sky wrote:
> Is it necessary to set default_search_provider_ to NULL here? If so, please
add
> a comment as to why.

You're right. I wanted to make sure the set happened, but no place sets
default_search_provider_ directly (except just a few lines above).

http://codereview.chromium.org/3110038/diff/8/20004
File chrome/browser/search_engines/template_url_model_unittest.cc (right):

http://codereview.chromium.org/3110038/diff/8/20004#newcode808
chrome/browser/search_engines/template_url_model_unittest.cc:808:
ResetModel(true);
On 2010/08/26 17:15:52, sky wrote:
> It doesn't seem like any of these tests trigger the merging code page now. Do
> they?

As discussed, they do.

Added a comment to ChangeModelToLoadState.

http://codereview.chromium.org/3110038/diff/8/20004#newcode820
chrome/browser/search_engines/template_url_model_unittest.cc:820: // Make sure
that load routine doesn't delete
On 2010/08/26 17:15:52, sky wrote:
> Please flow your comments to 80 chars.

Done.

http://codereview.chromium.org/3110038/diff/8/20005
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/3110038/diff/8/20005#newcode18
chrome/browser/search_engines/util.cc:18: 
On 2010/08/26 17:15:52, sky wrote:
> Nuke this line (only one empty line between includes.

Done.

http://codereview.chromium.org/3110038/diff/8/20006
File chrome/browser/search_engines/util.h (right):

http://codereview.chromium.org/3110038/diff/8/20006#newcode34
chrome/browser/search_engines/util.h:34: // updated for things that happen to
results.
On 2010/08/26 17:15:52, sky wrote:
> Nuke the 'for things to happen to results' as its a bit nebulous what that
> means.

Done.

[sky, 2010-08-26 18:22:10]

LGTM