More misc. cleanups to minimize future refactoring diffs.

More misc. cleanups to minimize future refactoring diffs:

* Fix indentation
* Slightly more const-correct TemplateURLData functions
* Use EXPECT_XXX() where possible, but ASSERT_XXX() to avoid crashes on expectation failure
* Remove unneeded #include
* Simplify AlternateErrorPageTabObserver slightly by having constructor take a Profile
* Shorter/simpler code
* Fix ordering in some places to be more consistent (e.g. matching member declaration order)
* Add some DCHECK()s to make more explicit some conditions that will become important later

Also, template_url_service.cc has some functional changes:

* GetTemplateURLForXXX() functions now return correct values when asked for the default search URL before the model has successfully loaded.  I wound up needing this later to fix some case, but I forget what now; the change seems more correct anyway.
* Similarly, UnregisterExtensionKeyword() now works correctly if called before loading has finished
* RegisterExtensionKeyword() now does nothing when there's already a keyword for that extension, as the comments in the header claim.  I'd think this wouldn't actually matter in normal use as I think extension upgrades first unregister the old extension?  (I don't remember if I checked that, though)
* Don't try to set an extension keyword as default -- this would be a bad UX as these are inherently async.  I don't think this should have been possible anyway except maybe if there was some kind of unintentional overlap between extensions and policy??
* Don't try to save extension keywords to disk or sync them (as the extension system should be responsible for registering them if necessary on startup in both these cases).  We already avoided syncing these, I just added checks in more places; I no longer recall if we had code to avoid persisting these to disk, I don't seem to see any at the moment.  (sky?)

BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131619

[Peter Kasting, 2012-04-10 01:24:33]

sky: review
atwilson: chrome/browser/sync OWNERS

https://chromiumcodereview.appspot.com/10033017/diff/1/chrome/browser/instant...
File chrome/browser/instant/instant_loader.cc (right):

https://chromiumcodereview.appspot.com/10033017/diff/1/chrome/browser/instant...
chrome/browser/instant/instant_loader.cc:1133:
TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, string16()));
sky, please make sure this value change (-1 == NO_SUGGESTION_CHOSEN) is correct.

https://chromiumcodereview.appspot.com/10033017/diff/1/chrome/browser/search_...
File chrome/browser/search_engines/search_provider_install_data.cc (left):

https://chromiumcodereview.appspot.com/10033017/diff/1/chrome/browser/search_...
chrome/browser/search_engines/search_provider_install_data.cc:148: return
template_url && requested_origin ==
I don't think our lone caller will ever pass NULL here.

https://chromiumcodereview.appspot.com/10033017/diff/1/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/10033017/diff/1/chrome/browser/search_...
chrome/browser/search_engines/template_url_service.cc:1204: AddNoNotify(*i,
false);
I changed this call because in the future I need to add some code to handle
keyword conflicts, and I wanted to add it to one function (AddNoNotify())
instead of insert calls to it both there and here.

https://chromiumcodereview.appspot.com/10033017/diff/1/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10033017/diff/1/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_unittest.cc:494: // Don't
generate the empty string.
I added this condition to GenerateKeyword() a little while back but forgot to
also add a test.

[sky, 2012-04-10 16:16:21]

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider.h (right):

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/auto...
chrome/browser/autocomplete/search_provider.h:123: bool valid_default_provider()
const { return !!default_provider_; }
nit: !! is obscure.

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/sear...
File chrome/browser/search_engines/search_provider_install_data.cc (left):

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/sear...
chrome/browser/search_engines/search_provider_install_data.cc:148: return
template_url && requested_origin ==
How come it's safe to remove the template_url check here?

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:618: for
(std::vector<TemplateURL*>::const_iterator i = template_urls.begin();
nit: refactor so that this and 496-502 can share code.

[Peter Kasting, 2012-04-10 17:53:43]

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider.h (right):

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/auto...
chrome/browser/autocomplete/search_provider.h:123: bool valid_default_provider()
const { return !!default_provider_; }
On 2012/04/10 16:16:22, sky wrote:
> nit: !! is obscure.

It is, but I'm just moving this code up from below.  I'll be replacing it
entirely very soon, so I'd rather just leave this for now and kill it later.

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/sear...
File chrome/browser/search_engines/search_provider_install_data.cc (left):

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/sear...
chrome/browser/search_engines/search_provider_install_data.cc:148: return
template_url && requested_origin ==
On 2012/04/10 16:16:22, sky wrote:
> How come it's safe to remove the template_url check here?

I wrote comments in patch set 1 for this and a few other places I thought needed
explanation -- see
http://codereview.chromium.org/10033017/diff/1/chrome/browser/search_engines/...
.  Basically, the answer is that this is called in one place and I don't think
that place can ever pass NULL.

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/10033017/diff/5009/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:618: for
(std::vector<TemplateURL*>::const_iterator i = template_urls.begin();
On 2012/04/10 16:16:22, sky wrote:
> nit: refactor so that this and 496-502 can share code.

Right now these loops use different container types.  I can consolidate them
once I land changes to sanitize our use of "const", since that will bring them
into line with each other.

[sky, 2012-04-10 19:47:06]

LGTM

[Raghu Simha, 2012-04-10 20:14:01]

Sync change LGTM.