Adding a new PyAuto hook for importing settings.

Adding a new PyAuto hook for importing settings. 

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

[Nirnimesh, 2010-07-13 00:12:11]

http://codereview.chromium.org/2923010/diff/2001/3001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2363
chrome/browser/automation/automation_provider.cc:2363: // Map from the json
string passed over to the browser type
nit: End with '.'

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2376
chrome/browser/automation/automation_provider.cc:2376: // Map from the json
string passed over to the import item masks
nit: End comment with '.'

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2386
chrome/browser/automation/automation_provider.cc:2386: 
too many blank lines

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2426
chrome/browser/automation/automation_provider.cc:2426: profile_info, prof,
import_items, new ProfileWriter(prof), first_run);
Does the ProfileWriter object get deleted?

http://codereview.chromium.org/2923010/diff/2001/3005
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/2923010/diff/2001/3005#newcode769
chrome/test/pyautolib/pyauto.py:769: MS_IE (only on Windows), FIREFOX2,
FIREFOX3,
Why should the user have to choose between Firefox2 & Firefox3? The first-run UI
just prompts for Firefox

http://codereview.chromium.org/2923010/diff/2001/3005#newcode772
chrome/test/pyautolib/pyauto.py:772: first_run: A boolean indicating whether
this is the first run of
what is this for?

[Paweł Hajdan Jr., 2010-07-13 03:39:28]

Drive-by with some minor automation comments.

http://codereview.chromium.org/2923010/diff/2001/3001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2401
chrome/browser/automation/automation_provider.cc:2401: if
(string_to_import_item.find(item) == string_to_import_item.end()) {
nit: Could you use ContainsKey from base/stl_util-inl.h?

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2410
chrome/browser/automation/automation_provider.cc:2410: if
(string_to_browser_type.find(browser_type) ==
ContainsKey please.

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2430
chrome/browser/automation/automation_provider.cc:2430: json_return =
Could you put the error case first and return early to avoid increasing the
indentation level?

http://codereview.chromium.org/2923010/diff/2001/3004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2923010/diff/2001/3004#newcode619
chrome/browser/automation/automation_provider_observers.h:619: public:
nit: uh-oh, the indentation seems messed up. This should be indented just one
space, and below just two spaces.

http://codereview.chromium.org/2923010/diff/2001/3004#newcode623
chrome/browser/automation/automation_provider_observers.h:623: provider_ =
provider;
nit: Please use initializer list syntax.

[Alyssa, 2010-07-13 21:55:40]

http://codereview.chromium.org/2923010/diff/2001/3001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2363
chrome/browser/automation/automation_provider.cc:2363: // Map from the json
string passed over to the browser type
On 2010/07/13 00:12:11, Nirnimesh wrote:
> nit: End with '.'

Done.

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2376
chrome/browser/automation/automation_provider.cc:2376: // Map from the json
string passed over to the import item masks
On 2010/07/13 00:12:11, Nirnimesh wrote:
> nit: End comment with '.'

Done.

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2386
chrome/browser/automation/automation_provider.cc:2386: 
On 2010/07/13 00:12:11, Nirnimesh wrote:
> too many blank lines

Done.

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2401
chrome/browser/automation/automation_provider.cc:2401: if
(string_to_import_item.find(item) == string_to_import_item.end()) {
On 2010/07/13 03:39:29, Paweł Hajdan Jr. wrote:
> nit: Could you use ContainsKey from base/stl_util-inl.h?

Done.

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2410
chrome/browser/automation/automation_provider.cc:2410: if
(string_to_browser_type.find(browser_type) ==
On 2010/07/13 03:39:29, Paweł Hajdan Jr. wrote:
> ContainsKey please.

Done.

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2426
chrome/browser/automation/automation_provider.cc:2426: profile_info, prof,
import_items, new ProfileWriter(prof), first_run);
On 2010/07/13 00:12:11, Nirnimesh wrote:
> Does the ProfileWriter object get deleted?

I assume so because all other calls to the StartImportSettings function look
like this (just making a ProfileWriter w/o deleting it) and its destructor is
protected. Is there  a good way to check how it gets deleted?

http://codereview.chromium.org/2923010/diff/2001/3001#newcode2430
chrome/browser/automation/automation_provider.cc:2430: json_return =
On 2010/07/13 03:39:29, Paweł Hajdan Jr. wrote:
> Could you put the error case first and return early to avoid increasing the
> indentation level?

Done. And the redundancy will be refactored with Nirnimesh's pending CL.

http://codereview.chromium.org/2923010/diff/2001/3004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2923010/diff/2001/3004#newcode619
chrome/browser/automation/automation_provider_observers.h:619: public:
On 2010/07/13 03:39:29, Paweł Hajdan Jr. wrote:
> nit: uh-oh, the indentation seems messed up. This should be indented just one
> space, and below just two spaces.

Done.

http://codereview.chromium.org/2923010/diff/2001/3004#newcode623
chrome/browser/automation/automation_provider_observers.h:623: provider_ =
provider;
On 2010/07/13 03:39:29, Paweł Hajdan Jr. wrote:
> nit: Please use initializer list syntax.

Done.

http://codereview.chromium.org/2923010/diff/2001/3005
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/2923010/diff/2001/3005#newcode769
chrome/test/pyautolib/pyauto.py:769: MS_IE (only on Windows), FIREFOX2,
FIREFOX3,
On 2010/07/13 00:12:11, Nirnimesh wrote:
> Why should the user have to choose between Firefox2 & Firefox3? The first-run
UI
> just prompts for Firefox

It looks like something happens before the choices are shown to the user to
determine which browsers are installed. Should I figure out a way to make the
call from higher up so that the tests don't have to specify the browser? I
haven't seen a simple way yet, but that doesn't mean it doesn't exist.

It seemed like this was an easy way to test with different browsers, but I could
see how a more end-to-end test would be helpful too.

http://codereview.chromium.org/2923010/diff/2001/3005#newcode772
chrome/test/pyautolib/pyauto.py:772: first_run: A boolean indicating whether
this is the first run of
On 2010/07/13 00:12:11, Nirnimesh wrote:
> what is this for?
It's a parameter to the function I'm calling. It's because you can import
settings at any time (under Options->Personal Stuff), or it automatically
prompts at the first run. It affects things like whether favorites get imported
to the bookmark bar.

Should I just assume it is a first run? Or provide better documentation?

[Nirnimesh, 2010-07-13 22:05:40]

http://codereview.chromium.org/2923010/diff/2001/3005
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/2923010/diff/2001/3005#newcode769
chrome/test/pyautolib/pyauto.py:769: MS_IE (only on Windows), FIREFOX2,
FIREFOX3,
On 2010/07/13 21:55:40, Alyssa wrote:
> On 2010/07/13 00:12:11, Nirnimesh wrote:
> > Why should the user have to choose between Firefox2 & Firefox3? The
first-run
> UI
> > just prompts for Firefox
> 
> It looks like something happens before the choices are shown to the user to
> determine which browsers are installed. Should I figure out a way to make the
> call from higher up so that the tests don't have to specify the browser?

I think it's worth figuring out.

http://codereview.chromium.org/2923010/diff/2001/3005#newcode772
chrome/test/pyautolib/pyauto.py:772: first_run: A boolean indicating whether
this is the first run of
On 2010/07/13 21:55:40, Alyssa wrote:
> On 2010/07/13 00:12:11, Nirnimesh wrote:
> > what is this for?
> It's a parameter to the function I'm calling. It's because you can import
> settings at any time (under Options->Personal Stuff), or it automatically
> prompts at the first run. It affects things like whether favorites get
imported
> to the bookmark bar.

Document this.

> 
> Should I just assume it is a first run? Or provide better documentation?

[Alyssa, 2010-07-14 00:44:18]

Per our discussion, I added another method to get the available browsers for
import.

http://codereview.chromium.org/2923010/diff/7002/18001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2923010/diff/7002/18001#newcode2440
chrome/browser/automation/automation_provider.cc:2440: // If we made it to the
end of the loop, then the input was bad.
I know there must be a better way to do this, but I couldn't figure out how to
tell if the ProfileInfo struct was ever set. I'd be happy to get input :).

[Paweł Hajdan Jr., 2010-07-14 00:53:14]

LGTM with a comment.

http://codereview.chromium.org/2923010/diff/7002/18001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2923010/diff/7002/18001#newcode2450
chrome/browser/automation/automation_provider.cc:2450: Profile* prof =
profile();
nit: Let's get rid of the abbreviation and change the name to |profile|.

[Alyssa, 2010-07-14 20:47:49]

http://codereview.chromium.org/2923010/diff/7002/18001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2923010/diff/7002/18001#newcode2450
chrome/browser/automation/automation_provider.cc:2450: Profile* prof =
profile();
On 2010/07/14 00:53:14, Paweł Hajdan Jr. wrote:
> nit: Let's get rid of the abbreviation and change the name to |profile|.

Done.