New first run sequence for Windows.

New first run sequence for Windows, with no UI.  Organic builds will display the Search Engine Dialog, otherwise search engine is set to Google or silently imported, depending on master_preferences.  (see go/chromefirstrun for details).

I also removed a no-longer-used SearchSelectObserver from FirstRunSearchView.

BUG=42612
TEST=First run import works as intended (see go/chromefirstrun). master_preferences file can still turn off import of history, search engine, and home page.


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

[Miranda Callahan, 2010-07-14 23:34:43]

This is kind of two fixes in one (but note that many of the file changes are
simple file deletions, so this looks bigger than it is), so I've split the
review:

1. Carlos, could you look at the changes to the import sequence, in browser_main
and first_run.h/.cc?  It's mostly moving code around (from OpenFirstRunDialog to
AutoImport), and the change we discussed this morning.

2. George, could you look at the small fix in
first_run_search_engine_view.cc/.h, and keyword_editor_view.cc?  I just removed
an observer that no longer needed to be there.

Thanks!

[cpu, 2010-07-15 17:43:57]

looking good on the parts you asked me to check. Some questions below:

http://codereview.chromium.org/2934011/diff/33001/34003
File chrome/browser/first_run_win.cc (right):

http://codereview.chromium.org/2934011/diff/33001/34003#newcode623
chrome/browser/first_run_win.cc:623: if (!(dont_import_items &
importer::HOME_PAGE) && !homepage_defined)
if ((a) || b) instead?

http://codereview.chromium.org/2934011/diff/33001/34003#newcode647
chrome/browser/first_run_win.cc:647: // Launch the search engine dialog only if
build is organic.
shortcut creation only if organic?

http://codereview.chromium.org/2934011/diff/33001/34003#newcode666
chrome/browser/first_run_win.cc:666: views::AcceleratorHandler
accelerator_handler;
is this accelerator handler mostly a no-op or are we enabling some accelerators
here?

[cpu, 2010-07-15 17:45:07]

Btw, if there is no UI how do we entertain the user while import happens?

On 2010/07/15 17:43:57, cpu wrote:
> looking good on the parts you asked me to check. Some questions below:
> 
> http://codereview.chromium.org/2934011/diff/33001/34003
> File chrome/browser/first_run_win.cc (right):
> 
> http://codereview.chromium.org/2934011/diff/33001/34003#newcode623
> chrome/browser/first_run_win.cc:623: if (!(dont_import_items &
> importer::HOME_PAGE) && !homepage_defined)
> if ((a) || b) instead?
> 
> http://codereview.chromium.org/2934011/diff/33001/34003#newcode647
> chrome/browser/first_run_win.cc:647: // Launch the search engine dialog only
if
> build is organic.
> shortcut creation only if organic?
> 
> http://codereview.chromium.org/2934011/diff/33001/34003#newcode666
> chrome/browser/first_run_win.cc:666: views::AcceleratorHandler
> accelerator_handler;
> is this accelerator handler mostly a no-op or are we enabling some
accelerators
> here?

[GeorgeY, 2010-07-15 18:24:49]

http://codereview.chromium.org/2934011/diff/33001/34003
File chrome/browser/first_run_win.cc (right):

http://codereview.chromium.org/2934011/diff/33001/34003#newcode353
chrome/browser/first_run_win.cc:353:
installer_util::master_preferences::kDistroImportHistoryPref, &value)
indentation is off: here it *could* be 2, but 4 is preferred and used elsewhere
in the code (and in this file, for example @342)

http://codereview.chromium.org/2934011/diff/33001/34003#newcode355
chrome/browser/first_run_win.cc:355: out_prefs->dont_import_items |=
importer::HISTORY;
Here it should be 2 spaces, not 4.

http://codereview.chromium.org/2934011/diff/33001/34003#newcode623
chrome/browser/first_run_win.cc:623: if (!(dont_import_items &
importer::HOME_PAGE) && !homepage_defined)
On 2010/07/15 17:43:58, cpu wrote:
> if ((a) || b) instead?

Didn't you you mean?
(!((a) || b))

http://codereview.chromium.org/2934011/diff/33001/34012
File chrome/browser/views/keyword_editor_view.cc (left):

http://codereview.chromium.org/2934011/diff/33001/34012#oldcode141
chrome/browser/views/keyword_editor_view.cc:141: observer_->SearchEngineChosen(
Wrong indentation - should be 2

http://codereview.chromium.org/2934011/diff/33001/34012#oldcode148
chrome/browser/views/keyword_editor_view.cc:148: if (observer_)
And here.

[Miranda Callahan, 2010-07-15 18:35:25]

Addressed comments, please take another look... Carlos, as to the entertainment
of the user, I discussed this with Glen when I started, because I had the same
question.  He assured me that the user would be sufficiently entertained for the
brief install period watching other things happen on his or her computer.  I'm
still a bit worried about what happens on a slow machine with a large history
import, but -- at least it speeds things up that we're not importing bookmarks
any more, with all the favicons that entails.  

Or perhaps we could show Google PacMan while they wait.

http://codereview.chromium.org/2934011/diff/33001/34003
File chrome/browser/first_run_win.cc (right):

http://codereview.chromium.org/2934011/diff/33001/34003#newcode353
chrome/browser/first_run_win.cc:353:
installer_util::master_preferences::kDistroImportHistoryPref, &value)
On 2010/07/15 18:24:49, GeorgeY wrote:
> indentation is off: here it *could* be 2, but 4 is preferred and used
elsewhere
> in the code (and in this file, for example @342)

Done.

http://codereview.chromium.org/2934011/diff/33001/34003#newcode355
chrome/browser/first_run_win.cc:355: out_prefs->dont_import_items |=
importer::HISTORY;
On 2010/07/15 18:24:49, GeorgeY wrote:
> Here it should be 2 spaces, not 4.

Done.

http://codereview.chromium.org/2934011/diff/33001/34003#newcode623
chrome/browser/first_run_win.cc:623: if (!(dont_import_items &
importer::HOME_PAGE) && !homepage_defined)
On 2010/07/15 17:43:58, cpu wrote:
> if ((a) || b) instead?

Hmm, that is not exactly the same logic; changed it to if (!((a) || b)), which
is perhaps easier to read, despite all the parens?

http://codereview.chromium.org/2934011/diff/33001/34003#newcode647
chrome/browser/first_run_win.cc:647: // Launch the search engine dialog only if
build is organic.
On 2010/07/15 17:43:58, cpu wrote:
> shortcut creation only if organic?

Good point.  Removed from if clause.  A second patch is going to allow
master_preferences creation to stop shortcut creation, but I'm trying to keep
this patch not so evil.

http://codereview.chromium.org/2934011/diff/33001/34003#newcode666
chrome/browser/first_run_win.cc:666: views::AcceleratorHandler
accelerator_handler;
On 2010/07/15 17:43:58, cpu wrote:
> is this accelerator handler mostly a no-op or are we enabling some
accelerators
> here?

Tabs needed to move between choices for the anti-mouse factions...

http://codereview.chromium.org/2934011/diff/33001/34012
File chrome/browser/views/keyword_editor_view.cc (left):

http://codereview.chromium.org/2934011/diff/33001/34012#oldcode141
chrome/browser/views/keyword_editor_view.cc:141: observer_->SearchEngineChosen(
On 2010/07/15 18:24:49, GeorgeY wrote:
> Wrong indentation - should be 2

Indeed, but this code is being deleted, so I'll let it be.  :-)

http://codereview.chromium.org/2934011/diff/33001/34012#oldcode148
chrome/browser/views/keyword_editor_view.cc:148: if (observer_)
On 2010/07/15 18:24:49, GeorgeY wrote:
> And here.

See above.

[cpu, 2010-07-15 19:05:04]

lgtm

On 2010/07/15 18:35:25, Miranda Callahan wrote:
> Addressed comments, please take another look... Carlos, as to the
entertainment
> of the user, I discussed this with Glen when I started, because I had the same
> question.  He assured me that the user would be sufficiently entertained for
the
> brief install period watching other things happen on his or her computer.  I'm
> still a bit worried about what happens on a slow machine with a large history
> import, but -- at least it speeds things up that we're not importing bookmarks
> any more, with all the favicons that entails.  
> 
> Or perhaps we could show Google PacMan while they wait.
> 
> http://codereview.chromium.org/2934011/diff/33001/34003
> File chrome/browser/first_run_win.cc (right):
> 
> http://codereview.chromium.org/2934011/diff/33001/34003#newcode353
> chrome/browser/first_run_win.cc:353:
> installer_util::master_preferences::kDistroImportHistoryPref, &value)
> On 2010/07/15 18:24:49, GeorgeY wrote:
> > indentation is off: here it *could* be 2, but 4 is preferred and used
> elsewhere
> > in the code (and in this file, for example @342)
> 
> Done.
> 
> http://codereview.chromium.org/2934011/diff/33001/34003#newcode355
> chrome/browser/first_run_win.cc:355: out_prefs->dont_import_items |=
> importer::HISTORY;
> On 2010/07/15 18:24:49, GeorgeY wrote:
> > Here it should be 2 spaces, not 4.
> 
> Done.
> 
> http://codereview.chromium.org/2934011/diff/33001/34003#newcode623
> chrome/browser/first_run_win.cc:623: if (!(dont_import_items &
> importer::HOME_PAGE) && !homepage_defined)
> On 2010/07/15 17:43:58, cpu wrote:
> > if ((a) || b) instead?
> 
> Hmm, that is not exactly the same logic; changed it to if (!((a) || b)), which
> is perhaps easier to read, despite all the parens?
> 
> http://codereview.chromium.org/2934011/diff/33001/34003#newcode647
> chrome/browser/first_run_win.cc:647: // Launch the search engine dialog only
if
> build is organic.
> On 2010/07/15 17:43:58, cpu wrote:
> > shortcut creation only if organic?
> 
> Good point.  Removed from if clause.  A second patch is going to allow
> master_preferences creation to stop shortcut creation, but I'm trying to keep
> this patch not so evil.
> 
> http://codereview.chromium.org/2934011/diff/33001/34003#newcode666
> chrome/browser/first_run_win.cc:666: views::AcceleratorHandler
> accelerator_handler;
> On 2010/07/15 17:43:58, cpu wrote:
> > is this accelerator handler mostly a no-op or are we enabling some
> accelerators
> > here?
> 
> Tabs needed to move between choices for the anti-mouse factions...
> 
> http://codereview.chromium.org/2934011/diff/33001/34012
> File chrome/browser/views/keyword_editor_view.cc (left):
> 
> http://codereview.chromium.org/2934011/diff/33001/34012#oldcode141
> chrome/browser/views/keyword_editor_view.cc:141:
observer_->SearchEngineChosen(
> On 2010/07/15 18:24:49, GeorgeY wrote:
> > Wrong indentation - should be 2
> 
> Indeed, but this code is being deleted, so I'll let it be.  :-)
> 
> http://codereview.chromium.org/2934011/diff/33001/34012#oldcode148
> chrome/browser/views/keyword_editor_view.cc:148: if (observer_)
> On 2010/07/15 18:24:49, GeorgeY wrote:
> > And here.
> 
> See above.

[GeorgeY, 2010-07-15 19:37:32]

LGTM

[Miranda Callahan, 2010-07-15 23:20:35]

Carlos, I made a minor change in first_run_win.cc to make sure that
master_preferences could control all prefs in all builds; I'll commit with TBR
by you if you can't get to it today, as it's a minor change.

[cpu, 2010-07-16 17:14:39]

OK