Integration with Toolbar autofill servers....

Integration with Toolbar autofill servers.
Still to do:
  1. Update upload rates based on response
  2. Differentiate autofilled/not uploads
  3. Do not query if there is no items in autofill
  4. Fix forms to correspond to the data queried by toolbar
BUG=none
TEST=Should request data from toolbar servers

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

[GeorgeY, 2010-03-19 21:31:01]

First cl on Toolbar servers integration

[James Hawkins, 2010-03-19 22:10:05]

No tests?  Shame.  This is high priority so I'll let it slide, but we'll need to
heavily test this once we get it working.

http://codereview.chromium.org/1119004/diff/8001/9001
File chrome/browser/autofill/autofill_download.cc (right):

http://codereview.chromium.org/1119004/diff/8001/9001#newcode19
chrome/browser/autofill/autofill_download.cc:19: // We only send fraction of the
forms to upload server.
nit: "send a fraction of the forms to the upload server."

http://codereview.chromium.org/1119004/diff/8001/9001#newcode30
chrome/browser/autofill/autofill_download.cc:30: for (std::map<URLFetcher *,
FormRequestData>::iterator it =
Replace all of this with STLDeleteContainerPairFirstPointers.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode85
chrome/browser/autofill/autofill_download.cc:85: url_fetchers_.begin();
alignment looks off.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode98
chrome/browser/autofill/autofill_download.cc:98: // URLFetcher::Delegate
overridables.
No need for this comment.  The comment in the header suffices.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode108
chrome/browser/autofill/autofill_download.cc:108: if (it == url_fetchers_.end())
Per the ML convo about defensive code, we shouldn't return if the DCHECK
condition fails in Release, so remove this bit.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode111
chrome/browser/autofill/autofill_download.cc:111: if (response_code != 200) {
Constant for 200?

http://codereview.chromium.org/1119004/diff/8001/9002
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/1119004/diff/8001/9002#newcode19
chrome/browser/autofill/autofill_download.h:19: // Handles getting and updating
Autofill heuristics
nit: period at the end of the comment.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode20
chrome/browser/autofill/autofill_download.h:20: class AutofillDownloadManager :
public URLFetcher::Delegate {
s/Autofill/AutoFill/ here and wherever else used.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode25
chrome/browser/autofill/autofill_download.h:25: virtual void
OnLoadedAutofillHeuristics(const std::string& form_signature,
These parameters should line up.  If there's not enough room, the first
parameter should be dropped down.

virtual void OnLoaded..(
    const std::string& form_signature, const std::string& heuristic_xml

All on one line if possible, dropped below if not.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode25
chrome/browser/autofill/autofill_download.h:25: virtual void
OnLoadedAutofillHeuristics(const std::string& form_signature,
Please add comments for each of these methods.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode50
chrome/browser/autofill/autofill_download.h:50: bool query_data, bool
form_was_matched);
nit: One parameter per line.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode52
chrome/browser/autofill/autofill_download.h:52: bool CancelRequest(const
std::string& form_signature, bool query_data);
nit: Blank line before 'protected:'.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode54
chrome/browser/autofill/autofill_download.h:54: // URLFetcher::Delegate
overridables.
// URLFetcher::Delegate implementation:

http://codereview.chromium.org/1119004/diff/8001/9002#newcode66
chrome/browser/autofill/autofill_download.h:66: };
nit: Blank line after definitions.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode67
chrome/browser/autofill/autofill_download.h:67: std::map<URLFetcher *,
FormRequestData> url_fetchers_;
Add a comment for |url_fetchers_|.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode70
chrome/browser/autofill/autofill_download.h:70: 
Remove second blank line.

http://codereview.chromium.org/1119004/diff/8001/9003
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/1119004/diff/8001/9003#newcode33
chrome/browser/autofill/autofill_manager.cc:33:
download_manager_.SetObserver(this);
You don't remove the observer on destruction.  This will cause flaky crashes if
the order of destruction changes at any point.

http://codereview.chromium.org/1119004/diff/8001/9006
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/1119004/diff/8001/9006#newcode79
chrome/browser/autofill/autofill_manager.h:79: //
AutofillDownloadManager::Observer implementation
append ':'

http://codereview.chromium.org/1119004/diff/8001/9006#newcode116
chrome/browser/autofill/autofill_manager.h:116: // Handles queries and uploads
to Toolbar servers.
Let's call these 'AutoFill servers' in the code, since they're not technically
just for Toolbar anymore.

http://codereview.chromium.org/1119004/diff/8001/9005
File chrome/browser/autofill/form_structure.h (right):

http://codereview.chromium.org/1119004/diff/8001/9005#newcode40
chrome/browser/autofill/form_structure.h:40: // |query| - true means request is
a query, false otherwise.
s/false/upload/

[GeorgeY, 2010-03-20 00:08:50]

All done

http://codereview.chromium.org/1119004/diff/8001/9001
File chrome/browser/autofill/autofill_download.cc (right):

http://codereview.chromium.org/1119004/diff/8001/9001#newcode19
chrome/browser/autofill/autofill_download.cc:19: // We only send fraction of the
forms to upload server.
On 2010/03/19 22:10:05, James Hawkins wrote:
> nit: "send a fraction of the forms to the upload server."

Done.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode30
chrome/browser/autofill/autofill_download.cc:30: for (std::map<URLFetcher *,
FormRequestData>::iterator it =
On 2010/03/19 22:10:05, James Hawkins wrote:
> Replace all of this with STLDeleteContainerPairFirstPointers.

Done.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode85
chrome/browser/autofill/autofill_download.cc:85: url_fetchers_.begin();
On 2010/03/19 22:10:05, James Hawkins wrote:
> alignment looks off
Two conflicting standards: 4 characters indentation, or by parenthesis (which in
this case is 5) :). Done.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode98
chrome/browser/autofill/autofill_download.cc:98: // URLFetcher::Delegate
overridables.
On 2010/03/19 22:10:05, James Hawkins wrote:
> No need for this comment.  The comment in the header suffices.

Done.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode108
chrome/browser/autofill/autofill_download.cc:108: if (it == url_fetchers_.end())
On 2010/03/19 22:10:05, James Hawkins wrote:
> Per the ML convo about defensive code, we shouldn't return if the DCHECK
> condition fails in Release, so remove this bit.

Done.

http://codereview.chromium.org/1119004/diff/8001/9001#newcode111
chrome/browser/autofill/autofill_download.cc:111: if (response_code != 200) {
On 2010/03/19 22:10:05, James Hawkins wrote:
> Constant for 200?

Surprisingly we do not seem to have a constant for HTTP OK. Added here.

http://codereview.chromium.org/1119004/diff/8001/9002
File chrome/browser/autofill/autofill_download.h (right):

http://codereview.chromium.org/1119004/diff/8001/9002#newcode19
chrome/browser/autofill/autofill_download.h:19: // Handles getting and updating
Autofill heuristics
On 2010/03/19 22:10:05, James Hawkins wrote:
> nit: period at the end of the comment.

Done.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode20
chrome/browser/autofill/autofill_download.h:20: class AutofillDownloadManager :
public URLFetcher::Delegate {
On 2010/03/19 22:10:05, James Hawkins wrote:
> s/Autofill/AutoFill/ here and wherever else used.

Done.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode25
chrome/browser/autofill/autofill_download.h:25: virtual void
OnLoadedAutofillHeuristics(const std::string& form_signature,
On 2010/03/19 22:10:05, James Hawkins wrote:
> These parameters should line up.  If there's not enough room, the first
> parameter should be dropped down.
> 
> virtual void OnLoaded..(
>     const std::string& form_signature, const std::string& heuristic_xml
> 
> All on one line if possible, dropped below if not.

Done. Exactly 1 character does not allow one line.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode25
chrome/browser/autofill/autofill_download.h:25: virtual void
OnLoadedAutofillHeuristics(const std::string& form_signature,
On 2010/03/19 22:10:05, James Hawkins wrote:
> Please add comments for each of these methods.

Done.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode50
chrome/browser/autofill/autofill_download.h:50: bool query_data, bool
form_was_matched);
On 2010/03/19 22:10:05, James Hawkins wrote:
> nit: One parameter per line.

Done.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode52
chrome/browser/autofill/autofill_download.h:52: bool CancelRequest(const
std::string& form_signature, bool query_data);
On 2010/03/19 22:10:05, James Hawkins wrote:
> nit: Blank line before 'protected:'.

Done.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode54
chrome/browser/autofill/autofill_download.h:54: // URLFetcher::Delegate
overridables.
On 2010/03/19 22:10:05, James Hawkins wrote:
> // URLFetcher::Delegate implementation:

Done.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode66
chrome/browser/autofill/autofill_download.h:66: };
On 2010/03/19 22:10:05, James Hawkins wrote:
> nit: Blank line after definitions.

Done.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode67
chrome/browser/autofill/autofill_download.h:67: std::map<URLFetcher *,
FormRequestData> url_fetchers_;
On 2010/03/19 22:10:05, James Hawkins wrote:
> Add a comment for |url_fetchers_|.

Done.

http://codereview.chromium.org/1119004/diff/8001/9002#newcode70
chrome/browser/autofill/autofill_download.h:70: 
On 2010/03/19 22:10:05, James Hawkins wrote:
> Remove second blank line.

Done.

http://codereview.chromium.org/1119004/diff/8001/9003
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/1119004/diff/8001/9003#newcode33
chrome/browser/autofill/autofill_manager.cc:33:
download_manager_.SetObserver(this);
On 2010/03/19 22:10:05, James Hawkins wrote:
> You don't remove the observer on destruction.  This will cause flaky crashes
if
> the order of destruction changes at any point.

Done.

http://codereview.chromium.org/1119004/diff/8001/9006
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/1119004/diff/8001/9006#newcode79
chrome/browser/autofill/autofill_manager.h:79: //
AutofillDownloadManager::Observer implementation
On 2010/03/19 22:10:05, James Hawkins wrote:
> append ':'

Done.

http://codereview.chromium.org/1119004/diff/8001/9006#newcode116
chrome/browser/autofill/autofill_manager.h:116: // Handles queries and uploads
to Toolbar servers.
On 2010/03/19 22:10:05, James Hawkins wrote:
> Let's call these 'AutoFill servers' in the code, since they're not technically
> just for Toolbar anymore.

Done.

http://codereview.chromium.org/1119004/diff/8001/9005
File chrome/browser/autofill/form_structure.h (right):

http://codereview.chromium.org/1119004/diff/8001/9005#newcode40
chrome/browser/autofill/form_structure.h:40: // |query| - true means request is
a query, false otherwise.
On 2010/03/19 22:10:05, James Hawkins wrote:
> s/false/upload/

Done.

[GeorgeY, 2010-03-20 00:09:37]

As for tests I have partially completed separate CL.

[James Hawkins, 2010-03-20 17:09:25]

LGTM with the removal of the uploading bit.

http://codereview.chromium.org/1119004/diff/17001/18003
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/1119004/diff/17001/18003#newcode366
chrome/browser/autofill/autofill_manager.cc:366:
download_manager_.StartRequest(xml, upload_form_structure_->FormSignature(),
Let's wait on this.  We need to verify that we're not swamping the stats with
bad data.  We also need to alert the toolbar folks when we do turn this on.

[GeorgeY, 2010-03-22 09:19:41]

Done. Will submit when the servers are happy.

http://codereview.chromium.org/1119004/diff/17001/18003
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/1119004/diff/17001/18003#newcode366
chrome/browser/autofill/autofill_manager.cc:366:
download_manager_.StartRequest(xml, upload_form_structure_->FormSignature(),
On 2010/03/20 17:09:25, James Hawkins wrote:
> Let's wait on this.  We need to verify that we're not swamping the stats with
> bad data.  We also need to alert the toolbar folks when we do turn this on.

Done.