Fixes for first run path....

Some first run fixes:
Add missing first run bubble back to opening page.
Wait for search engines to load before displaying first run bubble, so correct search engine is displayed in first run.
Because some distributions use "skip-first-run-ui", add back the "import_items" code that passes values to this code path.  Because this code path is obsolete (most import is silent), this needs to be refactored in the long term.
Fix some spelling issues.

BUG=42612
TEST= import with --skip-first-run-ui works 


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

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

[Miranda Callahan, 2010-07-21 00:18:01]

Peter, could you review the location_bar_view files?  I implemented the
template_url_model waiting code we discussed.

Glen, could you look at the changes to first run win?  Just some minor bug fixes
for the new FRUI.

[Glen Murphy, 2010-07-21 00:23:50]

first run win LGTM

On 2010/07/21 00:18:01, Miranda Callahan wrote:
> Peter, could you review the location_bar_view files?  I implemented the
> template_url_model waiting code we discussed.
> 
> Glen, could you look at the changes to first run win?  Just some minor bug
fixes
> for the new FRUI.

[Peter Kasting, 2010-07-21 00:44:53]

http://codereview.chromium.org/3042010/diff/16001/17001
File chrome/browser/first_run_win.cc (left):

http://codereview.chromium.org/3042010/diff/16001/17001#oldcode321
chrome/browser/first_run_win.cc:321: out_prefs->dont_import_items +=
importer::SEARCH_ENGINES;
I assume "don't import" is the default here, hence this removal?

http://codereview.chromium.org/3042010/diff/16001/17001
File chrome/browser/first_run_win.cc (right):

http://codereview.chromium.org/3042010/diff/16001/17001#newcode340
chrome/browser/first_run_win.cc:340: import_items |= importer::HISTORY;
I'm confused; here you use |=, above you use +=.  Which should it be?

http://codereview.chromium.org/3042010/diff/16001/17002
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/3042010/diff/16001/17002#newcode953
chrome/browser/views/location_bar/location_bar_view.cc:953:
profile_->GetTemplateURLModel()->AddObserver(this);
Shouldn't this be balanced with a RemoveObserver() once we get the callback?

http://codereview.chromium.org/3042010/diff/16001/17002#newcode957
chrome/browser/views/location_bar/location_bar_view.cc:957: // We wait 30
milliseconds to open. It allows less flicker.
Is this needed anymore?  I would guess "no"...

[Miranda Callahan, 2010-07-21 00:53:44]

Thanks for your comments; please take another look.

http://codereview.chromium.org/3042010/diff/16001/17001
File chrome/browser/first_run_win.cc (left):

http://codereview.chromium.org/3042010/diff/16001/17001#oldcode321
chrome/browser/first_run_win.cc:321: out_prefs->dont_import_items +=
importer::SEARCH_ENGINES;
On 2010/07/21 00:44:54, Peter Kasting wrote:
> I assume "don't import" is the default here, hence this removal?

That's correct.

http://codereview.chromium.org/3042010/diff/16001/17001
File chrome/browser/first_run_win.cc (right):

http://codereview.chromium.org/3042010/diff/16001/17001#newcode340
chrome/browser/first_run_win.cc:340: import_items |= importer::HISTORY;
On 2010/07/21 00:44:54, Peter Kasting wrote:
> I'm confused; here you use |=, above you use +=.  Which should it be?

Made uniformly |+.

http://codereview.chromium.org/3042010/diff/16001/17002
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/3042010/diff/16001/17002#newcode953
chrome/browser/views/location_bar/location_bar_view.cc:953:
profile_->GetTemplateURLModel()->AddObserver(this);
On 2010/07/21 00:44:54, Peter Kasting wrote:
> Shouldn't this be balanced with a RemoveObserver() once we get the callback?

Ack, yes -- done.

http://codereview.chromium.org/3042010/diff/16001/17002#newcode957
chrome/browser/views/location_bar/location_bar_view.cc:957: // We wait 30
milliseconds to open. It allows less flicker.
On 2010/07/21 00:44:54, Peter Kasting wrote:
> Is this needed anymore?  I would guess "no"...

I concur -- removed.

[Peter Kasting, 2010-07-21 01:04:04]

LGTM

http://codereview.chromium.org/3042010/diff/20002/4003
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/3042010/diff/20002/4003#newcode114
chrome/browser/views/location_bar/location_bar_view.cc:114:
profile_->GetTemplateURLModel()->RemoveObserver(this);
Nit: It might be nice to comment this with something like:

// This is in case we're destroyed before the model loads.  This is safe (a
no-op) if we're not a registered observer.

[Miranda Callahan, 2010-07-21 02:15:53]

http://codereview.chromium.org/3042010/diff/20002/4003
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/3042010/diff/20002/4003#newcode114
chrome/browser/views/location_bar/location_bar_view.cc:114:
profile_->GetTemplateURLModel()->RemoveObserver(this);
On 2010/07/21 01:04:04, Peter Kasting wrote:
> Nit: It might be nice to comment this with something like:
> 
> // This is in case we're destroyed before the model loads.  This is safe (a
> no-op) if we're not a registered observer.

Done.

[Miranda Callahan, 2010-07-22 01:17:32]

Peter -- calling profile_->GetTemplateModel() in the destructor of
LocationBarView caused crashes throughout the ui_tests, so I've tweaked the code
to fix this -- now, if we are observing the model in first run we store it in an
instance var, and check to see whether it exists before removing ourselves as an
observer.  Tests seem ok with this.

[Peter Kasting, 2010-07-22 01:32:39]

http://codereview.chromium.org/3042010/diff/31001/12005
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/3042010/diff/31001/12005#newcode113
chrome/browser/views/location_bar/location_bar_view.cc:113: // This is in case
we're destroyed before the model loads. We store the model
Nit: Put the explanation of why we store this on the variable's declaration.

http://codereview.chromium.org/3042010/diff/31001/12005#newcode114
chrome/browser/views/location_bar/location_bar_view.cc:114: // because calling
profile_->GetTemplateModel() in the destructor causes a
Nit: GetTemplateModel() -> GetTemplateURLModel()

http://codereview.chromium.org/3042010/diff/31001/12005#newcode1051
chrome/browser/views/location_bar/location_bar_view.cc:1051: if
(template_url_model_)
Nit: How can this conditional fail?

http://codereview.chromium.org/3042010/diff/31001/12005#newcode1052
chrome/browser/views/location_bar/location_bar_view.cc:1052:
template_url_model_->RemoveObserver(this);
Nit: NULL out template_url_model_ here

[Miranda Callahan, 2010-07-22 02:39:04]

Thanks for your feedback -- fixed all nits.

http://codereview.chromium.org/3042010/diff/31001/12005
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/3042010/diff/31001/12005#newcode113
chrome/browser/views/location_bar/location_bar_view.cc:113: // This is in case
we're destroyed before the model loads. We store the model
On 2010/07/22 01:32:39, Peter Kasting wrote:
> Nit: Put the explanation of why we store this on the variable's declaration.

Done.

http://codereview.chromium.org/3042010/diff/31001/12005#newcode114
chrome/browser/views/location_bar/location_bar_view.cc:114: // because calling
profile_->GetTemplateModel() in the destructor causes a
On 2010/07/22 01:32:39, Peter Kasting wrote:
> Nit: GetTemplateModel() -> GetTemplateURLModel()

Done.

http://codereview.chromium.org/3042010/diff/31001/12005#newcode1051
chrome/browser/views/location_bar/location_bar_view.cc:1051: if
(template_url_model_)
On 2010/07/22 01:32:39, Peter Kasting wrote:
> Nit: How can this conditional fail?

What a very good question.  Removed.

http://codereview.chromium.org/3042010/diff/31001/12005#newcode1052
chrome/browser/views/location_bar/location_bar_view.cc:1052:
template_url_model_->RemoveObserver(this);
On 2010/07/22 01:32:39, Peter Kasting wrote:
> Nit: NULL out template_url_model_ here

Done.

[Peter Kasting, 2010-07-22 03:09:02]

LGTM