Modifies the Firefox import behavior such that if we're in first run, the imp...

Modifies the Firefox import behavior such that if we're in first run, the importer is headless, and we're only importing the home page, skip the Firefox lock.
Otherwise, the process would silently wait for Firefox to close with no warnings.

BUG=18709
TEST=set "import_home_page" : true, "skip_first_run_ui" : true in the master_preferences, and run without FirstRun.  Import should not block.


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

[Ben Goodger (Google), 2009-08-21 18:40:37]

I'm really busy and haven't had a chance to study this. I know Wan-teh looked at
the Firefox import stuff a while ago and knows about the locking/unlocking etc.

[Glenn Wilson, 2009-08-21 22:13:37]

On 2009/08/21 18:40:37, Ben Goodger wrote:
> I'm really busy and haven't had a chance to study this. I know Wan-teh looked
at
> the Firefox import stuff a while ago and knows about the locking/unlocking
etc.

Wan-Teh, we talked about locking and importing the home page earlier this week. 
Does this look ok to you?

[Glenn Wilson, 2009-08-25 21:10:18]

wtc: What do you think?

[Glenn Wilson, 2009-08-31 20:43:03]

On 2009/08/25 21:10:18, gwilson wrote:
> wtc: What do you think?

Rahul, does this look like the right approach to you?

[kuchhal, 2009-08-31 20:49:59]

http://codereview.chromium.org/174057/diff/1/2
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/174057/diff/1/2#newcode549
Line 549: }
Shouldn't this go inside the if (!firefox_lock_->HasAcquired()) {} block?

[wtc, 2009-09-02 00:03:26]

LGTM, with the change Rahul suggested.

http://codereview.chromium.org/174057/diff/1/2
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/174057/diff/1/2#newcode545
Line 545: if ((items == HOME_PAGE) && first_run && this->headless_) {
Please add a comment.  The intention is not obvious at all.

[Glenn Wilson, 2009-09-02 21:39:48]

Thanks for the review!

http://codereview.chromium.org/174057/diff/1/2
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/174057/diff/1/2#newcode545
Line 545: if ((items == HOME_PAGE) && first_run && this->headless_) {
On 2009/09/02 00:03:27, wtc wrote:
> Please add a comment.  The intention is not obvious at all.
Done.

http://codereview.chromium.org/174057/diff/1/2#newcode549
Line 549: }
On 2009/08/31 20:49:59, kuchhal wrote:
> Shouldn't this go inside the if (!firefox_lock_->HasAcquired()) {} block?
Yes, you're correct -- we only want to escape the lock if we need the lock in
the first place (the previous version would fire even if Firefox wasn't open)

[wtc, 2009-09-02 22:09:04]

LGTM!

http://codereview.chromium.org/174057/diff/6003/5002
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/174057/diff/6003/5002#newcode554
Line 554: if ((items == HOME_PAGE) && first_run && this->headless_) {
Another way to write this code, which may be better, is:

  if (!((items == HOME_PAGE) && first_run && this->headless_)) {
    is_source_readable_ = false;
    ShowWarningDialog();
  }

This way we won't repeat the AddRef() and InvokeTaskIfDone code.

But we'll need to verify that we won't run the google
toolbar code below.  So perhaps your version is still
better.

[kuchhal, 2009-09-02 22:14:00]

lgtm.