Out-of-process import on Windows.

OOP import on Windows.

Gets rid of the import process on all platforms -- replaced by a utility process which communicates with a ProfileWriter back in the browser process to write to the profile (this uses the ExternalProcessImporterHost machinery written 2+ years ago by mirandac@ for import on Mac and still the state of the art today).

Gets rid of all issues regarding profile contention and races to profile creation between the browser and import processes on first run (example of issues this has previously caused: http://crbug.com/171475, http://crbug.com/174591, http://crbug.com/180459).

Makes bookmarks file import use the same mechanism on all platforms (this means bookmarks file import is now in-process on Linux which still uses ImporterHost (instead of ExternalProcessImporterHost) -- Linux used to use the import process solely for bookmarks file import -- but the work to switch Linux to ExternalProcessImporterHost should be very minimal after this CL and I intend to do it in a follow-up CL to unify the import flow cross-platform once and for all!).

Do not use the out-of-process import for Google Toolbar (this was already the case prior to this CL).
To make the Google Toolbar importer work out-of-process, we would have to augment the import IPC drastically to support the web auth flow required by this importer (it requires to login to import the google.com/bookmarks favorites).

This, as a side-effect, brings silent bookmarks file import from master_preferences to Mac (long standing issue 48880).

Also fixes issue 231710 (or at least removes the condition causing the bug by making the ImportLockDialog go away on first run on Windows -- as should already have been the case).

Also addresses issue 178083 since the early message loop spinning was caused by ImportSettingsWin which was called too early on Windows (actually resulting in running the full import twice on Windows!) -- via PreCreateThreadsImpl()-->ProcessMasterPreferences()-->SetImportPreferencesAndLaunchImport()-->ImportSettingsWin()... This whole flow is removed in this CL :).

This improves first run speed in a debug build from 4901ms to 1477ms, a 332% improvement!!!! (tested by instrumenting a first run browser test, the delta is between the time the test is constructed and the time the test case is called (which happens after the browser has been initialized and import has occurred)).

This supersedes https://codereview.chromium.org/12463030/ (which won't be committed because this fix is so much better).

BUG=219419, 22142, 56816, 178083, 178051, 48880, 232241, 231710, 223462, 87657, 236225
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201837

[gab, 2013-04-18 21:51:18]

Carlos/Miranda, OOP import on Windows is finally here, please take a look!

I extracted as many changes as I could in other CLs
(https://codereview.chromium.org/14322002/,
https://codereview.chromium.org/14316007/,
https://codereview.chromium.org/13954013/, and
https://codereview.chromium.org/14328019/), but I can't break this down much
further.

@tapted: Have a look at the modified first_run_browsertests

@dconnelly, see FYI below :)!

Cheers all!
Gab

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:82: // This class acts as an observer for
the ImporterProgressObserver::ImportEnded
Moved from first_run_internal

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:283: void ImportSettings(Profile* profile,
Moved from first_run_posix.cc (and tweaked some more).

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:94: // TODO(tapted): Make this
reusable.
Done via templates, see below :)!

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_internal.h (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_internal.h:118: bool ImportSettings(Profile*
profile,
Moved to unnamed namespace

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_linux.cc (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_linux.cc:35: // TODO(port): This is just a
piece of the silent import functionality from
Done :)

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_mac.mm (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_mac.mm:19: // http://crbug.com/48880
Fixed as a side-effect of running the same code on all platforms :)

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.cc (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/profiles/p...
chrome/browser/profiles/profile.cc:145: // TODO(dconnelly): revisit this when
crbug.com/22142 (unifying the profile
@dconnelly: FYI :)

[gab, 2013-04-19 17:12:01]

Wow... :)! Updated CL description, but in short: this is a drastic improvement
to first run speed -- can't wait to try this in a release build!

"This improves first run speed in a debug build from 4901ms to 1477ms, a 332%
improvement!!!! (tested by instrumenting a first run browser test, the delta is
between the time the test is constructed and the time the test case is called
(which happens after the browser has been initialized and import has occured)."

Cheers!
Gab

PS: Oh also, it looks like the "profile database is locked" issue
(http://crbug.com/219419) is back on ToT as I was getting the warning again when
testing from ToT... it will be so great to fix this recurring issue once and for
all :)!!!

[dconnelly, 2013-04-22 08:18:56]

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.cc (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/profiles/p...
chrome/browser/profiles/profile.cc:145: // TODO(dconnelly): revisit this when
crbug.com/22142 (unifying the profile
On 2013/04/18 21:51:18, gab wrote:
> @dconnelly: FYI :)

Cool! There's a browser test in
chrome/browser/ui/sync/profile_signin_confirmation_helper_browsertest.cc that
will make sure nothing is broken.

[gab, 2013-04-22 22:32:24]

An extra piece of information to help review this massive change.

Thanks!
Gab

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_win.cc (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_win.cc:440: if (!IsOrganicFirstRun()) {
FYI, the logic being removed here doesn't fully match the one replacing it in
AutoImport/SetImportItem in first_run.cc, but this one was incorrect and the one
already used by all platforms in the original AutoImport method will be assumed
to be correct.

Here is a spreadsheet of what maps to what:
https://docs.google.com/a/google.com/spreadsheet/ccc?key=0Avgot0batQ0TdHdPNU8...

[cpu_(ooo_6.6-7.5), 2013-04-22 23:54:18]

still digesting Cl. Don't we need a reviewer for the profiles part?

[gab, 2013-04-23 02:42:00]

On 2013/04/22 23:54:18, cpu wrote:
> still digesting Cl. Don't we need a reviewer for the profiles part?

Well the profiles part is just removing the IsImportProcess() call as a
side-effect of the import process itself being removed. I felt it would be good
enough to get one of the chrome/OWNERS (sky?) to signoff on that and the other
bits with side-effects like chrome/browser/(ui|extensions) once you and mirandac
signoff on the CL as a whole.

I was also planning to post a PSA on chromium-dev@ after this lands to let
anyone who may have added a hack to work around the import process (which was
more involved than just checking IsImportProcess()) know that they can now
remove that hack.

Let me know if you still feel strongly I should add a reviewer specifically for
profiles?

Thanks!
Gab

[Miranda Callahan, 2013-04-23 06:55:32]

I am still a profiles/* OWNER, and feel comfortable reviewing the relatively
minor changes to profiles here. So I don't think theres any need to get a
special extra reviewer for profiles.

On 2013/04/23 02:42:00, gab wrote:
> On 2013/04/22 23:54:18, cpu wrote:
> > still digesting Cl. Don't we need a reviewer for the profiles part?
> 
> Well the profiles part is just removing the IsImportProcess() call as a
> side-effect of the import process itself being removed. I felt it would be
good
> enough to get one of the chrome/OWNERS (sky?) to signoff on that and the other
> bits with side-effects like chrome/browser/(ui|extensions) once you and
mirandac
> signoff on the CL as a whole.
> 
> I was also planning to post a PSA on chromium-dev@ after this lands to let
> anyone who may have added a hack to work around the import process (which was
> more involved than just checking IsImportProcess()) know that they can now
> remove that hack.
> 
> Let me know if you still feel strongly I should add a reviewer specifically
for
> profiles?
> 
> Thanks!
> Gab

[Miranda Callahan, 2013-04-23 08:15:21]

(s/theres/there's). Also -- I am OOO at a conference in Paris this week,
unfortunately -- will try to get this reviewed by mid-week despite being away.
It's a big, important CL and I don't want to rush it. But after a first pass, my
first impression is that I'm really happy. 
:-D

On 2013/04/23 06:55:32, Miranda Callahan wrote:
> I am still a profiles/* OWNER, and feel comfortable reviewing the relatively
> minor changes to profiles here. So I don't think theres any need to get a
> special extra reviewer for profiles.
> 
> On 2013/04/23 02:42:00, gab wrote:
> > On 2013/04/22 23:54:18, cpu wrote:
> > > still digesting Cl. Don't we need a reviewer for the profiles part?
> > 
> > Well the profiles part is just removing the IsImportProcess() call as a
> > side-effect of the import process itself being removed. I felt it would be
> good
> > enough to get one of the chrome/OWNERS (sky?) to signoff on that and the
other
> > bits with side-effects like chrome/browser/(ui|extensions) once you and
> mirandac
> > signoff on the CL as a whole.
> > 
> > I was also planning to post a PSA on chromium-dev@ after this lands to let
> > anyone who may have added a hack to work around the import process (which
was
> > more involved than just checking IsImportProcess()) know that they can now
> > remove that hack.
> > 
> > Let me know if you still feel strongly I should add a reviewer specifically
> for
> > profiles?
> > 
> > Thanks!
> > Gab

[cpu_(ooo_6.6-7.5), 2013-04-23 23:36:04]

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:115: // ends, this class should quit the
message loop.
remove obvious comment.

[cpu_(ooo_6.6-7.5), 2013-04-23 23:37:21]

lgtm on the CL, I did not review the following

chrome/browser/importer/*
chrome/browser/profiles/*

[tapted, 2013-04-24 08:08:35]

On 2013/04/18 21:51:18, gab wrote:
> @tapted: Have a look at the modified first_run_browsertests

Really awesome work - can't wait for it to land (I've lost so much sleep over
these first run issues..)

I like what you did with the tests - thanks, and these lgtm!

I had a skim through the rest too, but it's mostly a surface-level review with a
few nits. Happy to absorb some more things and do a "deeper" level review, but I
think Miranda will have a better feel for it.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:774: is_first_run = force_first_run ||
first_run::IsChromeFirstRun();;
stray semicolon at end.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:264: scoped_refptr<ImporterHost>
file_importer_host,
nit: indenting

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:426:
installer::master_preferences::kDistroImportBookmarksFromFilePref,
nit: indenting

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:94: // TODO(tapted): Make this
reusable.
On 2013/04/18 21:51:18, gab wrote:
> Done via templates, see below :)!

nice!

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:107: text_.reset(new
std::string(text));
nit: indenting

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:145: extern const char
kImportDefault[] =
shouldn't be extern -- compiler will ignore an extern storage class specifier if
you also initialize it. Maybe static? or move into an anonymous namespace. (more
below).

gcc actually emits a warning about this, but only when compiling for C (e.g. the
file is ".c" not ".cc") - weird.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:161: 
nit: extra vertical whitespace

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:198: // The file doesn't
actually need to exist for this integration test to trigger
Could it be picking up '/Library/Google Chrome Master Preferences' in the corp
image instead when the file doesn't exist?

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_bridge.cc:50: //
|bookmars_left| is required for the checks below as Windows has a
typo: bookmars -> bookmarks

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:228:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
heads up: I just saw a change land that got rid of the
ALLOW_THIS_IN_INITIALIZER_LIST macro, so you might get a conflict here

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_factory.cc (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_factory.cc:42: // Do not start sync on
the import process.
Could these checks still be required for Linux?

[gab, 2013-04-24 14:48:15]

FYI, I merged in what should be the final version of all the extracted CLs (and
other conflicting CLs from trunk) for my sanity, feel free to ignore the merge
and keep reviewing patch set 15.

Thanks!
Gab

[Miranda Callahan, 2013-04-24 15:16:40]

LGTM with some nits. Great work.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:76: int g_auto_import_state =
first_run::AUTO_IMPORT_NONE;
should this be uint16 if it's a bitfield?

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
chrome/browser/first_run/first_run.cc:809: int GetAutoImportState() {
This should probably just be called "g_auto_import_state()" to match Google
getter style.

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
File chrome/browser/first_run/first_run_win.cc (left):

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
chrome/browser/first_run/first_run_win.cc:30: #include
"chrome/browser/importer/importer_list.h"
can remove these two importer includes.

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
chrome/browser/first_run/first_run_win.cc:39: #include
"chrome/common/worker_thread_ticker.h"
can remove this include.

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/importer/...
File chrome/browser/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/importer/...
chrome/browser/importer/external_process_importer_bridge.cc:50: //
|bookmars_left| is required for the checks below as Windows has a
nit: lost a k on mars.

[gab, 2013-04-24 20:24:58]

@thakis for OWNER approval on side-effects not covered by current reviewers,
i.e.:

chrome\browser\chrome_browser_main.cc
chrome\browser\extensions\extension_service.cc
chrome\browser\extensions\extension_system.cc
chrome\browser\policy\cloud\user_policy_signin_service.cc
chrome\browser\sync\profile_sync_service_factory.cc
chrome\browser\ui\sync\profile_signin_confirmation_helper_browsertest.cc
chrome\browser\ui\webui\options\import_data_handler.cc
chrome\common\switch_utils.cc
chrome\common\switch_utils_unittest.cc

and for chrome\utility\profile_import_handler.cc which is more than just a
side-effect.

Thanks all!
Gab

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:774: is_first_run = force_first_run ||
first_run::IsChromeFirstRun();;
On 2013/04/24 08:08:35, tapted wrote:
> stray semicolon at end.

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:76: int g_auto_import_state =
first_run::AUTO_IMPORT_NONE;
On 2013/04/24 15:16:40, Miranda Callahan wrote:
> should this be uint16 if it's a bitfield?

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:115: // ends, this class should quit the
message loop.
On 2013/04/23 23:36:04, cpu wrote:
> remove obvious comment.

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:264: scoped_refptr<ImporterHost>
file_importer_host,
On 2013/04/24 08:08:35, tapted wrote:
> nit: indenting

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:426:
installer::master_preferences::kDistroImportBookmarksFromFilePref,
On 2013/04/24 08:08:35, tapted wrote:
> nit: indenting

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:107: text_.reset(new
std::string(text));
On 2013/04/24 08:08:35, tapted wrote:
> nit: indenting

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:145: extern const char
kImportDefault[] =
On 2013/04/24 08:08:35, tapted wrote:
> shouldn't be extern -- compiler will ignore an extern storage class specifier
if
> you also initialize it. Maybe static? or move into an anonymous namespace.
(more
> below).
> 
> gcc actually emits a warning about this, but only when compiling for C (e.g.
the
> file is ".c" not ".cc") - weird.

If I don't make it extern, I get:
error C2970: '`anonymous-namespace'::FirstRunMasterPrefsBrowserTestT' : template
parameter 'Text' : 'kImportDefault' : an expression involving objects with
internal linkage cannot be used as a non-type argument

This is already in an anonymous namespace.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:161: 
On 2013/04/24 08:08:35, tapted wrote:
> nit: extra vertical whitespace

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:198: // The file doesn't
actually need to exist for this integration test to trigger
On 2013/04/24 08:08:35, tapted wrote:
> Could it be picking up '/Library/Google Chrome Master Preferences' in the corp
> image instead when the file doesn't exist?

This comment belonged with the ImportBookmarksFile test above, the "file"
mentioned is the bookmarks file (clarified comment), the master pref file itself
will exist.
This test is only testing the fact that the file import was launched, not
whether it succeeded, so that's ok. Makes sense?

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/importer/e...
File chrome/browser/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/importer/e...
chrome/browser/importer/external_process_importer_bridge.cc:50: //
|bookmars_left| is required for the checks below as Windows has a
On 2013/04/24 08:08:35, tapted wrote:
> typo: bookmars -> bookmarks

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:228:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
On 2013/04/24 08:08:35, tapted wrote:
> heads up: I just saw a change land that got rid of the
> ALLOW_THIS_IN_INITIALIZER_LIST macro, so you might get a conflict here

Ok thanks.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_factory.cc (left):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_factory.cc:42: // Do not start sync on
the import process.
On 2013/04/24 08:08:35, tapted wrote:
> Could these checks still be required for Linux?

No, I got rid of the import process entirely, on Linux as a side-effect the file
import is now in process (it was the only thing using the import process on
Linux).

I will port Linux over to OOP import in a follow-up CL anyways.

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
chrome/browser/first_run/first_run.cc:809: int GetAutoImportState() {
On 2013/04/24 15:16:40, Miranda Callahan wrote:
> This should probably just be called "g_auto_import_state()" to match Google
> getter style.

How about auto_import_state() to:
1) Not use the same name as the variable.
2) Not care about the g_ prefix, just like we don't add the _ suffix in class
member getters.
?

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
File chrome/browser/first_run/first_run_win.cc (left):

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
chrome/browser/first_run/first_run_win.cc:30: #include
"chrome/browser/importer/importer_list.h"
On 2013/04/24 15:16:40, Miranda Callahan wrote:
> can remove these two importer includes.

Nice catch, forgot to go over this file's includes looks like.

Fixed a bunch of other includes that weren't required anymore and added some
that were missing, the C++ include model is so broken!

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
chrome/browser/first_run/first_run_win.cc:39: #include
"chrome/common/worker_thread_ticker.h"
On 2013/04/24 15:16:40, Miranda Callahan wrote:
> can remove this include.

Done.

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/importer/...
File chrome/browser/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/importer/...
chrome/browser/importer/external_process_importer_bridge.cc:50: //
|bookmars_left| is required for the checks below as Windows has a
On 2013/04/24 15:16:40, Miranda Callahan wrote:
> nit: lost a k on mars.

:), done.

[Nico, 2013-04-24 22:53:47]

lgtm!

[tapted, 2013-04-25 00:02:42]

slgtm - thanks for clarifying

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:133: }
}  // namespace

(probably my bad from an earlier CL).

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:145: extern const char
kImportDefault[] =
On 2013/04/24 20:24:59, gab wrote:
> On 2013/04/24 08:08:35, tapted wrote:
> > shouldn't be extern -- compiler will ignore an extern storage class
specifier
> if
> > you also initialize it. Maybe static? or move into an anonymous namespace.
> (more
> > below).
> > 
> > gcc actually emits a warning about this, but only when compiling for C (e.g.
> the
> > file is ".c" not ".cc") - weird.
> 
> If I don't make it extern, I get:
> error C2970: '`anonymous-namespace'::FirstRunMasterPrefsBrowserTestT' :
template
> parameter 'Text' : 'kImportDefault' : an expression involving objects with
> internal linkage cannot be used as a non-type argument
> 
> This is already in an anonymous namespace.

Ah, drat. Where did I read that ~"static and anonymous namespaces are the same
except you can use things in anonymous namespaces for template arguments". This
seems to destroy that - neither gcc nor visual studio let this happen for me.

extern seems to be the workaround - good work on figuring that out! And it looks
like compilers in C++ mode are happy with that being on the definition.
Otherwise you could separate an extern declaration with a non-extern definition.

But I think the template type is in an anonymous namespace but not the string
constants? For me, it still works if the definition is in an anonymous
namespace, with the extern specifier. That might be an improvement (but probably
more of a nit ;).

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:198: // The file doesn't
actually need to exist for this integration test to trigger
On 2013/04/24 20:24:59, gab wrote:
> On 2013/04/24 08:08:35, tapted wrote:
> > Could it be picking up '/Library/Google Chrome Master Preferences' in the
corp
> > image instead when the file doesn't exist?
> 
> This comment belonged with the ImportBookmarksFile test above, the "file"
> mentioned is the bookmarks file (clarified comment), the master pref file
itself
> will exist.
> This test is only testing the fact that the file import was launched, not
> whether it succeeded, so that's ok. Makes sense?

Ah. Cool.

[Miranda Callahan, 2013-04-25 11:37:05]

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12670013/diff/113001/chrome/browser/first_run...
chrome/browser/first_run/first_run.cc:809: int GetAutoImportState() {
yes, that's even better. :-)

On 2013/04/24 20:24:59, gab wrote:
> On 2013/04/24 15:16:40, Miranda Callahan wrote:
> > This should probably just be called "g_auto_import_state()" to match Google
> > getter style.
> 
> How about auto_import_state() to:
> 1) Not use the same name as the variable.
> 2) Not care about the g_ prefix, just like we don't add the _ suffix in class
> member getters.
> ?

[gab, 2013-04-25 18:49:59]

Done, trying to find a solution to the failing IEImporterTest, the problem is
that they overwrite the registry, but the overwrite is only local to the test
process (so now that the importer runs in another process, it doesn't see the
override...).

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:133: }
On 2013/04/25 00:02:42, tapted wrote:
> }  // namespace
> 
> (probably my bad from an earlier CL).

Done.

https://codereview.chromium.org/12670013/diff/99003/chrome/browser/first_run/...
chrome/browser/first_run/first_run_browsertest.cc:145: extern const char
kImportDefault[] =
On 2013/04/25 00:02:42, tapted wrote:
> On 2013/04/24 20:24:59, gab wrote:
> > On 2013/04/24 08:08:35, tapted wrote:
> > > shouldn't be extern -- compiler will ignore an extern storage class
> specifier
> > if
> > > you also initialize it. Maybe static? or move into an anonymous namespace.
> > (more
> > > below).
> > > 
> > > gcc actually emits a warning about this, but only when compiling for C
(e.g.
> > the
> > > file is ".c" not ".cc") - weird.
> > 
> > If I don't make it extern, I get:
> > error C2970: '`anonymous-namespace'::FirstRunMasterPrefsBrowserTestT' :
> template
> > parameter 'Text' : 'kImportDefault' : an expression involving objects with
> > internal linkage cannot be used as a non-type argument
> > 
> > This is already in an anonymous namespace.
> 
> Ah, drat. Where did I read that ~"static and anonymous namespaces are the same
> except you can use things in anonymous namespaces for template arguments".
This
> seems to destroy that - neither gcc nor visual studio let this happen for me.
> 
> extern seems to be the workaround - good work on figuring that out! And it
looks
> like compilers in C++ mode are happy with that being on the definition.
> Otherwise you could separate an extern declaration with a non-extern
definition.
> 
> But I think the template type is in an anonymous namespace but not the string
> constants? For me, it still works if the definition is in an anonymous
> namespace, with the extern specifier. That might be an improvement (but
probably
> more of a nit ;).

Feel free to try different things after this lands as part of the work to
re-enable for Linux/MacASAN, for now I think, as you seem to agree, that this is
fine.

[gab, 2013-04-26 17:41:21]

Fixed the IE tests, the fix was fairly involved
(https://codereview.chromium.org/14143010/) so I kept it as a separate CL since
this CL is already complex enough.

Cheers!
Gab

PS: Not sure why, but this CL was somehow closed yesterday, no worries it's not
going away :)!

[gab, 2013-04-29 02:56:38]

@thakis: PTAL at chrome/browser/ui/webui/options/import_data_handler.cc.

Modified chrome/browser/ui/webui/options/import_data_handler.cc to keep the
Google Toolbar import in-process.

As I added to the CL description:
"Do not use the out-of-process import for Google Toolbar (this was already the
case prior to this CL).
To make the Google Toolbar importer work out-of-process, we would have to
augment the import IPC drastically to support the web auth flow required by this
importer (it requires to login to import the google.com/bookmarks favorites)."

Thanks!
Gab

[Nico, 2013-04-29 03:24:54]

still lgtm

https://codereview.chromium.org/12670013/diff/170004/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/12670013/diff/170004/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/import_data_handler.cc:115: // The Google
Toolbar import doesn't work for the out-of-process import.
Maybe call out that Google Toolbar import never runs at startup / first run.

The sync folks probably have thought about importing google bookmarks, so maybe
doing this toolbar import server-side might make sense long term. Ask them what
they think.

[gab, 2013-04-29 12:41:53]

Thanks

https://codereview.chromium.org/12670013/diff/170004/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/12670013/diff/170004/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/import_data_handler.cc:115: // The Google
Toolbar import doesn't work for the out-of-process import.
On 2013/04/29 03:24:54, Nico wrote:
> Maybe call out that Google Toolbar import never runs at startup / first run.

Done.

> 
> The sync folks probably have thought about importing google bookmarks, so
maybe
> doing this toolbar import server-side might make sense long term. Ask them
what
> they think.

Yes, I already talked about this with them F2F and on http://crbug.com/219419.
It would probably make sense for this to happen automatically when signing in to
Chrome for the first time. Keeping the toolbar importer as part of the import
code will soon become a big platform-specific/not well tested/unQAed(AFAIK) code
burden as this will become the only code that needs the in-process import flow.
(Also, it's Windows only because it's Toolbar I guess, but technically the
bookmarks are in the cloud and could be synced to any OS...).

Currently gathering stats about which importer gets used from each entry point,
we'll soon make a move based on that (http://crbug.com/234354).

[commit-bot: I haz the power, 2013-05-23 13:43:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/12670013/295001

[commit-bot: I haz the power, 2013-05-23 18:49:51]

Change committed as 201837