[Autofill] Send Autofill upload when active form loses focus.

[Autofill] Send Autofill upload when active form loses focus.

This is in addition to uploading on submission events. We now distinguish the two, with TODOs to have separate metrics and a bit in the payload. I will address the TODOs in subsequent CLs to keep complexity low.

The idea is to upload votes for any entered data (albeit lower quality, which we will distinguish on the server side).

Note: I moved OnFormsSeen and OnFormSubmitted because they were out of order with the .h

BUG=555015
TEST=AutofillManagerTest,FormAutocompleteTest

Committed: https://crrev.com/8ac68ce427132cd62ebc101ad08dd9c82f3eeb29
Cr-Commit-Position: refs/heads/master@{#363795}

[Mathieu, 2015-12-04 21:36:57]

Description was changed from

==========
[Autofill] Send Autofill upload when active form loses focus.

This is in addition to uploading on submission events. We now distinguish the
two, with TODOs to have separate metrics and a bit in the payload. I will
address the TODOs in subsequent CLs to keep complexity low.

The idea is to upload votes for any entered data (albeit lower quality, which we
will distinguish on the server side).

BUG=555015
TEST=AutofillManagerTest,FormAutocompleteTest
==========

to

==========
[Autofill] Send Autofill upload when active form loses focus.

This is in addition to uploading on submission events. We now distinguish the
two, with TODOs to have separate metrics and a bit in the payload. I will
address the TODOs in subsequent CLs to keep complexity low.

The idea is to upload votes for any entered data (albeit lower quality, which we
will distinguish on the server side).

Note: I moved OnFormsSeen and OnFormSubmitted because they were out of order
with the .h

BUG=555015
TEST=AutofillManagerTest,FormAutocompleteTest
==========

[Mathieu, 2015-12-04 21:38:29]

mathp@chromium.org changed reviewers:
+ vabr@chromium.org

[Mathieu, 2015-12-04 21:41:09]

Hello Václav, 

I tried to keep most of the logic in the browser, but focus handling pretty much
had to be in the renderer. Let me know if you agree, I've been pretty close to
this change for the past days so an external opinion will be good.

PTAL!

[vabr (Chromium), 2015-12-07 09:47:34]

Hi Mathieu,

This LGTM, with some comments.

Thanks!
Vaclav

https://codereview.chromium.org/1494373003/diff/1/chrome/renderer/autofill/fo...
File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right):

https://codereview.chromium.org/1494373003/diff/1/chrome/renderer/autofill/fo...
chrome/renderer/autofill/form_autocomplete_browsertest.cc:418:
EXPECT_TRUE(focus_no_longer_on_form_message != NULL);
nit: NULL -> nullptr, or drop the "!= NULL" completely. You may also consider
calling GetFirstMessageMatching directly from within EXPECT_TRUE, because the
variable name is duplicated by the message type name anyway, and the code will
be shorter.

https://codereview.chromium.org/1494373003/diff/1/chrome/renderer/autofill/fo...
chrome/renderer/autofill/form_autocomplete_browsertest.cc:453:
EXPECT_TRUE(focus_no_longer_on_form_message != NULL);
Consider my comment for line 418 here as well.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:246: for (size_t i = 0; i <
forms.size(); ++i) {
A range-based for-loop would be easier to read:
for (const auto& form : forms) {
  forms_loaded_timestamps_[form] = timestamp;
}

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:279:
StartUploadProcess(submitted_form.Pass(), timestamp, true);
nit: std::move(submitted_form) instead of .Pass()

(Note that the documentation in base/memory/scoped_ptr.h is updated to no longer
mention Pass(). The goal is to make scoped_ptr behave as close to unique_ptr as
possible, AFAIU.)

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:344:
base::Owned(form_structure.release()), loaded_timestamp,
Please use base::Passed(&form_structure) instead.
Also update line 334 once you do.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:363: // We copy
|pending_form_data_| to a new FormStructure to be consumed by the
Why not just pass |pending_form_data_| to StartUploadProcess? This copy seems
unnecessary.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:368:
StartUploadProcess(upload_form.Pass(), base::TimeTicks::Now(), false);
Pass -> std::move

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1052: // TODO(mathp):
Differentiate non-submission uploads in the uploaded payload.
optional nit: Could you use TODO(crbug.com/xxxxxxx) instead? While it is very
likely that this TODO will be addressed by you :), it won't hurt to make the
context more accessible to everybody.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1093: if
(pending_form_data_) {
ProcessPendingFormForUpload seems to be currently tolerant to pending_form_data_
being null. Did you consider either making a non-null pending_form_data_ part of
the calling contract (and keeping the null checks at callsites), or dropping the
callsite null checks?

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1096:
pending_form_data_.reset();
There seems to be no way that ProcessPendingFormForUpload would leave
|pending_form_data_| non-empty. I suggest dropping this line, or replacing it
with DCHECK(!pending_form_data_); to make that contract more explicit.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager_unittest.cc:3392: // We will
expect these types in the upload and no observed submission.
nit: Could you also point out in the comments, which line below leads to
verifying those checks? It is not obvious, and I'm fearing that somebody might
remove that by accident in the future, without causing any test breakage.

[Mathieu, 2015-12-07 14:23:07]

mathp@chromium.org changed reviewers:
+ tsepez@chromium.org

[Mathieu, 2015-12-07 14:23:08]

+tsepez for autofill_messages.h

Hello Václav,

There is still an open question in autofill_manager, have a look.

https://codereview.chromium.org/1494373003/diff/1/chrome/renderer/autofill/fo...
File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right):

https://codereview.chromium.org/1494373003/diff/1/chrome/renderer/autofill/fo...
chrome/renderer/autofill/form_autocomplete_browsertest.cc:418:
EXPECT_TRUE(focus_no_longer_on_form_message != NULL);
On 2015/12/07 09:47:33, vabr (Chromium) wrote:
> nit: NULL -> nullptr, or drop the "!= NULL" completely. You may also consider
> calling GetFirstMessageMatching directly from within EXPECT_TRUE, because the
> variable name is duplicated by the message type name anyway, and the code will
> be shorter.

Done.

https://codereview.chromium.org/1494373003/diff/1/chrome/renderer/autofill/fo...
chrome/renderer/autofill/form_autocomplete_browsertest.cc:453:
EXPECT_TRUE(focus_no_longer_on_form_message != NULL);
On 2015/12/07 09:47:33, vabr (Chromium) wrote:
> Consider my comment for line 418 here as well.

Done.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:246: for (size_t i = 0; i <
forms.size(); ++i) {
On 2015/12/07 09:47:34, vabr (Chromium) wrote:
> A range-based for-loop would be easier to read:
> for (const auto& form : forms) {
>   forms_loaded_timestamps_[form] = timestamp;
> }

Done.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:279:
StartUploadProcess(submitted_form.Pass(), timestamp, true);
On 2015/12/07 09:47:34, vabr (Chromium) wrote:
> nit: std::move(submitted_form) instead of .Pass()
> 
> (Note that the documentation in base/memory/scoped_ptr.h is updated to no
longer
> mention Pass(). The goal is to make scoped_ptr behave as close to unique_ptr
as
> possible, AFAIU.)

Done.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:344:
base::Owned(form_structure.release()), loaded_timestamp,
On 2015/12/07 09:47:33, vabr (Chromium) wrote:
> Please use base::Passed(&form_structure) instead.
> Also update line 334 once you do.

I'm having difficulty with this change: Here is the error which appears.
...
../../base/bind_internal.h:305:34: error: no viable conversion from 'typename
std::enable_if<IsMoveOnlyType<scoped_ptr<FormStructure,
default_delete<FormStructure> > >::value, scoped_ptr<FormStructure,
default_delete<FormStructure> > >::type' (aka
'scoped_ptr<autofill::FormStructure,
std::default_delete<autofill::FormStructure> >') to 'const
autofill::FormStructure *const'
    runnable.Run(weak_ptr.get(), CallbackForward(args)...);
...

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:363: // We copy
|pending_form_data_| to a new FormStructure to be consumed by the
On 2015/12/07 09:47:33, vabr (Chromium) wrote:
> Why not just pass |pending_form_data_| to StartUploadProcess? This copy seems
> unnecessary.

StartUploadProcess needs a FormStructure to do its work (the existing caller has
one ready and we construct one here). If we pass pending_form_data_ (FormData)
to StartUpload process a FormStructure would have to be created, which
duplicates work for the 1st, existing caller.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:368:
StartUploadProcess(upload_form.Pass(), base::TimeTicks::Now(), false);
On 2015/12/07 09:47:34, vabr (Chromium) wrote:
> Pass -> std::move

Done.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1052: // TODO(mathp):
Differentiate non-submission uploads in the uploaded payload.
On 2015/12/07 09:47:34, vabr (Chromium) wrote:
> optional nit: Could you use TODO(crbug.com/xxxxxxx) instead? While it is very
> likely that this TODO will be addressed by you :), it won't hurt to make the
> context more accessible to everybody.

Done.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1093: if
(pending_form_data_) {
On 2015/12/07 09:47:33, vabr (Chromium) wrote:
> ProcessPendingFormForUpload seems to be currently tolerant to
pending_form_data_
> being null. Did you consider either making a non-null pending_form_data_ part
of
> the calling contract (and keeping the null checks at callsites), or dropping
the
> callsite null checks?

You're right. It's tolerant to null pending_form_data_. No longer checking and
added a DCHECK.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1096:
pending_form_data_.reset();
On 2015/12/07 09:47:33, vabr (Chromium) wrote:
> There seems to be no way that ProcessPendingFormForUpload would leave
> |pending_form_data_| non-empty. I suggest dropping this line, or replacing it
> with DCHECK(!pending_form_data_); to make that contract more explicit.

Acknowledged.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager_unittest.cc:3392: // We will
expect these types in the upload and no observed submission.
On 2015/12/07 09:47:34, vabr (Chromium) wrote:
> nit: Could you also point out in the comments, which line below leads to
> verifying those checks? It is not obvious, and I'm fearing that somebody might
> remove that by accident in the future, without causing any test breakage.

Done.

[vabr (Chromium), 2015-12-07 15:32:50]

LGTM, sorry for the two misunderstandings I introduced.

Cheers,
Vaclav

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:344:
base::Owned(form_structure.release()), loaded_timestamp,
On 2015/12/07 14:23:08, Mathieu Perreault wrote:
> On 2015/12/07 09:47:33, vabr (Chromium) wrote:
> > Please use base::Passed(&form_structure) instead.
> > Also update line 334 once you do.
> 
> I'm having difficulty with this change: Here is the error which appears.
> ...
> ../../base/bind_internal.h:305:34: error: no viable conversion from 'typename
> std::enable_if<IsMoveOnlyType<scoped_ptr<FormStructure,
> default_delete<FormStructure> > >::value, scoped_ptr<FormStructure,
> default_delete<FormStructure> > >::type' (aka
> 'scoped_ptr<autofill::FormStructure,
> std::default_delete<autofill::FormStructure> >') to 'const
> autofill::FormStructure *const'
>     runnable.Run(weak_ptr.get(), CallbackForward(args)...);
> ...

Yes, sorry, I missed that the receiving method takes a raw pointer rather than a
scoped pointer. Fair enough, this is a valid use of Owned, please ignore my
previous comment.

https://codereview.chromium.org/1494373003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:363: // We copy
|pending_form_data_| to a new FormStructure to be consumed by the
On 2015/12/07 14:23:08, Mathieu Perreault wrote:
> On 2015/12/07 09:47:33, vabr (Chromium) wrote:
> > Why not just pass |pending_form_data_| to StartUploadProcess? This copy
seems
> > unnecessary.
> 
> StartUploadProcess needs a FormStructure to do its work (the existing caller
has
> one ready and we construct one here). If we pass pending_form_data_ (FormData)
> to StartUpload process a FormStructure would have to be created, which
> duplicates work for the 1st, existing caller.

My bad, I overlooked that one is FormData and the other a FormStructure. Please
ignore my previous comment.

[Tom Sepez, 2015-12-07 16:53:13]

lgtm

[Mathieu, 2015-12-07 16:55:50]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-12-07 16:56:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494373003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494373003/20001

[commit-bot: I haz the power, 2015-12-07 17:31:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-07 17:31:39]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2015-12-07 23:08:56]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-12-07 23:08:57]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1494373003/#ps40001
(title: "fixed test")

[commit-bot: I haz the power, 2015-12-07 23:09:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494373003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494373003/40001

[commit-bot: I haz the power, 2015-12-08 01:07:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 01:07:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2015-12-08 14:29:35]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-12-08 14:29:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494373003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494373003/40001

[commit-bot: I haz the power, 2015-12-08 21:49:13]

Description was changed from

==========
[Autofill] Send Autofill upload when active form loses focus.

This is in addition to uploading on submission events. We now distinguish the
two, with TODOs to have separate metrics and a bit in the payload. I will
address the TODOs in subsequent CLs to keep complexity low.

The idea is to upload votes for any entered data (albeit lower quality, which we
will distinguish on the server side).

Note: I moved OnFormsSeen and OnFormSubmitted because they were out of order
with the .h

BUG=555015
TEST=AutofillManagerTest,FormAutocompleteTest
==========

to

==========
[Autofill] Send Autofill upload when active form loses focus.

This is in addition to uploading on submission events. We now distinguish the
two, with TODOs to have separate metrics and a bit in the payload. I will
address the TODOs in subsequent CLs to keep complexity low.

The idea is to upload votes for any entered data (albeit lower quality, which we
will distinguish on the server side).

Note: I moved OnFormsSeen and OnFormSubmitted because they were out of order
with the .h

BUG=555015
TEST=AutofillManagerTest,FormAutocompleteTest
==========

[commit-bot: I haz the power, 2015-12-08 21:49:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-08 21:50:11]

Description was changed from

==========
[Autofill] Send Autofill upload when active form loses focus.

This is in addition to uploading on submission events. We now distinguish the
two, with TODOs to have separate metrics and a bit in the payload. I will
address the TODOs in subsequent CLs to keep complexity low.

The idea is to upload votes for any entered data (albeit lower quality, which we
will distinguish on the server side).

Note: I moved OnFormsSeen and OnFormSubmitted because they were out of order
with the .h

BUG=555015
TEST=AutofillManagerTest,FormAutocompleteTest
==========

to

==========
[Autofill] Send Autofill upload when active form loses focus.

This is in addition to uploading on submission events. We now distinguish the
two, with TODOs to have separate metrics and a bit in the payload. I will
address the TODOs in subsequent CLs to keep complexity low.

The idea is to upload votes for any entered data (albeit lower quality, which we
will distinguish on the server side).

Note: I moved OnFormsSeen and OnFormSubmitted because they were out of order
with the .h

BUG=555015
TEST=AutofillManagerTest,FormAutocompleteTest

Committed: https://crrev.com/8ac68ce427132cd62ebc101ad08dd9c82f3eeb29
Cr-Commit-Position: refs/heads/master@{#363795}
==========

[commit-bot: I haz the power, 2015-12-08 21:50:12]

Patchset 3 (id:??) landed as
https://crrev.com/8ac68ce427132cd62ebc101ad08dd9c82f3eeb29
Cr-Commit-Position: refs/heads/master@{#363795}