Ensure that Firefox search engines are imported correctly....

Ensure that Firefox search engines are imported correctly.
It also makes sure that engines which have been removed from Firefox
are not imported into Chrome.

BUG= http://crbug.com/12245
TEST= Install firefox, and do not change the default search engines.
Start Chrome, and import search engines from Firefox.  Default Firefox
engines should now be part of Chrome.



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

[Miranda Callahan, 2009-05-29 17:33:26]

Fixes the problem that Chrome had importing FF3 search engines by looking in the
FF3 app directory iff FF has not yet written to the search.sqlite database. 
Also fixes a bug which had not been mentioned: Chrome would import all search
engines which had been added to FF3, but not respect removes.

[sky, 2009-05-29 18:05:49]

http://codereview.chromium.org/115895/diff/1004/1005
File chrome/browser/importer/firefox3_importer.cc (right):

http://codereview.chromium.org/115895/diff/1004/1005#newcode332
Line 332: "SELECT engineid FROM engine_data WHERE name='hidden') "
I'm pretty confused by this statement. I think you want: "SELECT engineid from
engine_data WHERE name!='hidden'". If I'm wrong, could explain what you are
trying to accomplish?

http://codereview.chromium.org/115895/diff/1004/1005#newcode351
Line 351: for (engine_path = engines.Next(); !engine_path.value().empty();
nit: your spacing is off here.
Also, put engine_path inside the for block, eg for (FilePath engine_path = ...

http://codereview.chromium.org/115895/diff/1004/1005#newcode357
Line 357: const std::wstring kAppPrefix = L"[app]/";
nit: spacing off here too.

http://codereview.chromium.org/115895/diff/1004/1005#newcode378
Line 378: // Remove '[profile]/'.
nit: Your comment describes what happens in this block. As such, I would suggest
putting it right after the else if.

[Miranda Callahan, 2009-05-29 18:33:01]

http://codereview.chromium.org/115895/diff/1004/1005
File chrome/browser/importer/firefox3_importer.cc (right):

http://codereview.chromium.org/115895/diff/1004/1005#newcode332
Line 332: "SELECT engineid FROM engine_data WHERE name='hidden') "
On 2009/05/29 18:05:49, sky wrote:
> I'm pretty confused by this statement. I think you want: "SELECT engineid from
> engine_data WHERE name!='hidden'". If I'm wrong, could explain what you are
> trying to accomplish?

Heh -- it was a thorny SQL issue, and I'd be glad if someone has a more elegant
solution!  I was basically trying to reverse-engineer the FF3 search engine DB. 
Here's the problem:  The search engine names are stored multiple times in the
database: once for each time they have ever appeared in FF, and once *again* if
they have been removed.  So, for example,

Google
Yahoo
Wikipedia
Google | hidden

Means that only Yahoo and Wikipedia are visible in Firefox, and thus the only
values I want to import; Google was once there, but has been removed.  If I only
choose with name != hidden, I get Google anyway, because it's represented twice.
 So I do an except -- all search engines, EXCEPT those with the value hidden. 
NOW the problem is that I want to sort by the order they were in in the FF3
search engine -- which is their value field.  So the full data set looks
something like: 

Google | Order | 2
Yahoo | Order | 1
Wikipedia | Order | 3
Google | hidden | 1

The trouble here is that the value field is different in the hidden and
nonhidden lines, so that I can't do an EXCEPT with these columns -- thus I have
to nest the calls.  :-(

If there is a neater way to do this, I would be delighted!

http://codereview.chromium.org/115895/diff/1004/1005#newcode351
Line 351: for (engine_path = engines.Next(); !engine_path.value().empty();
On 2009/05/29 18:05:49, sky wrote:
> nit: your spacing is off here.
> Also, put engine_path inside the for block, eg for (FilePath engine_path = ...

Done.

http://codereview.chromium.org/115895/diff/1004/1005#newcode357
Line 357: const std::wstring kAppPrefix = L"[app]/";
On 2009/05/29 18:05:49, sky wrote:
> nit: spacing off here too.

Done.

http://codereview.chromium.org/115895/diff/1004/1005#newcode378
Line 378: // Remove '[profile]/'.
On 2009/05/29 18:05:49, sky wrote:
> nit: Your comment describes what happens in this block. As such, I would
suggest
> putting it right after the else if.

Makes sense -- I was contorting myself to preserve the commenting that had been
at the end of the line before I indented and made it run over 80 cols.  Done.

[Miranda Callahan, 2009-05-29 19:34:37]

SQL improved -- thanks, Scott!

[sky, 2009-05-29 19:38:30]

LGTM