Refactoring of upload, form saving, and UMA logging.

Refactoring of upload, form saving, and UMA logging.

BUG=none
TEST=PersonalDataManagerTest.*, AutoFillManagerTest.*

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

[Ilya Sherman, 2011-01-18 23:02:36]

Not sure if you're intending to commit this or just uploading it for
demonstrative purposes, but:

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.cc (left):

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.cc:224: // HandleSubmit().
Are you sure that we should only upload data to the server if our heuristics
detected at least three autofillable fields?  (That's the question this TODO is
really getting at -- should've made it clearer.)

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.cc:498: cached_submitted_form = NULL;
I wonder if we can make this a NOTREACHED()

[dhollowa, 2011-01-18 23:26:15]

Just illustrative.  This is in service of
http://codereview.chromium.org/6213002/

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.cc (left):

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.cc:224: // HandleSubmit().
Yes, typo.  UploadFormData should be moved up above the test for
|IsAutoFillable|.

On 2011/01/18 23:02:36, Ilya Sherman wrote:
> Are you sure that we should only upload data to the server if our heuristics
> detected at least three autofillable fields?  (That's the question this TODO
is
> really getting at -- should've made it clearer.)

[dhollowa, 2011-01-18 23:29:03]

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.cc:498: cached_submitted_form = NULL;
I would think yes, and that would be preferable.  That's not essential to
George's work, but would be nice to clean up.

On 2011/01/18 23:02:36, Ilya Sherman wrote:
> I wonder if we can make this a NOTREACHED()

[dhollowa, 2011-01-19 17:00:44]

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.cc (left):

http://codereview.chromium.org/6286003/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.cc:224: // HandleSubmit().
On 2011/01/18 23:26:15, dhollowa wrote:
> Yes, typo.  UploadFormData should be moved up above the test for
> |IsAutoFillable|.
> 
> On 2011/01/18 23:02:36, Ilya Sherman wrote:
> > Are you sure that we should only upload data to the server if our heuristics
> > detected at least three autofillable fields?  (That's the question this TODO
> is
> > really getting at -- should've made it clearer.)
> 

Done.

[Ilya Sherman, 2011-01-20 19:37:33]

LGTM