Implements SendAutocheckoutStatus API calls for stats tracking.

Implements SendAutocheckoutStatus API calls for stats tracking.


BUG=224159
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191445

[ahutter, 2013-03-26 17:39:07]

Ilya, PTAL. Thanks.

[Ilya Sherman, 2013-03-26 23:50:01]

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autocheckout_manager.cc (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:133:
google_transaction_id_);
nit: DCHECK that the transaction_id_ is set?

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:182:
google_transaction_id_);
nit: Decompose this code into a shared method rather than repeating it.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:204:
google_transaction_id_);
Ditto.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:306:
google_transaction_id_);
Ditto

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autocheckout_request_manager.cc (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.cc:1: 
License block goes here.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.cc:97: // This doesn't
really do anything. Should I just remove it?
Yes, let's remove it if it's not used.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.cc:114: // I think this
is already logged somewhere...
Indeed, logged by the MetricLogger.  Please update this comment to be less
ambivalent ;)

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.cc:122: // I think this
is already logged somewhere...
Ditto.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autocheckout_request_manager.h (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.h:37:
content::BrowserContext* browser_context);
nit: Docs

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:851: const std::string&
not_used) {
nit: "not_used" -> "unused_transaction_id"

[ahutter, 2013-03-27 01:23:33]

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autocheckout_manager.cc (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:133:
google_transaction_id_);
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> nit: DCHECK that the transaction_id_ is set?

By set do you mean non-empty? Because empty is ok.  That's what will happen if
the user chooses to Autocheckout with local data.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:182:
google_transaction_id_);
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> nit: Decompose this code into a shared method rather than repeating it.

Done.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:204:
google_transaction_id_);
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> Ditto.

Done.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:306:
google_transaction_id_);
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> Ditto

Done.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autocheckout_request_manager.cc (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.cc:1: 
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> License block goes here.

Done.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.cc:97: // This doesn't
really do anything. Should I just remove it?
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> Yes, let's remove it if it's not used.

Done.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.cc:114: // I think this
is already logged somewhere...
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> Indeed, logged by the MetricLogger.  Please update this comment to be less
> ambivalent ;)

Done.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.cc:122: // I think this
is already logged somewhere...
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> Ditto.

Done.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autocheckout_request_manager.h (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_request_manager.h:37:
content::BrowserContext* browser_context);
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> nit: Docs

Done.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:851: const std::string&
not_used) {
On 2013/03/26 23:50:01, Ilya Sherman wrote:
> nit: "not_used" -> "unused_transaction_id"

Done.

[Ilya Sherman, 2013-03-27 01:36:55]

LGTM, thanks.

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
File components/autofill/browser/autocheckout_manager.cc (right):

https://codereview.chromium.org/12457033/diff/2001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.cc:133:
google_transaction_id_);
On 2013/03/27 01:23:33, ahutter wrote:
> On 2013/03/26 23:50:01, Ilya Sherman wrote:
> > nit: DCHECK that the transaction_id_ is set?
> 
> By set do you mean non-empty? Because empty is ok.  That's what will happen if
> the user chooses to Autocheckout with local data.

Maybe you should initialize the transaction id to some arbitrary string constant
kTransactionIdNotSet, and DCHECK that was changed away from that?  It just seems
good to make sure that you're not reading stale data.

[Ilya Sherman, 2013-03-27 01:37:11]

Oh, and please add a bug id to the BUG= line in the CL description.

[ahutter, 2013-03-27 19:30:48]

+sky for chrome/chrome_browser.gypi

Ilya, PTAL. The DCHECK you suggested exposed a bug in autofill_agent so I made a
few minor changes.

Thanks.

https://chromiumcodereview.appspot.com/12457033/diff/2001/components/autofill...
File components/autofill/browser/autocheckout_manager.cc (right):

https://chromiumcodereview.appspot.com/12457033/diff/2001/components/autofill...
components/autofill/browser/autocheckout_manager.cc:133:
google_transaction_id_);
On 2013/03/27 01:36:55, Ilya Sherman wrote:
> On 2013/03/27 01:23:33, ahutter wrote:
> > On 2013/03/26 23:50:01, Ilya Sherman wrote:
> > > nit: DCHECK that the transaction_id_ is set?
> > 
> > By set do you mean non-empty? Because empty is ok.  That's what will happen
if
> > the user chooses to Autocheckout with local data.
> 
> Maybe you should initialize the transaction id to some arbitrary string
constant
> kTransactionIdNotSet, and DCHECK that was changed away from that?  It just
seems
> good to make sure that you're not reading stale data.

Done.

[ahutter, 2013-03-27 19:31:13]

+sky for realz this time.

[Ilya Sherman, 2013-03-27 22:27:21]

https://chromiumcodereview.appspot.com/12457033/diff/30002/components/autofil...
File components/autofill/renderer/autofill_agent.cc (left):

https://chromiumcodereview.appspot.com/12457033/diff/30002/components/autofil...
components/autofill/renderer/autofill_agent.cc:228: if (provisional_url !=
current_url && click_timer_.IsRunning()) {
Hmm, why is this check no longer relevant?

https://chromiumcodereview.appspot.com/12457033/diff/30002/components/autofil...
File components/autofill/renderer/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/12457033/diff/30002/components/autofil...
components/autofill/renderer/autofill_agent.cc:231: if (!frame->parent() &&
autocheckout_click_in_progress_) {
How do you know that all clicks in Autocheckout will always navigate the top
frame?  Though I suppose that if this isn't a problem for now, we can just hold
off on thinking about it until it comes up.

[Ilya Sherman, 2013-03-27 22:35:08]

LGTM

https://chromiumcodereview.appspot.com/12457033/diff/30002/components/autofil...
File components/autofill/renderer/autofill_agent.cc (left):

https://chromiumcodereview.appspot.com/12457033/diff/30002/components/autofil...
components/autofill/renderer/autofill_agent.cc:228: if (provisional_url !=
current_url && click_timer_.IsRunning()) {
On 2013/03/27 22:27:21, Ilya Sherman wrote:
> Hmm, why is this check no longer relevant?

Per IM discussion, this is now handled on the browser side, so it doesn't need
to be on the renderer.

[ahutter, 2013-03-27 22:41:44]

https://chromiumcodereview.appspot.com/12457033/diff/30002/components/autofil...
File components/autofill/renderer/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/12457033/diff/30002/components/autofil...
components/autofill/renderer/autofill_agent.cc:231: if (!frame->parent() &&
autocheckout_click_in_progress_) {
On 2013/03/27 22:27:21, Ilya Sherman wrote:
> How do you know that all clicks in Autocheckout will always navigate the top
> frame?  Though I suppose that if this isn't a problem for now, we can just
hold
> off on thinking about it until it comes up.

I guess I don't but so far they have.  If/when we need to worry about changes by
frame this will all become more complicated.

[sky, 2013-03-28 16:02:58]

.gypi LGTM

[commit-bot: I haz the power, 2013-03-28 16:34:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahutter@chromium.org/12457033/39001

[commit-bot: I haz the power, 2013-03-28 16:47:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahutter@chromium.org/12457033/52001

[ahutter, 2013-03-28 19:18:53]

Ilya, can you please take one final look? The unit tests were unhappy because
TestingProfile doesn't provide a URLRequestContextGetter unless you specifically
create one.  I also am using testing_profile.h which is soon to be illegal
include... not sure what to do about that.

[Ilya Sherman, 2013-03-28 21:09:57]

On 2013/03/28 19:18:53, ahutter wrote:
> Ilya, can you please take one final look? The unit tests were unhappy because
> TestingProfile doesn't provide a URLRequestContextGetter unless you
specifically
> create one.  I also am using testing_profile.h which is soon to be illegal
> include... not sure what to do about that.

Perhaps we should be using a TestURLRequestContextGetter in tests?
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

[commit-bot: I haz the power, 2013-03-28 22:29:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahutter@chromium.org/12457033/68001

[commit-bot: I haz the power, 2013-03-28 22:59:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahutter@chromium.org/12457033/75002

[commit-bot: I haz the power, 2013-03-29 00:14:09]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-29 16:44:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahutter@chromium.org/12457033/107001

[commit-bot: I haz the power, 2013-03-29 17:27:41]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-29 18:01:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahutter@chromium.org/12457033/107001

[commit-bot: I haz the power, 2013-03-29 21:16:31]

Change committed as 191445