Add url prefix to AutofillQuery requests when autocheckout enabled, and set accepts="a" (autochecko…

chrome/browser/autofill/form_structure.cc

Index: chrome/browser/autofill/form_structure.cc
diff --git a/chrome/browser/autofill/form_structure.cc b/chrome/browser/autofill/form_structure.cc
index 370ac2ab70a8526324714a1a62495e88b1bcdf67..d9e9499388d05da6399c7079cae6184194a82760 100644
--- a/chrome/browser/autofill/form_structure.cc
+++ b/chrome/browser/autofill/form_structure.cc
@@ -40,7 +40,9 @@ const char kAttributeClientVersion[] = "clientversion";
 const char kAttributeDataPresent[] = "datapresent";
 const char kAttributeFormSignature[] = "formsignature";
 const char kAttributeSignature[] = "signature";
-const char kAcceptedFeatures[] = "e"; // e=experiments
+const char kAttributeUrlprefix[] = "urlprefix";
+const char kAcceptedFeatureExperiment[] = "e"; // e=experiments
+const char kAcceptedFeatureAutocheckout[] = "a"; // a=autocheckout
 const char kClientVersion[] = "6.1.1715.1442/en (GGLL)";
 const char kXMLDeclaration[] = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>";
 const char kXMLElementAutofillQuery[] = "autofillquery";
@@ -392,9 +394,8 @@ bool FormStructure::EncodeQueryRequest(
       (buzz::QName(kXMLElementAutofillQuery)));
   autofill_request_xml.SetAttr(buzz::QName(kAttributeClientVersion),
                                kClientVersion);
-  autofill_request_xml.SetAttr(buzz::QName(kAttributeAcceptedFeatures),
-                               kAcceptedFeatures);
 
+  std::string autocheckout_urlprefix;
   // Some badly formatted web sites repeat forms - detect that and encode only
   // one form as returned data would be the same for all the repeated forms.
   std::set<std::string> processed_forms;
@@ -414,6 +415,13 @@ bool FormStructure::EncodeQueryRequest(
                                   encompassing_xml_element.get()))
       continue;  // Malformed form, skip it.
 
+    if ((*it)->autocheckout_enabled_ && autocheckout_urlprefix.empty()) {
+      GURL::Replacements replacements;
+      replacements.ClearQuery();
+      autocheckout_urlprefix = (*it)->source_url()
+          .ReplaceComponents(replacements).spec();
+    }
+
     autofill_request_xml.AddElement(encompassing_xml_element.release());
     encoded_signatures->push_back(signature);
   }
@@ -421,6 +429,16 @@ bool FormStructure::EncodeQueryRequest(
   if (!encoded_signatures->size())
     return false;
 
+  if (autocheckout_urlprefix.empty()) {
+    autofill_request_xml.SetAttr(buzz::QName(kAttributeAcceptedFeatures),
+                                 kAcceptedFeatureExperiment);
+  } else {
+    autofill_request_xml.SetAttr(buzz::QName(kAttributeAcceptedFeatures),
+                                 kAcceptedFeatureAutocheckout);

[Ilya Sherman, 2013/01/25 20:37:35]

Shouldn't we support both experiments and autocheckout in this case?

[benquan, 2013/01/29 00:36:09]

We use a different form_data in server side, when we enable  autocheckout, we do
not want to active any other experiments.

[Ilya Sherman, 2013/01/29 01:49:23]

That sounds like something that should be controlled server-side, rather than
client-side.  The client supports both autocheckout and experiments.  It should
be up to the server whether or not to serve both of those together.

[benquan, 2013/01/30 19:24:22]

Basically autocheckout and experiments are exclusive, we could let the server to
make the decision, but it does not make good sense to pass down both of them to
the server.

[Ilya Sherman, 2013/01/30 21:12:14]

What in the client code makes them necessarily exclusive?

[benquan, 2013/01/31 03:58:05]

Done.

+    autofill_request_xml.SetAttr(buzz::QName(kAttributeUrlprefix),
+                                 autocheckout_urlprefix);

[Ilya Sherman, 2013/01/25 20:37:35]

What does this attribute accomplish?  Why do we only use the URL prefix from the
first form for which Autocheckout is enabled?

[benquan, 2013/01/29 00:36:09]

urlprefix tells autofillserver where the forms in the request came from, and the
autofillserver checks internal status and decide to enable autocheckout or not
and may return autocheckout related data in the response.

autocheckout is determined by page url and the whitelist, url-prefix are same
for all forms in the page, and it does not matter which one we use. However is
no page level object available from FormStructure, we ends up using the first
one. The assumption was that we request forms from the same page only (which is
true so far).

[Ilya Sherman, 2013/01/29 01:49:23]

Ok, that makes some sense -- thanks for the explanation.  Two comments:

(1) We've been very careful in the design of the Autofill client-to-server
communication not to send any PII or URLs up to the server whenever possible. 
Instead, we always send hashes in place of URLs, and locally computed field
types in place of field contents.  Rather than sending a raw URL prefix, we
should use a hash here as well.

(2) Pretty much everything that you wrote here should be documented inline in
the code as well.  Moreover, it's best to add DCHECKs to verify that the
invariants you expect -- in this case, that all the forms share the same URL
prefix -- do hold.

[benquan, 2013/01/30 19:24:22]

The query string is stripped, and we only send this when user opt-in, and it
should be in the TOS.

If we hash the url prefix, we can not do prefix check on the server. This is in
the PDD, I'm going to start an email thread with Chrome privacy team about it.

https://docs.google.com/a/google.com/document/d/1uY1Q4R0n0YwclTd4FGRa4kCSEPkn...
I'm going to add comments.

[Ilya Sherman, 2013/01/30 21:12:14]

Why not?  If the server knows the URL, it can compute the same hash, and compare
the hashes.

[benquan, 2013/01/31 03:58:05]

Changed to pass down hash code of the url prefix in the whitelist.

+  }
+
   // Obtain the XML structure as a string.
   *encoded_xml = kXMLDeclaration;
   *encoded_xml += autofill_request_xml.Str().c_str();