Fixes bug in importer where we could set the default search provider...

Fixes bug in importer where we could set the default search provider
to one that doesn't support replacement. Also changed uniquing to
consider invalid OSDD urls. Need this to pick up Windows Live Search.

I also changed the ff importer to return out early on if it couldn't
find the value for the search provider. I encountered this do to
hitting a NOTREACHED.

BUG=1507
TEST=In IE set your default search to Live Search. Import from IE and
  make sure Chrome sets the default search to Live Search. Also make
  sure this didn't break keyword importing.


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

[Peter Kasting, 2008-09-25 20:54:44]

http://codereview.chromium.org/4281/diff/1/3
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/4281/diff/1/3#newcode334
Line 334: // TODO: should fallback to 'browser.search.defaultengine' if
selectedEngine
Is this harder to do than just "ReadPrefsJsValue(profile_path,
"browser.search.defaultengine")"?  If not, why not go ahead and do it here?

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

http://codereview.chromium.org/4281/diff/1/2#newcode153
Line 153: // string is build from the raw TemplateURL string.
Nit: "build" -> "built"

http://codereview.chromium.org/4281/diff/1/2#newcode226
Line 226: BuildHostPathKey(t_url, true)) != host_path_map.end()) {
I don't understand what the practical effect of this is going to be.  Can you
write some comments about what sort of input data set you're trying to
accommodate?  It looks sort of like you're trying to catch the case where a user
has two entries for the same engine, one replaceable and one not, and the "not
replaceable" one has been set as the default?

[sky, 2008-09-25 23:08:05]

http://codereview.chromium.org/4281/diff/1/3
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/4281/diff/1/3#newcode334
Line 334: // TODO: should fallback to 'browser.search.defaultengine' if
selectedEngine
On 2008/09/25 20:54:44, pkasting wrote:
> Is this harder to do than just "ReadPrefsJsValue(profile_path,
> "browser.search.defaultengine")"?  If not, why not go ahead and do it here?


That results in reading the file in twice. We should avoid that if possible.

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

http://codereview.chromium.org/4281/diff/1/2#newcode226
Line 226: BuildHostPathKey(t_url, true)) != host_path_map.end()) {
On 2008/09/25 20:54:44, pkasting wrote:
> I don't understand what the practical effect of this is going to be.  Can you
> write some comments about what sort of input data set you're trying to
> accommodate?  It looks sort of like you're trying to catch the case where a
user
> has two entries for the same engine, one replaceable and one not, and the "not
> replaceable" one has been set as the default?

I had this wrong. I've updated BuildHostPathKey appropriately.

[Peter Kasting, 2008-09-25 23:29:59]

http://codereview.chromium.org/4281/diff/1/3
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/4281/diff/1/3#newcode334
Line 334: // TODO: should fallback to 'browser.search.defaultengine' if
selectedEngine
On 2008/09/25 23:08:06, sky wrote:
> On 2008/09/25 20:54:44, pkasting wrote:
> > Is this harder to do than just "ReadPrefsJsValue(profile_path,
> > "browser.search.defaultengine")"?  If not, why not go ahead and do it here?
> 
> 
> That results in reading the file in twice. We should avoid that if possible.

But how much does that hurt?  Aren't we only calling this once per import?  How
much time does it take to read this file?  Without a proposal of how we're going
to do a faster version of the fallback code, it seems like it'd be better to do
the slow version than nothing.  Or is the practical effect here not large?

http://codereview.chromium.org/4281/diff/6/205
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/4281/diff/6/205#newcode157
Line 157: // match the host+path of one of the default URLs.
OK, this is making a little more sense.  It still might be nice if you provided
a real-world sample invalid URL as an example.

[Peter Kasting, 2008-09-25 23:44:19]

LGTM

http://codereview.chromium.org/4281/diff/10/210
File chrome/browser/importer/importer.cc (right):

http://codereview.chromium.org/4281/diff/10/210#newcode161
Line 161: // the TemplateURL is invalid.
Thanks for this comment, it helps.

It might be even clearer if you moved it to the caller location and changed it
to something like: "Set the default engine based on even invalid URLs, if they
match a valid URL we already know.  For example, IE's Live Search URL contains
'{Language}', which is not a valid OSDD parameter and thus makes the TemplateURL
invalid, but its host+path portion still matches our Live Search prepopulate
data entry."

Up to you.