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

Add url prefix to AutofillQuery requests when autocheckout enabled, and set accepts="a" (autocheckout) rather than "e" (experiment)

BUG=172143, 172186
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181845

[benquan, 2013-01-25 03:15:06]

this depends on a pending cl: https://codereview.chromium.org/11867025/

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

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:437: kAcceptedFeatureAutocheckout);
Shouldn't we support both experiments and autocheckout in this case?

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:439: autocheckout_urlprefix);
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:08]

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:437: kAcceptedFeatureAutocheckout);
On 2013/01/25 20:37:35, Ilya Sherman wrote:
> Shouldn't we support both experiments and autocheckout in this case?

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

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:439: autocheckout_urlprefix);
On 2013/01/25 20:37:35, Ilya Sherman wrote:
> What does this attribute accomplish?  Why do we only use the URL prefix from
the
> first form for which Autocheckout is enabled?

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:22]

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:437: kAcceptedFeatureAutocheckout);
On 2013/01/29 00:36:09, benquan wrote:
> On 2013/01/25 20:37:35, Ilya Sherman wrote:
> > Shouldn't we support both experiments and autocheckout in this case?
> 
> We use a different form_data in server side, when we enable  autocheckout, we
do
> not want to active any other experiments.

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.

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:439: autocheckout_urlprefix);
On 2013/01/29 00:36:09, benquan wrote:
> On 2013/01/25 20:37:35, Ilya Sherman wrote:
> > What does this attribute accomplish?  Why do we only use the URL prefix from
> the
> > first form for which Autocheckout is enabled?
> 
> 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).

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]

I've messed up this git branch, I started a new one
(https://codereview.chromium.org/12089035/), and I'm going to delete this cl,
please use the new cl instead.

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:437: kAcceptedFeatureAutocheckout);
On 2013/01/29 01:49:23, Ilya Sherman wrote:
> On 2013/01/29 00:36:09, benquan wrote:
> > On 2013/01/25 20:37:35, Ilya Sherman wrote:
> > > Shouldn't we support both experiments and autocheckout in this case?
> > 
> > We use a different form_data in server side, when we enable  autocheckout,
we
> do
> > not want to active any other experiments.
> 
> 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.

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.

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:439: autocheckout_urlprefix);
On 2013/01/29 01:49:23, Ilya Sherman wrote:
> On 2013/01/29 00:36:09, benquan wrote:
> > On 2013/01/25 20:37:35, Ilya Sherman wrote:
> > > What does this attribute accomplish?  Why do we only use the URL prefix
from
> > the
> > > first form for which Autocheckout is enabled?
> > 
> > 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).
> 
> 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.

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...

> 
> (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.

I'm going to add comments.

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

On 2013/01/30 19:24:22, benquan wrote:
> I've messed up this git branch, I started a new one
> (https://codereview.chromium.org/12089035/), and I'm going to delete this cl,
> please use the new cl instead.

Please don't create a new CL.  Instead, on your new git branch, run the command
"git cl issue 11953100" to associate that branch with this CL.  That way, all of
the review history is preserved.  Thanks.

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:437: kAcceptedFeatureAutocheckout);
On 2013/01/30 19:24:22, benquan wrote:
> On 2013/01/29 01:49:23, Ilya Sherman wrote:
> > On 2013/01/29 00:36:09, benquan wrote:
> > > On 2013/01/25 20:37:35, Ilya Sherman wrote:
> > > > Shouldn't we support both experiments and autocheckout in this case?
> > > 
> > > We use a different form_data in server side, when we enable  autocheckout,
> we
> > do
> > > not want to active any other experiments.
> > 
> > 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.
> 
> 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.

What in the client code makes them necessarily exclusive?

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:439: autocheckout_urlprefix);
On 2013/01/30 19:24:22, benquan wrote:
> On 2013/01/29 01:49:23, Ilya Sherman wrote:
> > On 2013/01/29 00:36:09, benquan wrote:
> > > On 2013/01/25 20:37:35, Ilya Sherman wrote:
> > > > What does this attribute accomplish?  Why do we only use the URL prefix
> from
> > > the
> > > > first form for which Autocheckout is enabled?
> > > 
> > > 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).
> > 
> > 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.
> 
> 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.

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

> 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...
> 
> > 
> > (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.
> 
> I'm going to add comments.
>

[benquan_google.com, 2013-01-30 23:55:00]

On Wed, Jan 30, 2013 at 1:12 PM, <isherman@chromium.org> wrote:

> On 2013/01/30 19:24:22, benquan wrote:
>
>> I've messed up this git branch, I started a new one
>>
(https://codereview.chromium.**org/12089035/<https://codereview.chromium.org/1...),
>> and I'm going to delete this cl,
>> please use the new cl instead.
>>
>
> Please don't create a new CL.  Instead, on your new git branch, run the
> command
> "git cl issue 11953100" to associate that branch with this CL.  That way,
> all of
> the review history is preserved.  Thanks.

Thanks for the tips, I will try that.

>
>
>
> https://codereview.chromium.**org/11953100/diff/1/chrome/**
>
browser/autofill/form_**structure.cc<https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_structure.cc>
> File chrome/browser/autofill/form_**structure.cc (right):
>
> https://codereview.chromium.**org/11953100/diff/1/chrome/**
>
browser/autofill/form_**structure.cc#newcode437<https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_structure.cc#newcode437>
> chrome/browser/autofill/form_**structure.cc:437:
> kAcceptedFeatureAutocheckout);
> On 2013/01/30 19:24:22, benquan wrote:
>
>> On 2013/01/29 01:49:23, Ilya Sherman wrote:
>> > On 2013/01/29 00:36:09, benquan wrote:
>> > > On 2013/01/25 20:37:35, Ilya Sherman wrote:
>> > > > Shouldn't we support both experiments and autocheckout in this
>>
> case?
>
>> > >
>> > > We use a different form_data in server side, when we enable
>>
> autocheckout,
>
>> we
>> > do
>> > > not want to active any other experiments.
>> >
>> > 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.
>>
>
>  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.
>>
>
> What in the client code makes them necessarily exclusive?

For autocheckout forms, we manually craft formdata and field types and put
then in autofillserver sstable as a separate entry, in that entry we also
have autocheckout related data like page_number and continue button etc.
Here, the client intent to start autocheckoout, but experiment feature
would not give you autocheckout data at all, in that sense, they are
exclusive. I can still pass "e,a" to the server, but the server will always
ignore "e", if this makes sense, I can do that. That would need to adjust
server side logic a little bit.

>
>
> https://codereview.chromium.**org/11953100/diff/1/chrome/**
>
browser/autofill/form_**structure.cc#newcode439<https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_structure.cc#newcode439>
> chrome/browser/autofill/form_**structure.cc:439: autocheckout_urlprefix);
> On 2013/01/30 19:24:22, benquan wrote:
>
>> On 2013/01/29 01:49:23, Ilya Sherman wrote:
>> > On 2013/01/29 00:36:09, benquan wrote:
>> > > On 2013/01/25 20:37:35, Ilya Sherman wrote:
>> > > > What does this attribute accomplish?  Why do we only use the URL
>>
> prefix
>
>> from
>> > > the
>> > > > first form for which Autocheckout is enabled?
>> > >
>> > > 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).
>> >
>> > 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.
>>
>
>  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.
>>
>
> Why not?  If the server knows the URL, it can compute the same hash, and
> compare the hashes.

 The server don't know how to cut the prefix, so the server has to use
string::StartsWith() to match the url, which can not be done using hash.
For example, the client sends https://abc.com/checkout/ShippingInfo, or
https://abc.com/checkout/PaymentInfo to autofillserver, and the whitelist
in the server contains just "https://abc.com/checkout/", hash of these two
strings does not match.

>
>
>  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/**
>
1uY1Q4R0n0YwclTd4FGRa4kCSEPknk**Bza_KZf2stP0Gg/edit<https://docs.google.com/a/google.com/document/d/1uY1Q4R0n0YwclTd4FGRa4kCSEPknkBza_KZf2stP0Gg/edit>
>
>  >
>> > (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.
>>
>
>  I'm going to add comments.
>>
>
>
>
https://codereview.chromium.**org/11953100/<https://codereview.chromium.org/1...
>

[benquan, 2013-01-31 03:58:04]

PTAL

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:437: kAcceptedFeatureAutocheckout);
On 2013/01/30 21:12:14, Ilya Sherman wrote:
> On 2013/01/30 19:24:22, benquan wrote:
> > On 2013/01/29 01:49:23, Ilya Sherman wrote:
> > > On 2013/01/29 00:36:09, benquan wrote:
> > > > On 2013/01/25 20:37:35, Ilya Sherman wrote:
> > > > > Shouldn't we support both experiments and autocheckout in this case?
> > > > 
> > > > We use a different form_data in server side, when we enable 
autocheckout,
> > we
> > > do
> > > > not want to active any other experiments.
> > > 
> > > 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.
> > 
> > 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.
> 
> What in the client code makes them necessarily exclusive?

Done.

https://codereview.chromium.org/11953100/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:439: autocheckout_urlprefix);
On 2013/01/30 21:12:14, Ilya Sherman wrote:
> On 2013/01/30 19:24:22, benquan wrote:
> > On 2013/01/29 01:49:23, Ilya Sherman wrote:
> > > On 2013/01/29 00:36:09, benquan wrote:
> > > > On 2013/01/25 20:37:35, Ilya Sherman wrote:
> > > > > What does this attribute accomplish?  Why do we only use the URL
prefix
> > from
> > > > the
> > > > > first form for which Autocheckout is enabled?
> > > > 
> > > > 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).
> > > 
> > > 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.
> > 
> > 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.
> 
> Why not?  If the server knows the URL, it can compute the same hash, and
compare
> the hashes.
> 
> > 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...
> > 
> > > 
> > > (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.
> > 
> > I'm going to add comments.
> > 
> 

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

[Ilya Sherman, 2013-01-31 05:03:28]

https://chromiumcodereview.appspot.com/11953100/diff/8004/chrome/browser/auto...
File chrome/browser/autofill/form_structure.cc (right):

https://chromiumcodereview.appspot.com/11953100/diff/8004/chrome/browser/auto...
chrome/browser/autofill/form_structure.cc:382: // autocheckout_url_prefix tells
autofillserver where the forms in the request
nit: "tells autofillserver" -> "tells the Autofill server"

https://chromiumcodereview.appspot.com/11953100/diff/8004/chrome/browser/auto...
chrome/browser/autofill/form_structure.cc:394: replacements.ClearQuery();
nit: Remove these; they don't seem to be used.

[Ilya Sherman, 2013-01-31 05:04:54]

LGTM with those nits addressed (though please wait for the CL this depends on to
land prior to landing this one).

[benquan, 2013-02-01 02:55:04]

https://codereview.chromium.org/11953100/diff/8004/chrome/browser/autofill/fo...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11953100/diff/8004/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure.cc:382: // autocheckout_url_prefix tells
autofillserver where the forms in the request
On 2013/01/31 05:03:28, Ilya Sherman wrote:
> nit: "tells autofillserver" -> "tells the Autofill server"

Done.

https://codereview.chromium.org/11953100/diff/8004/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure.cc:394: replacements.ClearQuery();
On 2013/01/31 05:03:28, Ilya Sherman wrote:
> nit: Remove these; they don't seem to be used.

Done.

[commit-bot: I haz the power, 2013-02-12 01:45:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benquan@chromium.org/11953100/18001

[commit-bot: I haz the power, 2013-02-12 04:45:15]

Change committed as 181845