Implement "Autofill form data" import for Firefox

Implement "Autofill form data" import for Firefox

This patches adds support to import autofill form data
from Firefox. Firefox stores form data in formhistory.sqlite
file and currently Chrome's firefox importer doesn't import data
from it.

With this patch, "Autofill form data" option is available in import
menu and allows user to import form data from Firefox. The imported
form data is stored in autofill table and is then used in forms for
autocomplete.

BUG=59087
Committed: https://crrev.com/884eafca142af3f8e8ed252048edeeb5c3d25319
Cr-Commit-Position: refs/heads/master@{#293381}

[Nikhil, 2014-08-18 13:50:08]

PTAL at the changes to support importing "Autofill form data" from Firefox. 

[*] If you think it's better to split this CL into smaller patches then please
let me know.

[*] This is missing tests. I'll add one once my other import related CL lands
(https://codereview.chromium.org/426653002/). But, please take a look.

If you've any review comments, then please let me know. I'll incorporate them.

Thanks!

https://codereview.chromium.org/480953002/diff/1/chrome/browser/importer/prof...
File chrome/browser/importer/profile_writer.cc (right):

https://codereview.chromium.org/480953002/diff/1/chrome/browser/importer/prof...
chrome/browser/importer/profile_writer.cc:340:
web_data_service->UpdateAutofillEntries(autofill_entries);
I'm not exactly sure if this is the correct approach. Please let me know WDYT.

[Nikhil, 2014-08-18 13:52:03]

Forgot to add reviewer :/

Ilya, PTAL.

Thanks!

[Nikhil, 2014-08-19 12:52:37]

isherman@ - Test case added. PTAL. Thanks!

[Ilya Sherman, 2014-08-20 05:47:42]

Thanks, Nikhil!  I have a bunch of nits for you; but on the whole, this CL looks
great :)

https://codereview.chromium.org/480953002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7413: +          Autofill form data
nit: I think "Form data" or "Form data for Autofill" would be a slightly better
choice of string here.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/external_process_importer_client.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/external_process_importer_client.h:90:
ImporterAutofillFormDataEntry>& autofill_form_data_entry_group);
nit: Please run git cl format to fix up the formatting here, and throughout the
CL.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:103: const
AutofillFormDataInfo kFirefoxAutofill[] = {
nit: "kFirefoxAutofillEntries"

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:105: {"n300",
"user_two"},
Please include additional cases: case sensitivity, unicode characters, empty
strings, and any other potential edge cases that you can think of.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/profile_writer.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/profile_writer.cc:339: if (!autofill_entries.empty() ||
web_data_service.get())
Hmm, why is this condition an 'or'?  It seems like it should be an 'and'
instead.  Actually, it seems like whether or not |autofill_entries| is empty
ought to be handled internally to the UpdateAutofillEntries() method.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/profile_writer.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/profile_writer.h:94: const
std::vector<autofill::AutofillEntry>& autofill_entries);
nit: Please document this method.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/import_data_overlay.html (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.html:67: <span
id="import-autofillformdata-with-label"
nit: "autofillformdata" -> "autofill-form-data" (applies throughout)

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.html:72: <label
for="import-autofillformdata" i18n-content="importAutofillFormData"></label>
nit: Please wrap this line to 80-col.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/import_data_handler.cc:58: {"importFromLabel",
IDS_IMPORT_FROM_LABEL},
nit: Are the diffs in these lines a result of running "git cl format" (or in
line with what that tool would output)?  If not, please revert the diffs.  (IMO,
either style is fine; but the code review tool makes these diffs look more
significant than they are, which makes them a red herring for this CL.  Hence,
I'd only include this change if the clang format tool is more opinionated than I
am about which style is correct.)

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/import_data_handler.cc:194: "autofillformdata",
nit: Even though the style above is wrong, please match it (or update all of the
lines to use the more correct style).

https://codereview.chromium.org/480953002/diff/20001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/chrome_common.gyp...
chrome/chrome_common.gypi:253:
'common/importer/importer_autofill_form_data_entry.h',
nit: Please alphabetize.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
File chrome/common/importer/importer_autofill_form_data_entry.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
chrome/common/importer/importer_autofill_form_data_entry.h:11: // Used as the
target for importing form history from other browser's profiles
nit: "browser's" -> "browsers'"

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
File chrome/common/importer/importer_bridge.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
chrome/common/importer/importer_bridge.h:15: #include
"chrome/common/importer/importer_autofill_form_data_entry.h"
nit: Can you forward-declare the type, rather than #including the header here?

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
File chrome/common/importer/importer_data_types.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
chrome/common/importer/importer_data_types.h:32: ALL = (1 << 7) - 1  // All the
bits should be 1, hence the -1.
nit: Please keep all the equals signs aligned.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...
chrome/common/importer/profile_import_process_messages.h:268: };  //
ParamTraits<ImporterAutofillFormDataEntry>
Hmm, can you use the IPC_STRUCT_TRAITS_* macros, rather than writing all of this
code by hand?  If so, it would be great (in a follow-up CL) to also update the
other code in this file to do so.  Writing this code by hand is verbose and
error-prone.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...
chrome/common/importer/profile_import_process_messages.h:376:
std::vector<ImporterAutofillFormDataEntry>)
nit: Please leave a blank line after this one.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1139: const char kImportAutofillFormData[] =
"import_autofill_form_data";
Hmm, does this only affect first-run?  Is there any reason that we'd ever want
to automatically import form data on the first run?  That seems pretty
heavy-handed to me... though maybe that's consistent with how we import on first
run in general.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.h:357: extern const char kImportAutofillFormData[];
nit: Please alphabetize (it looks like the last two existing lines should be
swapped as well).

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/external_process_importer_bridge.cc:28: const int
kNumAutofillFormDataToSend = 100;
nit: Please leave a blank line after this one.

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/external_process_importer_bridge.cc:28: const int
kNumAutofillFormDataToSend = 100;
nit: Perhaps it's appropriate to reduce these four constants down to a single
one, since they are all equal?

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:400: const char* query =
nit: "const char query[] =" is more const (both the characters and the pointer
are const), and slightly more efficient.

https://codereview.chromium.org/480953002/diff/20001/components/autofill/core...
File components/autofill/core/browser/webdata/autofill_webdata.h (right):

https://codereview.chromium.org/480953002/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_webdata.h:79: // Schedules a
task to update autofill entries to the web database.
nit: "to the web database" -> "in the web database"

https://codereview.chromium.org/480953002/diff/20001/components/autofill/core...
File components/autofill/core/browser/webdata/autofill_webdata_backend_impl.cc
(right):

https://codereview.chromium.org/480953002/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_webdata_backend_impl.cc:254:
NOTREACHED();
nit: I think it's possible and reasonable for this code to be reached, e.g. if
the user's disk is full.  I'd omit the NOTREACHED().

[Nikhil, 2014-08-20 11:29:18]

Ilya, Thanks for the quick review :)

PTAL at my changes and responses to your review comments.

[*] I'm going to improve test case in next revision of this CL. But please do
take a look at other changes.

Thanks!

https://codereview.chromium.org/480953002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7413: +          Autofill form data
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: I think "Form data" or "Form data for Autofill" would be a slightly
better
> choice of string here.

Copying your comment from an earlier patch -
"nit: Per glen@, let's make this string "Autofill form data" to match the string
in the "Clear Browsing Data" UI."

Link -
https://chromiumcodereview.appspot.com/11305021/diff/12001/chrome/app/generat...

I think it's better to keep them consistent. Please let me know WDYT.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/external_process_importer_client.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/external_process_importer_client.h:90:
ImporterAutofillFormDataEntry>& autofill_form_data_entry_group);
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Please run git cl format to fix up the formatting here, and throughout
the
> CL.

Hmm, what is it that is wrong here? This is a result of "git cl format".

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:103: const
AutofillFormDataInfo kFirefoxAutofill[] = {
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: "kFirefoxAutofillEntries"

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:105: {"n300",
"user_two"},
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> Please include additional cases: case sensitivity, unicode characters, empty
> strings, and any other potential edge cases that you can think of.

I'm going to add these in next revision of this CL as it will take some time to
prepare binary file accordingly. Hope this is okay.

But please do take a look at other changes, and let me know your review comments
:)

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/profile_writer.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/profile_writer.cc:339: if (!autofill_entries.empty() ||
web_data_service.get())
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> Hmm, why is this condition an 'or'?  It seems like it should be an 'and'
> instead.  Actually, it seems like whether or not |autofill_entries| is empty
> ought to be handled internally to the UpdateAutofillEntries() method.

It should have been an 'and'. Thanks for catching this! 

I've removed the check whether |autofill_entries| is empty or not since it is
already being done in UpdateAutofillEntries() method.
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/profile_writer.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/profile_writer.h:94: const
std::vector<autofill::AutofillEntry>& autofill_entries);
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Please document this method.

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/import_data_overlay.html (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.html:67: <span
id="import-autofillformdata-with-label"
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: "autofillformdata" -> "autofill-form-data" (applies throughout)

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.html:72: <label
for="import-autofillformdata" i18n-content="importAutofillFormData"></label>
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Please wrap this line to 80-col.

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/import_data_handler.cc:58: {"importFromLabel",
IDS_IMPORT_FROM_LABEL},
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Are the diffs in these lines a result of running "git cl format" (or in
> line with what that tool would output)?  If not, please revert the diffs. 
(IMO,
> either style is fine; but the code review tool makes these diffs look more
> significant than they are, which makes them a red herring for this CL.  Hence,
> I'd only include this change if the clang format tool is more opinionated than
I
> am about which style is correct.)

Yep, this is a result of running "git cl format".

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/import_data_handler.cc:194: "autofillformdata",
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Even though the style above is wrong, please match it (or update all of
the
> lines to use the more correct style).

This was result of "git cl format". I've matched it with above.

https://codereview.chromium.org/480953002/diff/20001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/chrome_common.gyp...
chrome/chrome_common.gypi:253:
'common/importer/importer_autofill_form_data_entry.h',
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Please alphabetize.

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
File chrome/common/importer/importer_autofill_form_data_entry.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
chrome/common/importer/importer_autofill_form_data_entry.h:11: // Used as the
target for importing form history from other browser's profiles
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: "browser's" -> "browsers'"

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
File chrome/common/importer/importer_bridge.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
chrome/common/importer/importer_bridge.h:15: #include
"chrome/common/importer/importer_autofill_form_data_entry.h"
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Can you forward-declare the type, rather than #including the header here?

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
File chrome/common/importer/importer_data_types.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/i...
chrome/common/importer/importer_data_types.h:32: ALL = (1 << 7) - 1  // All the
bits should be 1, hence the -1.
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Please keep all the equals signs aligned.

This was a result of git cl format. I've changed this manually now to keep all
equal signs aligned.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...
chrome/common/importer/profile_import_process_messages.h:268: };  //
ParamTraits<ImporterAutofillFormDataEntry>
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> Hmm, can you use the IPC_STRUCT_TRAITS_* macros, rather than writing all of
this
> code by hand?  If so, it would be great (in a follow-up CL) to also update the
> other code in this file to do so.  Writing this code by hand is verbose and
> error-prone.

Changes done for ImporterAutofillFormDataEntry. I'll update others in follow-up
CL.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...
chrome/common/importer/profile_import_process_messages.h:376:
std::vector<ImporterAutofillFormDataEntry>)
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1139: const char kImportAutofillFormData[] =
"import_autofill_form_data";
On 2014/08/20 05:47:41, Ilya Sherman wrote:
> Hmm, does this only affect first-run?  Is there any reason that we'd ever want
> to automatically import form data on the first run?  That seems pretty
> heavy-handed to me... though maybe that's consistent with how we import on
first
> run in general.

I think this is only for registering preferences with user profile. As I
understand, this preference is used in import_data_overlay.html.

One place that I seem to have missed (on account of consistency) adding
kImportAutofillFormData is configuration_policy_handler_list_factory.cc.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pol...

Shall I add there also?

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.h:357: extern const char kImportAutofillFormData[];
On 2014/08/20 05:47:42, Ilya Sherman wrote:
> nit: Please alphabetize (it looks like the last two existing lines should be
> swapped as well).

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/external_process_importer_bridge.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/external_process_importer_bridge.cc:28: const int
kNumAutofillFormDataToSend = 100;
On 2014/08/20 05:47:42, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/external_process_importer_bridge.cc:28: const int
kNumAutofillFormDataToSend = 100;
On 2014/08/20 05:47:42, Ilya Sherman wrote:
> nit: Perhaps it's appropriate to reduce these four constants down to a single
> one, since they are all equal?

Hmm, they all use different structures and have different memory requirements.
Not sure whether all these constants should be equal ideally.

Anyways, if you think these four constants should be reduced to one, then let me
know and I'll update this (maybe in follow-up CL?).

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:400: const char* query =
On 2014/08/20 05:47:42, Ilya Sherman wrote:
> nit: "const char query[] =" is more const (both the characters and the pointer
> are const), and slightly more efficient.

Changes done for this. But there are other queries in this file which could also
use similar change (follow-up patch?).

https://codereview.chromium.org/480953002/diff/20001/components/autofill/core...
File components/autofill/core/browser/webdata/autofill_webdata.h (right):

https://codereview.chromium.org/480953002/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_webdata.h:79: // Schedules a
task to update autofill entries to the web database.
On 2014/08/20 05:47:42, Ilya Sherman wrote:
> nit: "to the web database" -> "in the web database"

Done.

https://codereview.chromium.org/480953002/diff/20001/components/autofill/core...
File components/autofill/core/browser/webdata/autofill_webdata_backend_impl.cc
(right):

https://codereview.chromium.org/480953002/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_webdata_backend_impl.cc:254:
NOTREACHED();
On 2014/08/20 05:47:42, Ilya Sherman wrote:
> nit: I think it's possible and reasonable for this code to be reached, e.g. if
> the user's disk is full.  I'd omit the NOTREACHED().

Done.

[Ilya Sherman, 2014-08-20 20:47:23]

Thanks!

https://codereview.chromium.org/480953002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7413: +          Autofill form data
On 2014/08/20 11:29:16, Nikhil wrote:
> On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > nit: I think "Form data" or "Form data for Autofill" would be a slightly
> better
> > choice of string here.
> 
> Copying your comment from an earlier patch -
> "nit: Per glen@, let's make this string "Autofill form data" to match the
string
> in the "Clear Browsing Data" UI."
> 
> Link -
>
https://chromiumcodereview.appspot.com/11305021/diff/12001/chrome/app/generat...
> 
> I think it's better to keep them consistent. Please let me know WDYT.

Ah, thanks for reminding me.  I trust Glen's opinion on UI strings more than my
own, so SGTM :)

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/external_process_importer_client.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/external_process_importer_client.h:90:
ImporterAutofillFormDataEntry>& autofill_form_data_entry_group);
On 2014/08/20 11:29:16, Nikhil wrote:
> On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > nit: Please run git cl format to fix up the formatting here, and throughout
> the
> > CL.
> 
> Hmm, what is it that is wrong here? This is a result of "git cl format".

Ah, I see, this is wrapped as it is because the more common wrapping -- a line
break after the opening paren -- doesn't fit on two lines.  I guess this is ok,
though it's very unusual looking.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/import_data_handler.cc:194: "autofillformdata",
On 2014/08/20 11:29:16, Nikhil wrote:
> On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > nit: Even though the style above is wrong, please match it (or update all of
> the
> > lines to use the more correct style).
> 
> This was result of "git cl format". I've matched it with above.

Yep, I think "git cl format" did the right thing for this line, but it's odd
given that the other lines use a different format.  It would be good to fix up
the formatting for all of these lines as a small follow-up CL.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...
chrome/common/importer/profile_import_process_messages.h:268: };  //
ParamTraits<ImporterAutofillFormDataEntry>
On 2014/08/20 11:29:17, Nikhil wrote:
> On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > Hmm, can you use the IPC_STRUCT_TRAITS_* macros, rather than writing all of
> this
> > code by hand?  If so, it would be great (in a follow-up CL) to also update
the
> > other code in this file to do so.  Writing this code by hand is verbose and
> > error-prone.
> 
> Changes done for ImporterAutofillFormDataEntry. I'll update others in
follow-up
> CL.

Thanks!

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1139: const char kImportAutofillFormData[] =
"import_autofill_form_data";
On 2014/08/20 11:29:17, Nikhil wrote:
> On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > Hmm, does this only affect first-run?  Is there any reason that we'd ever
want
> > to automatically import form data on the first run?  That seems pretty
> > heavy-handed to me... though maybe that's consistent with how we import on
> first
> > run in general.
> 
> I think this is only for registering preferences with user profile. As I
> understand, this preference is used in import_data_overlay.html.

If that's so, then let's update the comment for this preference.

> One place that I seem to have missed (on account of consistency) adding
> kImportAutofillFormData is configuration_policy_handler_list_factory.cc.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pol...
> 
> Shall I add there also? 

Yeah, probably a good idea.  I'm not too familiar with the enterprise policy
code, though.

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:400: const char* query =
On 2014/08/20 11:29:18, Nikhil wrote:
> On 2014/08/20 05:47:42, Ilya Sherman wrote:
> > nit: "const char query[] =" is more const (both the characters and the
pointer
> > are const), and slightly more efficient.
> 
> Changes done for this. But there are other queries in this file which could
also
> use similar change (follow-up patch?).

Yes, this would be great to fix in a follow-up patch.  Thanks!

https://codereview.chromium.org/480953002/diff/40001/chrome/browser/importer/...
File chrome/browser/importer/in_process_importer_bridge.cc (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/browser/importer/...
chrome/browser/importer/in_process_importer_bridge.cc:15: #include
"chrome/common/importer/importer_autofill_form_data_entry.h"
nit: Please alphabetize.

https://codereview.chromium.org/480953002/diff/40001/chrome/browser/importer/...
File chrome/browser/importer/profile_writer.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/browser/importer/...
chrome/browser/importer/profile_writer.h:93: // Adds the imported autofill
entries in the autofill table.
nit: "in the autofill table" -> "to the autofill database"

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/i...
File chrome/common/importer/importer_bridge.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/i...
chrome/common/importer/importer_bridge.h:21: struct
ImporterAutofillFormDataEntry;
nit: Please alphabetize.

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/p...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/p...
chrome/common/importer/profile_import_process_messages.h:335: int /* total
number of entries to be imported */)
nit: It looks like two spaces before the opening "/*" is used pretty
consistently in this file.

https://codereview.chromium.org/480953002/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.h:357: extern const char kImportSearchEngine[];
Please update the .cc file to use the same order as the .h file.

[Nikhil, 2014-08-21 07:35:37]

Ilya, Thanks for the quick review!

PTAL at my changes and responses to your review comments.

[*] One additonal query - Should formhistory.sqlite be submitted manually? I see
an "A+" sign for it. Not sure what the "+" stands for.

[*] browser_test to be updated next :)

Thanks!

https://codereview.chromium.org/480953002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7413: +          Autofill form data
On 2014/08/20 20:47:22, Ilya Sherman wrote:
> On 2014/08/20 11:29:16, Nikhil wrote:
> > On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > > nit: I think "Form data" or "Form data for Autofill" would be a slightly
> > better
> > > choice of string here.
> > 
> > Copying your comment from an earlier patch -
> > "nit: Per glen@, let's make this string "Autofill form data" to match the
> string
> > in the "Clear Browsing Data" UI."
> > 
> > Link -
> >
>
https://chromiumcodereview.appspot.com/11305021/diff/12001/chrome/app/generat...
> > 
> > I think it's better to keep them consistent. Please let me know WDYT.
> 
> Ah, thanks for reminding me.  I trust Glen's opinion on UI strings more than
my
> own, so SGTM :)

Acknowledged.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
File chrome/browser/importer/external_process_importer_client.h (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/importer/...
chrome/browser/importer/external_process_importer_client.h:90:
ImporterAutofillFormDataEntry>& autofill_form_data_entry_group);
On 2014/08/20 20:47:22, Ilya Sherman wrote:
> On 2014/08/20 11:29:16, Nikhil wrote:
> > On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > > nit: Please run git cl format to fix up the formatting here, and
throughout
> > the
> > > CL.
> > 
> > Hmm, what is it that is wrong here? This is a result of "git cl format".
> 
> Ah, I see, this is wrapped as it is because the more common wrapping -- a line
> break after the opening paren -- doesn't fit on two lines.  I guess this is
ok,
> though it's very unusual looking.

Acknowledged.

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/import_data_handler.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/import_data_handler.cc:194: "autofillformdata",
On 2014/08/20 20:47:22, Ilya Sherman wrote:
> On 2014/08/20 11:29:16, Nikhil wrote:
> > On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > > nit: Even though the style above is wrong, please match it (or update all
of
> > the
> > > lines to use the more correct style).
> > 
> > This was result of "git cl format". I've matched it with above.
> 
> Yep, I think "git cl format" did the right thing for this line, but it's odd
> given that the other lines use a different format.  It would be good to fix up
> the formatting for all of these lines as a small follow-up CL.

Acknowledged.

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1139: const char kImportAutofillFormData[] =
"import_autofill_form_data";
On 2014/08/20 20:47:22, Ilya Sherman wrote:
> On 2014/08/20 11:29:17, Nikhil wrote:
> > On 2014/08/20 05:47:41, Ilya Sherman wrote:
> > > Hmm, does this only affect first-run?  Is there any reason that we'd ever
> want
> > > to automatically import form data on the first run?  That seems pretty
> > > heavy-handed to me... though maybe that's consistent with how we import on
> > first
> > > run in general.
> > 
> > I think this is only for registering preferences with user profile. As I
> > understand, this preference is used in import_data_overlay.html.
> 
> If that's so, then let's update the comment for this preference.
> 

[1] Feature works fine even if we remove this pref. This pref is not really used
in importer logic. But UI uses this exact string and sets it in pref property
(import_data_overlay.html). So, I think it might be okay to keep this preference
and set it in user pref to keep things consistent.

[2] First run, indeed, uses these preferences to decide whether to import or not
based on master preference (not sure what this is) and profile preference. But I
think additional logic is required to handle autofill form data (if we do want
to import it on first run). Also, I think current first run import logic doesn't
import saved passwords. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fir...

> > One place that I seem to have missed (on account of consistency) adding
> > kImportAutofillFormData is configuration_policy_handler_list_factory.cc.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pol...
> > 
> > Shall I add there also? 
> 
> Yeah, probably a good idea.  I'm not too familiar with the enterprise policy
> code, though.

Me neither. Anyhow, I tried to modify it but it didn't compile. Generated file
policy_constants.h didn't have kImportAutofillFormData . I'm guessing this is
something to do with Enterprise policy but I'm not sure if we need this change.

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer.cc (right):

https://codereview.chromium.org/480953002/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:400: const char* query =
On 2014/08/20 20:47:22, Ilya Sherman wrote:
> On 2014/08/20 11:29:18, Nikhil wrote:
> > On 2014/08/20 05:47:42, Ilya Sherman wrote:
> > > nit: "const char query[] =" is more const (both the characters and the
> pointer
> > > are const), and slightly more efficient.
> > 
> > Changes done for this. But there are other queries in this file which could
> also
> > use similar change (follow-up patch?).
> 
> Yes, this would be great to fix in a follow-up patch.  Thanks!

Acknowledged.

https://codereview.chromium.org/480953002/diff/40001/chrome/browser/importer/...
File chrome/browser/importer/in_process_importer_bridge.cc (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/browser/importer/...
chrome/browser/importer/in_process_importer_bridge.cc:15: #include
"chrome/common/importer/importer_autofill_form_data_entry.h"
On 2014/08/20 20:47:23, Ilya Sherman wrote:
> nit: Please alphabetize.

Hmm, this looks to be correct. 
c/c/i/imported_*
c/c/i/importer_*

https://codereview.chromium.org/480953002/diff/40001/chrome/browser/importer/...
File chrome/browser/importer/profile_writer.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/browser/importer/...
chrome/browser/importer/profile_writer.h:93: // Adds the imported autofill
entries in the autofill table.
On 2014/08/20 20:47:23, Ilya Sherman wrote:
> nit: "in the autofill table" -> "to the autofill database"

Done.

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/i...
File chrome/common/importer/importer_bridge.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/i...
chrome/common/importer/importer_bridge.h:21: struct
ImporterAutofillFormDataEntry;
On 2014/08/20 20:47:23, Ilya Sherman wrote:
> nit: Please alphabetize.

Looks to be correct or I'm completely missing something :/
Imported*
Imported*
Importer*

Should the names be changed to make them look more consistent? That being said,
importer looks to be more common than imported as prefix (importer_bridge,
importer_data_types, importer_url_row etc.).

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/p...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/p...
chrome/common/importer/profile_import_process_messages.h:335: int /* total
number of entries to be imported */)
On 2014/08/20 20:47:23, Ilya Sherman wrote:
> nit: It looks like two spaces before the opening "/*" is used pretty
> consistently in this file.

That's a result of "git cl format". I've made the change now to make this look
consistent with rest of the file.

https://codereview.chromium.org/480953002/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.h:357: extern const char kImportSearchEngine[];
On 2014/08/20 20:47:23, Ilya Sherman wrote:
> Please update the .cc file to use the same order as the .h file.

Done.

[Ilya Sherman, 2014-08-22 06:11:32]

Thanks!  LG, pending the remaining test changes.

Joao, can you comment on whether there should be enterprise changes
corresponding to this CL?  If so, could you please point Nikhil in the right
direction for which code he should be updating?  Thanks.

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/i...
File chrome/common/importer/importer_bridge.h (right):

https://codereview.chromium.org/480953002/diff/40001/chrome/common/importer/i...
chrome/common/importer/importer_bridge.h:21: struct
ImporterAutofillFormDataEntry;
On 2014/08/21 07:35:36, Nikhil wrote:
> On 2014/08/20 20:47:23, Ilya Sherman wrote:
> > nit: Please alphabetize.
> 
> Looks to be correct or I'm completely missing something :/
> Imported*
> Imported*
> Importer*
> 
> Should the names be changed to make them look more consistent? That being
said,
> importer looks to be more common than imported as prefix (importer_bridge,
> importer_data_types, importer_url_row etc.).

Yes, you're right.  I missed the difference between "Imported" and "Importer". 
Sorry! :)

[Nikhil, 2014-08-23 11:51:41]

isherman@ - Thanks for taking a look at this!

I've added a few more cases, and have also updated formhistory.sqlite
accordingly. PTAL.

Also, it will be great if you can kick-off try bots once.

Thanks!

[Joao da Silva, 2014-08-25 18:24:33]

Thanks for the heads up! There are enterprise policies for the current Import*
prefs, so it would make sense to add a new one for this pref too.

More details inline.

https://codereview.chromium.org/480953002/diff/80001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/480953002/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.h:356: extern const char kImportSavedPasswords[];
There is an enterprise policy that controls this pref (ImportSavedPasswords) as
well as the other kImport* prefs here. It would be great if you could add a new
policy for the new pref too; it can be done in a separate CL.

Here's what we need:

- look for ImportSavedPasswords in policy_templates.json and add a similar entry
for ImportAutofillFormData

- map that new policy to the new pref in
configuration_policy_handler_list_factory.cc

- map the new policy ID to a string description in histograms.xml

- update the policy mappings in chrome/test/data/policy/policy_test_cases.json

That should be it; see
http://www.chromium.org/developers/how-tos/enterprise/adding-new-policies for
more details.

[Ilya Sherman, 2014-08-25 23:50:14]

Thanks for the pointers, Joao :)

Nikhil, I've kicked off some try jobs, though I think there's a good chance that
they'll fail due to the changes to the binary SQLite file.  If they do, I can
land that manually as an individual CL, and then try again.

https://codereview.chromium.org/480953002/diff/80001/chrome/browser/importer/...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/480953002/diff/80001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:109: {"name", "john"},
I don't see any test cases testing non-ASCII characters.  Could you please add a
test case for that?

[Nikhil, 2014-08-26 10:09:33]

Thanks isherman@ and joaodasila@ for taking a look at this! 

Ilya, I've added a few more cases for testing. PTAL. When you think we have
enough cases, please submit binary file and kick off bots once :)

Thanks!

https://codereview.chromium.org/480953002/diff/80001/chrome/browser/importer/...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/480953002/diff/80001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:109: {"name", "john"},
On 2014/08/25 23:50:14, Ilya Sherman wrote:
> I don't see any test cases testing non-ASCII characters.  Could you please add
a
> test case for that?

Added a few more cases. PTAL.

https://codereview.chromium.org/480953002/diff/80001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/480953002/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.h:356: extern const char kImportSavedPasswords[];
On 2014/08/25 18:24:33, Joao da Silva wrote:
> There is an enterprise policy that controls this pref (ImportSavedPasswords)
as
> well as the other kImport* prefs here. It would be great if you could add a
new
> policy for the new pref too; it can be done in a separate CL.
> 
> Here's what we need:
> 
> - look for ImportSavedPasswords in policy_templates.json and add a similar
entry
> for ImportAutofillFormData
> 
> - map that new policy to the new pref in
> configuration_policy_handler_list_factory.cc
> 
> - map the new policy ID to a string description in histograms.xml
> 
> - update the policy mappings in chrome/test/data/policy/policy_test_cases.json
> 
> That should be it; see
> http://www.chromium.org/developers/how-tos/enterprise/adding-new-policies for
> more details.

Thanks for reviewing this! I'll update the policy as you suggested in a
follow-up CL :)

[Ilya Sherman, 2014-08-27 00:41:34]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2014-08-27 00:41:35]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2014-08-27 00:42:03]

https://codereview.chromium.org/480953002/diff/100001/chrome/browser/importer...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/480953002/diff/100001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:111: {"address",
"télévision@example.com"},
Hmm, I'm pretty sure that non-ASCII characters are supposed to be escaped in
source code files, like so:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...

[Nikhil, 2014-08-27 12:30:10]

isherman@ - I've added a couple more cases. PTAL. 

In addition to browser_tests, I tested importing non-ASCII characters through
import settings and autofill seems to works well.

PTAL. Thanks!

https://codereview.chromium.org/480953002/diff/100001/chrome/browser/importer...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/480953002/diff/100001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:111: {"address",
"télévision@example.com"},
On 2014/08/27 00:42:03, Ilya Sherman wrote:
> Hmm, I'm pretty sure that non-ASCII characters are supposed to be escaped in
> source code files, like so:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...

Done.

[Ilya Sherman, 2014-08-27 22:20:00]

LGTM, thanks.  Let's try landing this via the CQ.  If that fails, I'll try
manually landing the .sqlite file in a separate CL, and then we can land the
rest of this via the CQ.

[Ilya Sherman, 2014-08-27 22:20:54]

isherman@chromium.org changed reviewers:
+ estade@chromium.org

[Ilya Sherman, 2014-08-27 22:20:54]

+Evan for WebUI settings changes.

[Ilya Sherman, 2014-08-27 22:22:41]

isherman@chromium.org changed reviewers:
+ msw@chromium.org, tsepez@chromium.org

[Ilya Sherman, 2014-08-27 22:22:41]

+tsepez for IPC review
+msw for chrome/browser/ui/browser_ui_prefs.cc

[Evan Stade, 2014-08-27 22:32:39]

webui and resources lg, but question about the string

https://codereview.chromium.org/480953002/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/480953002/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7398: +          Autofill form data
Shouldn't this just be "form data"? what are you importing? form data. The
"Autofill" in "Autofill form data" sounds like a verb. Maybe "Form data for
Autofill". Maybe "Form history", which is more descriptive since it implies it's
just remembering what you've typed in the past, whereas "form data" could be
anything.

We should consult a UX designer.

[Ilya Sherman, 2014-08-27 22:34:55]

https://codereview.chromium.org/480953002/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/480953002/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7398: +          Autofill form data
On 2014/08/27 22:32:38, Evan Stade wrote:
> Shouldn't this just be "form data"? what are you importing? form data. The
> "Autofill" in "Autofill form data" sounds like a verb. Maybe "Form data for
> Autofill". Maybe "Form history", which is more descriptive since it implies
it's
> just remembering what you've typed in the past, whereas "form data" could be
> anything.
> 
> We should consult a UX designer.

I had the same reaction, but Glen was actually the one who proposed this string
many moons ago.

[Evan Stade, 2014-08-27 22:37:58]

On 2014/08/27 22:34:55, Ilya Sherman wrote:
>
https://codereview.chromium.org/480953002/diff/120001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/480953002/diff/120001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:7398: +          Autofill form data
> On 2014/08/27 22:32:38, Evan Stade wrote:
> > Shouldn't this just be "form data"? what are you importing? form data. The
> > "Autofill" in "Autofill form data" sounds like a verb. Maybe "Form data for
> > Autofill". Maybe "Form history", which is more descriptive since it implies
> it's
> > just remembering what you've typed in the past, whereas "form data" could be
> > anything.
> > 
> > We should consult a UX designer.
> 
> I had the same reaction, but Glen was actually the one who proposed this
string
> many moons ago.

K. lgtm. If some designer pipes up with a differing opinion we can easily change
it later.

[Tom Sepez, 2014-08-27 23:10:21]

https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...
File chrome/browser/importer/external_process_importer_client.cc (right):

https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...
chrome/browser/importer/external_process_importer_client.cc:275: if
(autofill_form_data_.size() == total_autofill_form_data_entry_count_)
What if the import process can't count?  There's no reason to trust that it
won't give more data than previously claimed in the start message.

https://codereview.chromium.org/480953002/diff/120001/chrome/common/importer/...
File chrome/common/importer/importer_autofill_form_data_entry.h (right):

https://codereview.chromium.org/480953002/diff/120001/chrome/common/importer/...
chrome/common/importer/importer_autofill_form_data_entry.h:14: public:
nit: public: is redundant for struct.

[msw, 2014-08-27 23:23:32]

chrome/browser/ui/browser_ui_prefs.cc lgtm

[Nikhil, 2014-08-28 06:55:00]

tsepez@ - I've replied to your review comment. PTAL.

Also, modified the struct as per review comment and rebase done to ToT.

Thanks!

https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...
File chrome/browser/importer/external_process_importer_client.cc (right):

https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...
chrome/browser/importer/external_process_importer_client.cc:275: if
(autofill_form_data_.size() == total_autofill_form_data_entry_count_)
On 2014/08/27 23:10:21, Tom Sepez wrote:
> What if the import process can't count?  There's no reason to trust that it
> won't give more data than previously claimed in the start message.

Sorry, but I don't fully understand the comment so I'm unsure about the change
required. Could you please explain a little more?

The count is actually maintained by Importer utility based on number of enteries
imported from Firefox. Enteries are sent in groups through IPC. The count of
enteris left to be sent is then updated accordingly (and same is repeated until
all enteries have been sent). Since the count is maintained consistently, I
think it is not possible to send data more than what was initially claimed in
start message.

PTAL at
https://codereview.chromium.org/480953002/diff/120001/chrome/utility/importer...

Please let me know WDYT.

https://codereview.chromium.org/480953002/diff/120001/chrome/common/importer/...
File chrome/common/importer/importer_autofill_form_data_entry.h (right):

https://codereview.chromium.org/480953002/diff/120001/chrome/common/importer/...
chrome/common/importer/importer_autofill_form_data_entry.h:14: public:
On 2014/08/27 23:10:21, Tom Sepez wrote:
> nit: public: is redundant for struct.

Done.

[Tom Sepez, 2014-08-28 16:54:42]

The comparison == feels frail against future bugs in the utility process.  You
ought to handle the > case in some manner, perhaps shooting the importer.

[Ilya Sherman, 2014-08-30 01:30:19]

https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...
File chrome/browser/importer/external_process_importer_client.cc (right):

https://codereview.chromium.org/480953002/diff/120001/chrome/browser/importer...
chrome/browser/importer/external_process_importer_client.cc:275: if
(autofill_form_data_.size() == total_autofill_form_data_entry_count_)
On 2014/08/28 06:54:59, Nikhil wrote:
> On 2014/08/27 23:10:21, Tom Sepez wrote:
> > What if the import process can't count?  There's no reason to trust that it
> > won't give more data than previously claimed in the start message.
> 
> Sorry, but I don't fully understand the comment so I'm unsure about the change
> required. Could you please explain a little more?
> 
> The count is actually maintained by Importer utility based on number of
enteries
> imported from Firefox. Enteries are sent in groups through IPC. The count of
> enteris left to be sent is then updated accordingly (and same is repeated
until
> all enteries have been sent). Since the count is maintained consistently, I
> think it is not possible to send data more than what was initially claimed in
> start message.
> 
> PTAL at
>
https://codereview.chromium.org/480953002/diff/120001/chrome/utility/importer...
> 
> Please let me know WDYT.

FWIW, all the other importer types use "==" as the comparison.  Perhaps it's
best to match those for now, and to file a follow-up bug to update this
comparison for all of the importer types?  Tom, WDYT?

[Tom Sepez, 2014-08-30 01:43:16]

> FWIW, all the other importer types use "==" as the comparison.  Perhaps it's
> best to match those for now, and to file a follow-up bug to update this
> comparison for all of the importer types?  Tom, WDYT?
Sounds fine to me.  I don't see a reason to hold this up further over this
issue. LGTM.

[Nikhil, 2014-08-30 09:37:53]

Thank you all for the quick reviews! I'm going to submit this now, after rebase
:)

[Nikhil, 2014-08-30 10:41:07]

Rebase patch! Going to submit this now.

Ilya, if this fails to submit because of SQLite file then please help me with it
:)

Thanks!

[Nikhil, 2014-08-30 10:41:23]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-08-30 10:41:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/480953002/160001

[commit-bot: I haz the power, 2014-08-30 11:47:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-30 11:58:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-30 11:58:42]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Nikhil, 2014-08-30 12:30:12]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-08-30 12:31:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/480953002/160001

[commit-bot: I haz the power, 2014-08-30 12:36:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2014-08-30 12:48:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-30 12:48:49]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Nikhil, 2014-09-02 06:36:45]

Rebase patch.

[Nikhil, 2014-09-02 06:37:02]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-09-02 06:37:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/480953002/180001

[commit-bot: I haz the power, 2014-09-02 07:33:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2014-09-02 07:56:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-02 07:56:46]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Nikhil, 2014-09-02 08:46:14]

isherman@ - Do we need to manually submit SQLite file? mac_chromium_compile_dbg
seems to fail consistently.

[Evan Stade, 2014-09-02 15:54:55]

On 2014/09/02 08:46:14, Nikhil wrote:
> isherman@ - Do we need to manually submit SQLite file?
mac_chromium_compile_dbg
> seems to fail consistently.

I suspect so. You can create a separate CL with just the sqlite file and
NOTRY=true

[Ilya Sherman, 2014-09-02 20:22:37]

Actually, what's failing is the compilation.  More specifically, the linking
step is failing; I've copy/pasted the output below.  FYI, if you're not already
using it, you might want to install the BuildBot Error extension [1] -- it's
quite handy for Chromium development.  As long as I'm recommending handy
extensions, you might also want to try out Chromite Butler [2].

[1]
https://chrome.google.com/webstore/detail/buildbot-error/iehocdgbbocmkdidlbnn...
[2]
https://chrome.google.com/webstore/detail/chromite-butler/bhcnanendmgjjeghama...



Build output:

FAILED: /Volumes/data/b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ [... snipped ...] -o
unit_tests [... snipped ...]
duplicate symbol
__ZN35ProfileImportProcessMsg_StartImportC2ERKN8importer13SourceProfileERKiRKN4base15DictionaryValueE
in:
    obj/chrome/utility/importer/unit_tests.firefox_importer_unittest_utils_mac.o
    libcommon.a(common.profile_import_process_messages.o)

https://codereview.chromium.org/480953002/diff/180001/chrome/common/importer/...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/180001/chrome/common/importer/...
chrome/common/importer/profile_import_process_messages.h:266:
IPC_STRUCT_TRAITS_END()
It looks like this should be defined slightly earlier in the file, possible
before line 256.

[Nikhil, 2014-09-03 04:36:27]

Ilya, please see my response to your comment. 

Also, Thanks for suggesting extensions for Chromium development. I wasn't aware
of them. They look quite useful.

https://codereview.chromium.org/480953002/diff/180001/chrome/common/importer/...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/180001/chrome/common/importer/...
chrome/common/importer/profile_import_process_messages.h:266:
IPC_STRUCT_TRAITS_END()
On 2014/09/02 20:22:37, Ilya Sherman wrote:
> It looks like this should be defined slightly earlier in the file, possible
> before line 256.

If I do that, then I get linker errors locally for Read and Write methods.
Please see -

error: undefined reference to
'IPC::ParamTraits<ImporterAutofillFormDataEntry>::Write(IPC::Message*,
ImporterAutofillFormDataEntry const&)'

error: undefined reference to
'IPC::ParamTraits<ImporterAutofillFormDataEntry>::Read(IPC::Message const*,
PickleIterator*, ImporterAutofillFormDataEntry*)'

How about - 

[1] Go to initial approach of packing/unpacking message without using
IPC_STRUCT_TRAITS_* macros for now. I'm guessing mac bot will compile.
(https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...)

[2] I update this whole file to use IPC_STRUCT_TRAITS_* macros. Similar to
print_messages.h.
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/prin...)

If you think doing [2] first makes more sense, then we can do that also. Please
let me know WDYT.

[Ilya Sherman, 2014-09-03 17:48:25]

On 2014/09/03 04:36:27, Nikhil wrote:
> Ilya, please see my response to your comment. 
> 
> Also, Thanks for suggesting extensions for Chromium development. I wasn't
aware
> of them. They look quite useful.
> 
>
https://codereview.chromium.org/480953002/diff/180001/chrome/common/importer/...
> File chrome/common/importer/profile_import_process_messages.h (right):
> 
>
https://codereview.chromium.org/480953002/diff/180001/chrome/common/importer/...
> chrome/common/importer/profile_import_process_messages.h:266:
> IPC_STRUCT_TRAITS_END()
> On 2014/09/02 20:22:37, Ilya Sherman wrote:
> > It looks like this should be defined slightly earlier in the file, possible
> > before line 256.
> 
> If I do that, then I get linker errors locally for Read and Write methods.
> Please see -
> 
> error: undefined reference to
> 'IPC::ParamTraits<ImporterAutofillFormDataEntry>::Write(IPC::Message*,
> ImporterAutofillFormDataEntry const&)'
> 
> error: undefined reference to
> 'IPC::ParamTraits<ImporterAutofillFormDataEntry>::Read(IPC::Message const*,
> PickleIterator*, ImporterAutofillFormDataEntry*)'
> 
> How about - 
> 
> [1] Go to initial approach of packing/unpacking message without using
> IPC_STRUCT_TRAITS_* macros for now. I'm guessing mac bot will compile.
>
(https://codereview.chromium.org/480953002/diff/20001/chrome/common/importer/p...)
> 
> [2] I update this whole file to use IPC_STRUCT_TRAITS_* macros. Similar to
> print_messages.h.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/prin...)
> 
> If you think doing [2] first makes more sense, then we can do that also.
Please
> let me know WDYT.

Option [1] sounds fine for now -- thanks.

[Tom Sepez, 2014-09-03 17:50:34]

> Option [1] sounds fine for now -- thanks.
Yes, I also think that option [2] should be separate patch.

[Nikhil, 2014-09-04 04:48:55]

Patchset #11 (id:200001) has been deleted

[Nikhil, 2014-09-04 05:03:18]

isherman@, tsepez@ - I've updated the patch to remove IPC_STRUCT_TRAITS_*
macros. Hopefully, Mac bot will agree now. PTAL.

I'll update profile_import_process_messages.h to use IPC_STRUCT_TRAITS_* macros
once this patch lands.

Thanks!

[Ilya Sherman, 2014-09-04 06:17:47]

https://codereview.chromium.org/480953002/diff/220001/chrome/common/importer/...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/220001/chrome/common/importer/...
chrome/common/importer/profile_import_process_messages.h:253: return true;
I think you can simplify this to look more like the Read() function
corresponding to ImportedBookmarkEntry.

[Nikhil, 2014-09-04 06:22:03]

On 2014/09/04 06:17:47, Ilya Sherman wrote:
>
https://codereview.chromium.org/480953002/diff/220001/chrome/common/importer/...
> File chrome/common/importer/profile_import_process_messages.h (right):
> 
>
https://codereview.chromium.org/480953002/diff/220001/chrome/common/importer/...
> chrome/common/importer/profile_import_process_messages.h:253: return true;
> I think you can simplify this to look more like the Read() function
> corresponding to ImportedBookmarkEntry.

Could you please kick-off try bots once? Thanks!

[Nikhil, 2014-09-04 06:34:30]

Simplified the Read() function. PTAL. 

I verified the changes manually and feature is working fine.

Thanks!

https://codereview.chromium.org/480953002/diff/220001/chrome/common/importer/...
File chrome/common/importer/profile_import_process_messages.h (right):

https://codereview.chromium.org/480953002/diff/220001/chrome/common/importer/...
chrome/common/importer/profile_import_process_messages.h:253: return true;
On 2014/09/04 06:17:47, Ilya Sherman wrote:
> I think you can simplify this to look more like the Read() function
> corresponding to ImportedBookmarkEntry.

Done.

[Tom Sepez, 2014-09-04 17:51:25]

messages LGTM.

[Ilya Sherman, 2014-09-04 18:14:29]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-04 18:18:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/480953002/240001

[commit-bot: I haz the power, 2014-09-04 20:21:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 20:21:05]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ilya Sherman, 2014-09-04 22:29:51]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-04 22:36:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/480953002/240001

[commit-bot: I haz the power, 2014-09-05 00:37:52]

Committed patchset #12 (id:240001) as 07d10297344a4c1993aa5a713dfb03dd380837cf

[commit-bot: I haz the power, 2014-09-10 03:34:18]

Patchset 12 (id:??) landed as
https://crrev.com/884eafca142af3f8e8ed252048edeeb5c3d25319
Cr-Commit-Position: refs/heads/master@{#293381}