Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1050)

Issue 12463030: Do not do AutoImport on Windows since the import process is already ran earlier as part of ProcessM… (Closed)

Created:
7 years, 9 months ago by gab
Modified:
7 years, 8 months ago
CC:
chromium-reviews, sail+watch_chromium.org, grt (UTC plus 2)
Visibility:
Public.

Description

Do not do AutoImport on Windows since the import process is already ran earlier as part of ProcessMasterPreferences. It has to happen earlier (i.e. in PreProfileInit) because the import process interacts with the profile (and we thus want to launch it before profile creation, but after the process singleton has been created). The reason this needs to happen earlier on Windows is that the import process is completely independent and creates/interacts with the profile itself... on Linux the import is in-process and on Mac we use the out-of-process import with a utility process that talks with the browser process over IPC (but the browser process is still the one doing the interaction with the profile); hence in both cases (Linux/Mac) it makes sense to do the AutoImport after the profile has been initialized, but not on Windows the way the import is currently designed. The best solution is to use the OOP import on Windows as well, but this is a more involved task (http://crbug.com/22142) and should be done in a follow-up CL (the OOP import is actually not very platform-specific as it stands, but it will need to be tweaked to support IE, etc.). This still has the issue that it's done before the process singleton is grabbed (and thus could potentially be launched more than once...). BUG=219419 TEST=Import process is launched only once whether there is a master_preferences file or not.

Patch Set 1 #

Total comments: 14

Patch Set 2 : adressed joao's comments #

Patch Set 3 : Move import task from ProcessMasterPreferences() to PreProfileInit(). #

Total comments: 11

Patch Set 4 : fix posix compile #

Unified diffs Side-by-side diffs Delta from patch set Stats (+205 lines, -219 lines) Patch
M chrome/browser/chrome_browser_main.cc View 1 2 4 chunks +24 lines, -4 lines 0 comments Download
M chrome/browser/first_run/first_run.h View 1 2 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/first_run/first_run.cc View 1 2 14 chunks +112 lines, -49 lines 0 comments Download
M chrome/browser/first_run/first_run_internal.h View 1 2 3 chunks +15 lines, -18 lines 0 comments Download
M chrome/browser/first_run/first_run_posix.cc View 1 2 3 4 chunks +11 lines, -92 lines 0 comments Download
M chrome/browser/first_run/first_run_win.cc View 1 2 4 chunks +36 lines, -56 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
gab
I think this should fix the current issues seen with the import process. I'm leaving ...
7 years, 9 months ago (2013-03-08 21:55:19 UTC) #1
gab
Oh, and FWIW, I'm running out the door, didn't have time to re-read the code...it ...
7 years, 9 months ago (2013-03-08 21:57:08 UTC) #2
gab
+atwilson :) Cheers! Gab
7 years, 9 months ago (2013-03-08 21:59:42 UTC) #3
cpu_(ooo_6.6-7.5)
Now the nested message loop of current code is not problematic because the singleton is ...
7 years, 9 months ago (2013-03-11 20:54:20 UTC) #4
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/12463030/diff/1/chrome/browser/first_run/first_run.cc File chrome/browser/first_run/first_run.cc (left): https://codereview.chromium.org/12463030/diff/1/chrome/browser/first_run/first_run.cc#oldcode603 chrome/browser/first_run/first_run.cc:603: internal::SetRLZPref(out_prefs, install_prefs.get()); where did the rlz stuff go?
7 years, 9 months ago (2013-03-11 20:56:05 UTC) #5
gab
https://codereview.chromium.org/12463030/diff/1/chrome/browser/first_run/first_run.cc File chrome/browser/first_run/first_run.cc (left): https://codereview.chromium.org/12463030/diff/1/chrome/browser/first_run/first_run.cc#oldcode603 chrome/browser/first_run/first_run.cc:603: internal::SetRLZPref(out_prefs, install_prefs.get()); On 2013/03/11 20:56:05, cpu wrote: > where ...
7 years, 9 months ago (2013-03-12 12:32:19 UTC) #6
gab
On 2013/03/11 20:54:20, cpu wrote: > Now the nested message loop of current code is ...
7 years, 9 months ago (2013-03-12 12:35:00 UTC) #7
Joao da Silva
I don't see any problems in this CL but I'm not terribly familiar with this ...
7 years, 9 months ago (2013-03-12 13:32:05 UTC) #8
cpu_(ooo_6.6-7.5)
I guess a blocking wait would do. Since the singleton window has been created and ...
7 years, 9 months ago (2013-03-13 22:47:41 UTC) #9
gab
Adressed Joao's comments. https://codereview.chromium.org/12463030/diff/1/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/12463030/diff/1/chrome/browser/chrome_browser_main.cc#newcode1350 chrome/browser/chrome_browser_main.cc:1350: // like http://crbug.com/180459 and http://crbug.com/171475). On ...
7 years, 9 months ago (2013-03-25 16:00:43 UTC) #10
gab
On 2013/03/13 22:47:41, cpu wrote: > I guess a blocking wait would do. Since the ...
7 years, 9 months ago (2013-03-25 16:18:25 UTC) #11
cpu_(ooo_6.6-7.5)
Indeed. Once upon a time the process singleton was grabbed before starting the import. In ...
7 years, 9 months ago (2013-03-25 21:45:20 UTC) #12
gab
On 2013/03/25 21:45:20, cpu wrote: > Indeed. Once upon a time the process singleton was ...
7 years, 9 months ago (2013-03-25 22:55:02 UTC) #13
gab
So here it is, I moved the early import step that was part of ProcessMasterPreferences() ...
7 years, 9 months ago (2013-03-26 15:55:25 UTC) #14
gab
@cpu: friendly ping. I added multiple comments to patch set 3 to make it easier ...
7 years, 8 months ago (2013-04-02 15:22:58 UTC) #15
gab
7 years, 8 months ago (2013-04-03 20:00:03 UTC) #16
I am about to put out a CL to do out-of-process import (as on Mac) on Windows; I
don't think committing this first simplifies the upcoming CL which gets rid of
the idef this CL was introducing and goes in a completely different direction
(deleting some of the code introduced in this CL in the process).

I will thus close this CL for now to avoid wasting anyone's time.

Cheers!
Gab

Powered by Google App Engine
This is Rietveld 408576698