First cut at Firefox import on OSX + stubs for Safari import.

First cut at Firefox import on OSX + stubs for Safari import.

Known limitations:
* Runs in browser process, should run in a separate process.
* No UI.
* No FF password import.

BUG=15455
TEST=Check that firefox settings are correctly imported on first run, firefox password importing and Safari importing still don't work.

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

[stuartmorgan, 2009-07-29 20:04:05]

http://codereview.chromium.org/160341/diff/16/21
File chrome/browser/first_run_mac.mm (right):

http://codereview.chromium.org/160341/diff/16/21#newcode54
Line 54: // On OS X, we should always at least have Safari.
What if the user ran some kind of app-cleaner that deletes their Safari
directory? Or what if Chrome is the first browser they ever launch (in a new
account on a machine with Chrome installed)?

http://codereview.chromium.org/160341/diff/16/21#newcode94
Line 94: importer_host->GetSourceProfileInfoAt(
Fix indentation.

http://codereview.chromium.org/160341/diff/16/23
File chrome/browser/importer/firefox_importer_utils_mac.mm (right):

http://codereview.chromium.org/160341/diff/16/23#newcode22
Line 22: if (profiles_ini) {
How would this be nil?

http://codereview.chromium.org/160341/diff/16/24
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/160341/diff/16/24#newcode824
Line 824: IDS_IMPORT_FROM_SAFARI);
Fix indentation

http://codereview.chromium.org/160341/diff/16/26
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/160341/diff/16/26#newcode824
Line 824: // On OSX, Safari should always come first in the importer list.
Same comments here as for the assertion; seems fragile to have a unit test that
depends on a non-permanent part of the filesystem existing on the machine that
runs the test.

[jeremy, 2009-07-29 20:14:27]

All fixed, ready for another look.

http://codereview.chromium.org/160341/diff/16/21
File chrome/browser/first_run_mac.mm (right):

http://codereview.chromium.org/160341/diff/16/21#newcode54
Line 54: // On OS X, we should always at least have Safari.
Removed DCHECK, added TODO.

http://codereview.chromium.org/160341/diff/16/21#newcode94
Line 94: importer_host->GetSourceProfileInfoAt(
On 2009/07/29 20:04:06, stuartmorgan wrote:
> Fix indentation.

Done.

http://codereview.chromium.org/160341/diff/16/23
File chrome/browser/importer/firefox_importer_utils_mac.mm (right):

http://codereview.chromium.org/160341/diff/16/23#newcode22
Line 22: if (profiles_ini) {
It can't at present, I'm being defensive.

http://codereview.chromium.org/160341/diff/16/24
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/160341/diff/16/24#newcode824
Line 824: IDS_IMPORT_FROM_SAFARI);
On 2009/07/29 20:04:06, stuartmorgan wrote:
> Fix indentation

Done.

http://codereview.chromium.org/160341/diff/16/26
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/160341/diff/16/26#newcode824
Line 824: // On OSX, Safari should always come first in the importer list.
Removed this test.

[Mark Mentovai, 2009-07-29 20:16:28]

stuartmorgan@chromium.org wrote:
> http://codereview.chromium.org/160341/diff/16/26#newcode824
> Line 824: // On OSX, Safari should always come first in the importer
> list.
> Same comments here as for the assertion; seems fragile to have a unit
> test that depends on a non-permanent part of the filesystem existing on
> the machine that runs the test.

For that matter, most of our buildbots will never have launched
Safari, or any other browser.

Mark

[stuartmorgan, 2009-07-29 20:22:37]

LGTM

http://codereview.chromium.org/160341/diff/1017/51
File chrome/browser/cocoa/first_run_dialog.mm (right):

http://codereview.chromium.org/160341/diff/1017/51#newcode32
Line 32: [super dealloc];
Oops missed this before; indentation is off.

http://codereview.chromium.org/160341/diff/1017/56
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/160341/diff/1017/56#newcode653
Line 653: case SAFARI:
Should this have platform ifdef-ing, since MS_IE has it?

http://codereview.chromium.org/160341/diff/1017/57
File chrome/browser/importer/importer.h (right):

http://codereview.chromium.org/160341/diff/1017/57#newcode39
Line 39: SAFARI,
ifdef-ing here too?

[kuchhal, 2009-07-29 20:28:09]

lgtm for *.h and *.cc files.

and +1 to stuart's comments of using ifdef for Safari everywhere if we only plan
to support it on Mac.

http://codereview.chromium.org/160341/diff/1017/52
File chrome/browser/first_run_mac.mm (right):

http://codereview.chromium.org/160341/diff/1017/52#newcode94
Line 94: // Utility class that does the actual import.
Not sure if this comment makes sense. ProfileInfo is only responsible for
writing to Chrome profile. The whole import process is more involved than that.

http://codereview.chromium.org/160341/diff/1017/52#newcode101
Line 101: new ProfileWriter(profile), true);
reduce indentation by one space.