Fix a couple of firefox search engine import bugs.

Fix a couple of firefox search engine import bugs.

1) on ubuntu, firefox3 search engines are stored in a slightly different place. See bug 53899 for more info.
2) fix parse error in ReadPrefJsValue. Firefox pref values may contain parentheses, e.g. "Wikipedia (en)"

BUG=53899
TEST=create a new firefox profile and set it as the default profile, and set firefox3 as the default browser. Launch chrome with --user-data-dir=/tmp/foo. The default set of firefox search engines should be silently imported. (Also, after I write a patch to do so, the first run ballot box should show the default ffox search engine along side the 3 defaults, if it is not one of the three defaults).

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

[Evan Martin, 2010-08-31 16:26:01]

Code LGTM, one concern that might warrant more testing

http://codereview.chromium.org/3221007/diff/2001/3001
File chrome/browser/importer/firefox3_importer.cc (right):

http://codereview.chromium.org/3221007/diff/2001/3001#newcode398
chrome/browser/importer/firefox3_importer.cc:398: FilePath locale_app_path =
app_path.AppendASCII(locale_);
Do we need to worry about differences in how locales are named?  I'm especially
concerned with the difference between "en_US" and "en-US", but I think there's
also a corner case in that different apps represent Hebrew in different ways (HE
vs IW or something).

[Evan Martin, 2010-08-31 16:27:01]

http://codereview.chromium.org/3221007/diff/2001/3003
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/3221007/diff/2001/3003#newcode298
chrome/browser/importer/firefox_importer_utils.cc:298: }
Oops, skipped over this file in my review.  Is there a unittest we can amend to
cover this case?

[Evan Stade, 2010-08-31 19:59:47]

+jshin (probably has a better answer than me. Jungshik: Evan's question is in
his first message)

We do some sanitation of the locale string.

$ LANG=en_US ./out/Release/chrome
$ LANG=en-US ./out/Release/chrome
$ LANG=en_US.utf8 ./out/Release/chrome

all of these commands yield a locale of en-US in the eyes of Chrome. The locale
strings that ubuntu uses for the searchengines directories, at least on my
machine, are also "language[-REGION]" format. The language and country come from
ICU, so hopefully the names will match the names that ubuntu is using.

http://codereview.chromium.org/3221007/diff/2001/3003
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/3221007/diff/2001/3003#newcode298
chrome/browser/importer/firefox_importer_utils.cc:298: }
On 2010/08/31 16:27:02, Evan Martin wrote:
> Oops, skipped over this file in my review.  Is there a unittest we can amend
to
> cover this case?

no, this code is unfortunately untested (same for the search engine import)

[Evan Martin, 2010-08-31 21:31:59]

http://codereview.chromium.org/3221007/diff/2001/3003
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/3221007/diff/2001/3003#newcode296
chrome/browser/importer/firefox_importer_utils.cc:296: stop = content.find("\n",
start + 1);
Will this do the right thing if the line isn't \n terminated? (Like the last one
in the file)

http://codereview.chromium.org/3221007/diff/2001/3003#newcode297
chrome/browser/importer/firefox_importer_utils.cc:297: stop = content.rfind(")",
stop);
Will this do the right thing if it finds a ')' somewhere in a previous line?

[Evan Stade, 2010-08-31 22:06:32]

http://codereview.chromium.org/3221007/diff/2001/3003
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/3221007/diff/2001/3003#newcode296
chrome/browser/importer/firefox_importer_utils.cc:296: stop = content.find("\n",
start + 1);
On 2010/08/31 21:31:59, Evan Martin wrote:
> Will this do the right thing if the line isn't \n terminated? (Like the last
one
> in the file)

that one should also be \n terminated, but yes I think it would work anyway
(find would return npos)

http://codereview.chromium.org/3221007/diff/2001/3003#newcode297
chrome/browser/importer/firefox_importer_utils.cc:297: stop = content.rfind(")",
stop);
On 2010/08/31 21:31:59, Evan Martin wrote:
> Will this do the right thing if it finds a ')' somewhere in a previous line?

well I guess if there is no ) in this line, then something bad could happen.
I'll add some tests I guess.

[jungshik at Google, 2010-08-31 22:18:48]

Evan, does Firefox3 on Ubuntu use 'he-IL', 'fr-FR', 'sv-SE' instead of 'he',
'fr', 'sv'?  

One more question: does it also have 'en-Foo' other than 'en-US' and 'en-GB' and
'fr-CA', 'fr-CH', 'de-CH', 'ar-Foo', 'es-Foo'  and so forth?

http://codereview.chromium.org/3221007/diff/2001/3001
File chrome/browser/importer/firefox3_importer.cc (right):

http://codereview.chromium.org/3221007/diff/2001/3001#newcode398
chrome/browser/importer/firefox3_importer.cc:398: FilePath locale_app_path =
app_path.AppendASCII(locale_);
On 2010/08/31 16:26:02, Evan Martin wrote:
> Do we need to worry about differences in how locales are named?  I'm
especially
> concerned with the difference between "en_US" and "en-US", but I think there's
> also a corner case in that different apps represent Hebrew in different ways
(HE
> vs IW or something).

Chrome uses the standard 'he' internally and Firefox does, too. ('iw' got
obsoleted so long ago that Firefox couldn't possibly use it. Why Google use it?
Don't ask me :-)) So, Hebrew is not a concern except that Firefox may use
'he-IL' instead of just 'he'. 

However, we have to go through the list of Firefox locale codes (Firefox may use
'hi-IN' instead of 'hi' for Hindi, for instance). 

If that's indeed the case, we have to 'expand' Chrome's UI locale name to
include the region where a given language is most widely spoken (there's an ICU
API for that). For now, just adding a TODO comment about the possibility would
suffice.

[Evan Martin, 2010-08-31 22:20:54]

+fta, who might know the answer to jshin's question

[Evan Stade, 2010-08-31 23:01:41]

test added

http://codereview.chromium.org/3221007/diff/2001/3001
File chrome/browser/importer/firefox3_importer.cc (right):

http://codereview.chromium.org/3221007/diff/2001/3001#newcode398
chrome/browser/importer/firefox3_importer.cc:398: FilePath locale_app_path =
app_path.AppendASCII(locale_);
On 2010/08/31 22:18:49, Jungshik Shin wrote:
> On 2010/08/31 16:26:02, Evan Martin wrote:
> > Do we need to worry about differences in how locales are named?  I'm
> especially
> > concerned with the difference between "en_US" and "en-US", but I think
there's
> > also a corner case in that different apps represent Hebrew in different ways
> (HE
> > vs IW or something).
> 
> Chrome uses the standard 'he' internally and Firefox does, too. ('iw' got
> obsoleted so long ago that Firefox couldn't possibly use it. Why Google use
it?
> Don't ask me :-)) So, Hebrew is not a concern except that Firefox may use
> 'he-IL' instead of just 'he'. 
> 
> However, we have to go through the list of Firefox locale codes (Firefox may
use
> 'hi-IN' instead of 'hi' for Hindi, for instance). 
> 
> If that's indeed the case, we have to 'expand' Chrome's UI locale name to
> include the region where a given language is most widely spoken (there's an
ICU
> API for that). For now, just adding a TODO comment about the possibility would
> suffice. 
> 
> 

TODO added

[Evan Martin, 2010-08-31 23:31:01]

LGTM

[micah.gersten, 2010-09-01 07:00:59]

On 2010/08/31 22:18:48, Jungshik Shin wrote:
> Evan, does Firefox3 on Ubuntu use 'he-IL', 'fr-FR', 'sv-SE' instead of 'he',
> 'fr', 'sv'?  
> 
> One more question: does it also have 'en-Foo' other than 'en-US' and 'en-GB'
and
> 'fr-CA', 'fr-CH', 'de-CH', 'ar-Foo', 'es-Foo'  and so forth?
> 
> http://codereview.chromium.org/3221007/diff/2001/3001
> File chrome/browser/importer/firefox3_importer.cc (right):
> 
> http://codereview.chromium.org/3221007/diff/2001/3001#newcode398
> chrome/browser/importer/firefox3_importer.cc:398: FilePath locale_app_path =
> app_path.AppendASCII(locale_);
> On 2010/08/31 16:26:02, Evan Martin wrote:
> > Do we need to worry about differences in how locales are named?  I'm
> especially
> > concerned with the difference between "en_US" and "en-US", but I think
there's
> > also a corner case in that different apps represent Hebrew in different ways
> (HE
> > vs IW or something).
> 
> Chrome uses the standard 'he' internally and Firefox does, too. ('iw' got
> obsoleted so long ago that Firefox couldn't possibly use it. Why Google use
it?
> Don't ask me :-)) So, Hebrew is not a concern except that Firefox may use
> 'he-IL' instead of just 'he'. 
> 
> However, we have to go through the list of Firefox locale codes (Firefox may
use
> 'hi-IN' instead of 'hi' for Hindi, for instance). 
> 
> If that's indeed the case, we have to 'expand' Chrome's UI locale name to
> include the region where a given language is most widely spoken (there's an
ICU
> API for that). For now, just adding a TODO comment about the possibility would
> suffice.

There can be multiple locales per language and Firefox and Ubuntu take this into
account.

[jungshik at Google, 2010-09-01 18:15:28]

On 2010/09/01 07:00:59, micah.gersten wrote:
> On 2010/08/31 22:18:48, Jungshik Shin wrote:
> > Evan, does Firefox3 on Ubuntu use 'he-IL', 'fr-FR', 'sv-SE' instead of 'he',
> > 'fr', 'sv'?  
> > 
> > One more question: does it also have 'en-Foo' other than 'en-US' and 'en-GB'
> and
> > 'fr-CA', 'fr-CH', 'de-CH', 'ar-Foo', 'es-Foo'  and so forth?
> > 
> > http://codereview.chromium.org/3221007/diff/2001/3001
> > File chrome/browser/importer/firefox3_importer.cc (right):
> > 
> > http://codereview.chromium.org/3221007/diff/2001/3001#newcode398
> > chrome/browser/importer/firefox3_importer.cc:398: FilePath locale_app_path =
> > app_path.AppendASCII(locale_);
> > On 2010/08/31 16:26:02, Evan Martin wrote:
> > > Do we need to worry about differences in how locales are named?  I'm
> > especially
> > > concerned with the difference between "en_US" and "en-US", but I think
> there's
> > > also a corner case in that different apps represent Hebrew in different
ways
> > (HE
> > > vs IW or something).
> > 
> > Chrome uses the standard 'he' internally and Firefox does, too. ('iw' got
> > obsoleted so long ago that Firefox couldn't possibly use it. Why Google use
> it?
> > Don't ask me :-)) So, Hebrew is not a concern except that Firefox may use
> > 'he-IL' instead of just 'he'. 
> > 
> > However, we have to go through the list of Firefox locale codes (Firefox may
> use
> > 'hi-IN' instead of 'hi' for Hindi, for instance). 
> > 
> > If that's indeed the case, we have to 'expand' Chrome's UI locale name to
> > include the region where a given language is most widely spoken (there's an
> ICU
> > API for that). For now, just adding a TODO comment about the possibility
would
> > suffice.
> 
> There can be multiple locales per language and Firefox and Ubuntu take this
into
> account.

Thta's what I suspected. Anyway, for now, the CL LGTM.