Add import settings dialog on linux.

Add import settings dialog on linux.

BUG=11191
TEST=Open Import Settings dialog from Chrome wrench menu on Linux and make sure it imports Firefox data as selected.

[kuchhal, 2009-05-08 17:32:13]

There are still many todos here including doing the actual import in a different
thread but I thought I will do those incrementally. I have tried to keep the MAC
behavior unchanged since I haven't tested this there. On my ghardy machine I can
import bookmarks, history and search_engines successfully.

[Evan Martin, 2009-05-08 17:50:00]

Wow, that was fast.

http://codereview.chromium.org/115133/diff/1011/15
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/115133/diff/1011/15#newcode41
Line 41: gtk_widget_set_size_request(dialog, 300, -1);
I'd use

http://library.gnome.org/devel/gtk/2.15/GtkWindow.html#gtk-window-set-default...

if you want it to be wider than its default width.

http://codereview.chromium.org/115133/diff/1011/15#newcode44
Line 44: GtkWidget* alignment = gtk_alignment_new(0.0, 0.0, 0.0, 0.0);
Doesn't this make all the content flush against the top-left?  I think it's more
common for dialogs to either be nonresizable or to resize with their content...

Ah, I see below you make it nonresizable.  So why do this?

http://codereview.chromium.org/115133/diff/1011/15#newcode92
Line 92: g_signal_connect(dialog, "response",
G_CALLBACK(HandleOnResponseDialog),
FWIW, normal GTK style is to put the newline before G_CALLBACK (that way the
function and its argument are on the same line).

http://codereview.chromium.org/115133/diff/1011/15#newcode99
Line 99: if (response == GTK_RESPONSE_REJECT) {
What happens in the case when someone clicks the close button on the dialog?  I
believe you might get a different response there?

http://codereview.chromium.org/115133/diff/1011/17
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/115133/diff/1011/17#newcode115
Line 115: int& version,
For out parameters we typically do "int* profile", "std::wstring* app_path",
right?

http://codereview.chromium.org/115133/diff/1011/17#newcode121
Line 121: file_util::ReadFileToString(compatibility_file, &content);
What happens if this file doesn't exist?  Should the function return false?

http://codereview.chromium.org/115133/diff/1011/17#newcode127
Line 127: std::string line = lines[i];
You could use a const reference here to avoid a copy, not that it matters much.

http://codereview.chromium.org/115133/diff/1011/17#newcode134
Line 134: version = line.substr(equal + 1)[0] - 48;
I'd write this as:
  version = line[equal + 1] - '0';
or maybe just use atoi() or one of the string utility functions?

http://codereview.chromium.org/115133/diff/1011/17#newcode142
Line 142: }
By the way, my ~/.mozilla/firefox/ has no compatibility.ini file...

http://codereview.chromium.org/115133/diff/1011/17#newcode145
Line 145: std::wstring GetProfilesINI() {
This could return a FilePath to avoid string conversions.

http://codereview.chromium.org/115133/diff/1011/17#newcode162
Line 162: // under user HOME directory in .mozilla\firefox folder on Linux.
.mozilla/firefox

http://codereview.chromium.org/115133/diff/1011/17#newcode163
Line 163: const char *home_dir = getenv("HOME");
We have GetHomeDir in chrome/common/chrome_paths_linux.cc, but I think there's
no easier way to get at it from here.  This is probably ok.

http://codereview.chromium.org/115133/diff/1011/17#newcode165
Line 165: FilePath homed(home_dir);
To avoid this weird name you could maybe rename home_dir to just "home" since it
matches the env variable.

[kuchhal, 2009-05-08 19:02:29]

Thanks for the review. I believe I fixed all of the comments except the ones I
commented on below. Please take another look when you get a chance.

http://codereview.chromium.org/115133/diff/1011/15
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/115133/diff/1011/15#newcode41
Line 41: gtk_widget_set_size_request(dialog, 300, -1);
Hmm for some reason when I call gtk_window_set_default_size(GTK_WINDOW(dialog),
300, -1), the call gets ignored completely. Does anything in this call look
wrong?

On 2009/05/08 17:50:00, Evan Martin wrote:
> I'd use
> 
>
http://library.gnome.org/devel/gtk/2.15/GtkWindow.html#gtk-window-set-default...
> 
> if you want it to be wider than its default width.

http://codereview.chromium.org/115133/diff/1011/15#newcode44
Line 44: GtkWidget* alignment = gtk_alignment_new(0.0, 0.0, 0.0, 0.0);
I added this to keep the controls left aligned, otherwise the combo hbox and the
select label were showing up centered in the dialog box. 
On 2009/05/08 17:50:00, Evan Martin wrote:
> Doesn't this make all the content flush against the top-left?  I think it's
more
> common for dialogs to either be nonresizable or to resize with their
content...
> 
> Ah, I see below you make it nonresizable.  So why do this?

http://codereview.chromium.org/115133/diff/1011/15#newcode92
Line 92: g_signal_connect(dialog, "response",
G_CALLBACK(HandleOnResponseDialog),
Done.
On 2009/05/08 17:50:00, Evan Martin wrote:
> FWIW, normal GTK style is to put the newline before G_CALLBACK (that way the
> function and its argument are on the same line).

http://codereview.chromium.org/115133/diff/1011/15#newcode99
Line 99: if (response == GTK_RESPONSE_REJECT) {
Correct. Fixed it.
On 2009/05/08 17:50:00, Evan Martin wrote:
> What happens in the case when someone clicks the close button on the dialog? 
I
> believe you might get a different response there?

http://codereview.chromium.org/115133/diff/1011/17
File chrome/browser/importer/firefox_importer_utils.cc (right):

http://codereview.chromium.org/115133/diff/1011/17#newcode115
Line 115: int& version,
Fixed.
On 2009/05/08 17:50:00, Evan Martin wrote:
> For out parameters we typically do "int* profile", "std::wstring* app_path",
> right?

http://codereview.chromium.org/115133/diff/1011/17#newcode121
Line 121: file_util::ReadFileToString(compatibility_file, &content);
Actually since content will be empty we will end up returning false later.

On 2009/05/08 17:50:00, Evan Martin wrote:
> What happens if this file doesn't exist?  Should the function return false?

http://codereview.chromium.org/115133/diff/1011/17#newcode142
Line 142: }
Did you check under .mozilla/firefox/<profile> directory? This page -
http://kb.mozillazine.org/Profile_folder_-_Firefox - seems to claim the file
gets regenerated every time Firefox is launched.
On 2009/05/08 17:50:00, Evan Martin wrote:
> By the way, my ~/.mozilla/firefox/ has no compatibility.ini file...

[Evan Martin, 2009-05-08 19:12:12]

(It looks like you didn't upload the new patch?)

[Evan Martin, 2009-05-08 19:24:00]

Ok, looks good!

http://codereview.chromium.org/115133/diff/28/1019
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/115133/diff/28/1019#newcode8
Line 8: #include <wchar.h>
is this necessary?

[Evan Martin, 2009-05-11 23:48:13]

http://codereview.chromium.org/115133/diff/97/1041
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/115133/diff/97/1041#newcode106
Line 106: // TODO(rahulk): We should not do the import on this thread. Instead
Please don't forget about this (file a bug if you're not working on it soon)!