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

components/autofill/core/browser/autofill_manager.cc

Index: components/autofill/core/browser/autofill_manager.cc
diff --git a/components/autofill/core/browser/autofill_manager.cc b/components/autofill/core/browser/autofill_manager.cc
index e3c5e3c66c9fc84f24197da1106f06db992fd77a..605a780d3dca2b510d9c5b5e99bf1c0b92dcab3f 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -226,6 +226,30 @@ bool AutofillManager::ShouldShowScanCreditCard(const FormData& form,
                                  base::ASCIIToUTF16("0123456789"));
 }
 
+void AutofillManager::OnFormsSeen(const std::vector<FormData>& forms,
+                                  const TimeTicks& timestamp) {
+  if (!IsValidFormDataVector(forms))
+    return;
+
+  if (!driver_->RendererIsAvailable())
+    return;
+
+  bool enabled = IsAutofillEnabled();
+  if (!has_logged_autofill_enabled_) {
+    AutofillMetrics::LogIsAutofillEnabledAtPageLoad(enabled);
+    has_logged_autofill_enabled_ = true;
+  }
+
+  if (!enabled)
+    return;
+
+  for (size_t i = 0; i < forms.size(); ++i) {

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

A range-based for-loop would be easier to read:
for (const auto& form : forms) {
  forms_loaded_timestamps_[form] = timestamp;
}

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

Done.

+    forms_loaded_timestamps_[forms[i]] = timestamp;
+  }
+
+  ParseForms(forms);
+}
+
 bool AutofillManager::OnWillSubmitForm(const FormData& form,
                                        const TimeTicks& timestamp) {
   if (!IsValidFormData(form))
@@ -252,10 +276,42 @@ bool AutofillManager::OnWillSubmitForm(const FormData& form,
   address_form_event_logger_->OnWillSubmitForm();
   credit_card_form_event_logger_->OnWillSubmitForm();
 
+  StartUploadProcess(submitted_form.Pass(), timestamp, true);

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

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

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

Done.

+
+  return true;
+}
+
+bool AutofillManager::OnFormSubmitted(const FormData& form) {
+  if (!IsValidFormData(form))
+    return false;
+
+  // We will always give Autocomplete a chance to save the data.
+  scoped_ptr<FormStructure> submitted_form = ValidateSubmittedForm(form);
+  if (!submitted_form) {
+    return false;
+  }
+
+  address_form_event_logger_->OnFormSubmitted();
+  credit_card_form_event_logger_->OnFormSubmitted();
+
+  // Update Personal Data with the form's submitted data.
+  if (submitted_form->IsAutofillable())
+    ImportFormData(*submitted_form);
+
+  recently_unmasked_cards_.clear();
+
+  return true;
+}
+
+void AutofillManager::StartUploadProcess(
+    scoped_ptr<FormStructure> form_structure,
+    const TimeTicks& timestamp,
+    bool observed_submission) {
   // Only upload server statistics and UMA metrics if at least some local data
   // is available to use as a baseline.
   const std::vector<AutofillProfile*>& profiles = personal_data_->GetProfiles();
-  if (submitted_form->IsAutofillable()) {
+  if (observed_submission && form_structure->IsAutofillable()) {
+    // TODO(mathp): provide metrics for non-submission uploads.
     AutofillMetrics::LogNumberOfProfilesAtAutofillableFormSubmission(
         personal_data_->GetProfiles().size());
   }
@@ -274,71 +330,42 @@ bool AutofillManager::OnWillSubmitForm(const FormData& form,
     for (const CreditCard* card : credit_cards)
       copied_credit_cards.push_back(*card);
 
-    // Note that ownership of |submitted_form| is passed to the second task,
+    // Note that ownership of |form_structure| is passed to the second task,
     // using |base::Owned|.
-    FormStructure* raw_submitted_form = submitted_form.get();
+    FormStructure* raw_form = form_structure.get();
+    TimeTicks loaded_timestamp =
+        forms_loaded_timestamps_[raw_form->ToFormData()];
     driver_->GetBlockingPool()->PostTaskAndReply(
         FROM_HERE,
-        base::Bind(&DeterminePossibleFieldTypesForUpload,
-                   copied_profiles,
-                   copied_credit_cards,
-                   app_locale_,
-                   raw_submitted_form),
+        base::Bind(&DeterminePossibleFieldTypesForUpload, copied_profiles,
+                   copied_credit_cards, app_locale_, raw_form),
         base::Bind(&AutofillManager::UploadFormDataAsyncCallback,
                    weak_ptr_factory_.GetWeakPtr(),
-                   base::Owned(submitted_form.release()),
-                   forms_loaded_timestamps_[form],
-                   initial_interaction_timestamp_,
-                   timestamp));
+                   base::Owned(form_structure.release()), loaded_timestamp,

[vabr (Chromium), 2015/12/07 09:47:33]

Please use base::Passed(&form_structure) instead.
Also update line 334 once you do.

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

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

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

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.

+                   initial_interaction_timestamp_, timestamp,
+                   observed_submission));
   }
-
-  return true;
 }
 
-bool AutofillManager::OnFormSubmitted(const FormData& form) {
-  if (!IsValidFormData(form))
-    return false;
-
-  // We will always give Autocomplete a chance to save the data.
-  scoped_ptr<FormStructure> submitted_form = ValidateSubmittedForm(form);
-  if (!submitted_form) {
-    return false;
+void AutofillManager::UpdatePendingForm(const FormData& form) {
+  // Process the current pending form if different than supplied |form|.
+  if (pending_form_data_ && !pending_form_data_->SameFormAs(form)) {
+    ProcessPendingFormForUpload();
   }
-
-  address_form_event_logger_->OnFormSubmitted();
-  credit_card_form_event_logger_->OnFormSubmitted();
-
-  // Update Personal Data with the form's submitted data.
-  if (submitted_form->IsAutofillable())
-    ImportFormData(*submitted_form);
-
-  recently_unmasked_cards_.clear();
-
-  return true;
+  // A new pending form is assigned.
+  pending_form_data_.reset(new FormData(form));
 }
 
-void AutofillManager::OnFormsSeen(const std::vector<FormData>& forms,
-                                  const TimeTicks& timestamp) {
-  if (!IsValidFormDataVector(forms))
-    return;
-
-  if (!driver_->RendererIsAvailable())
-    return;
-
-  bool enabled = IsAutofillEnabled();
-  if (!has_logged_autofill_enabled_) {
-    AutofillMetrics::LogIsAutofillEnabledAtPageLoad(enabled);
-    has_logged_autofill_enabled_ = true;
-  }
-
-  if (!enabled)
+void AutofillManager::ProcessPendingFormForUpload() {
+  if (!pending_form_data_)
     return;
 
-  for (size_t i = 0; i < forms.size(); ++i) {
-    forms_loaded_timestamps_[forms[i]] = timestamp;
-  }
+  // We copy |pending_form_data_| to a new FormStructure to be consumed by the

[vabr (Chromium), 2015/12/07 09:47:33]

Why not just pass |pending_form_data_| to StartUploadProcess? This copy seems
unnecessary.

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

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.

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

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

+  // upload process and reset |pending_form_data_|.
+  scoped_ptr<FormStructure> upload_form(new FormStructure(*pending_form_data_));
+  pending_form_data_.reset();
 
-  ParseForms(forms);
+  StartUploadProcess(upload_form.Pass(), base::TimeTicks::Now(), false);

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

Pass -> std::move

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

Done.

 }
 
 void AutofillManager::OnTextFieldDidChange(const FormData& form,
@@ -352,6 +379,8 @@ void AutofillManager::OnTextFieldDidChange(const FormData& form,
   if (!GetCachedFormAndField(form, field, &form_structure, &autofill_field))
     return;
 
+  UpdatePendingForm(form);
+
   if (!user_did_type_) {
     user_did_type_ = true;
     AutofillMetrics::LogUserHappinessMetric(AutofillMetrics::USER_DID_TYPE);
@@ -584,15 +613,22 @@ void AutofillManager::FillCreditCardForm(int query_id,
                              form, field, credit_card, true);
 }
 
+void AutofillManager::OnFocusNoLongerOnForm() {
+  ProcessPendingFormForUpload();
+}
+
 void AutofillManager::OnDidPreviewAutofillFormData() {
   if (test_delegate_)
     test_delegate_->DidPreviewFormData();
 }
 
-void AutofillManager::OnDidFillAutofillFormData(const TimeTicks& timestamp) {
+void AutofillManager::OnDidFillAutofillFormData(const FormData& form,
+                                                const TimeTicks& timestamp) {
   if (test_delegate_)
     test_delegate_->DidFillFormData();
 
+  UpdatePendingForm(form);
+
   AutofillMetrics::LogUserHappinessMetric(AutofillMetrics::USER_DID_AUTOFILL);
   if (!user_did_autofill_) {
     user_did_autofill_ = true;
@@ -1004,11 +1040,16 @@ void AutofillManager::UploadFormDataAsyncCallback(
     const FormStructure* submitted_form,
     const TimeTicks& load_time,
     const TimeTicks& interaction_time,
-    const TimeTicks& submission_time) {
+    const TimeTicks& submission_time,
+    bool observed_submission) {
+  // TODO(mathp): Have different set of metrics for non-submission uploads.
+  if (observed_submission) {
   submitted_form->LogQualityMetrics(
       load_time, interaction_time, submission_time, client_->GetRapporService(),
       did_show_suggestions_);
+  }
 
+  // TODO(mathp): Differentiate non-submission uploads in the uploaded payload.

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

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.

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

Done.

   if (submitted_form->ShouldBeCrowdsourced())
     UploadFormData(*submitted_form);
 }
@@ -1049,6 +1090,10 @@ void AutofillManager::Reset() {
   user_did_type_ = false;
   user_did_autofill_ = false;
   user_did_edit_autofilled_field_ = false;
+  if (pending_form_data_) {

[vabr (Chromium), 2015/12/07 09:47:33]

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?

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

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

+    ProcessPendingFormForUpload();
+  }
+  pending_form_data_.reset();

[vabr (Chromium), 2015/12/07 09:47:33]

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.

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

Acknowledged.

   unmask_request_ = payments::PaymentsClient::UnmaskRequestDetails();
   unmasking_query_id_ = -1;
   unmasking_form_ = FormData();
@@ -1491,7 +1536,6 @@ void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
   for (const FormData& form : forms) {
     scoped_ptr<FormStructure> form_structure(new FormStructure(form));
     form_structure->ParseFieldTypesFromAutocompleteAttributes();
-
     if (!form_structure->ShouldBeParsed())
       continue;