[Autofill] OnLoadedServerPredictions correctly supplies FormStructure to ParseQueryResponse.

components/autofill/core/browser/autofill_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/core/browser/autofill_manager.h"
 
 #include <stddef.h>
 
+#include <algorithm>
 #include <limits>
 #include <map>
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/containers/adapters.h"
 #include "base/files/file_util.h"
 #include "base/guid.h"
@@ skipping 718 matching lines @@
       values.size() != labels.size())
     return;
 
   external_delegate_->SetCurrentDataListValues(values, labels);
 }
 
 void AutofillManager::OnLoadedServerPredictions(
     const std::string& response_xml,
     const std::vector<std::string>& form_signatures) {
   // We obtain the current valid FormStructures represented by
   // |form_signatures|. We invert both lists because most recent forms are at

[vabr (Chromium), 2015/11/23 16:59:25]

Just curious -- was this meant to improve performance? Is it still worth it with
the std::reverse at the end, and the added complexity?

[Mathieu, 2015/11/23 17:27:07]

Yes it's meant to improve performance. With <form>-less Autofill a new form
structure is created at every field added. This pool of forms can then get quite
large. I think complexity is not that bad and replicated elsewhere in this file.

[vabr (Chromium), 2015/11/23 17:47:07]

Acknowledged.

-  // the end of the list.
+  // the end of the list (and reverse the resulting pointer vector).
   std::vector<FormStructure*> queried_forms;
   for (const std::string& signature : base::Reversed(form_signatures)) {
+    bool found_signature = false;
     for (FormStructure* cur_form : base::Reversed(form_structures_)) {
+      if (found_signature)
+        break;
       if (cur_form->FormSignature() == signature) {
         queried_forms.push_back(cur_form);
-        continue;
+        found_signature = true;

[vabr (Chromium), 2015/11/23 16:59:25]

Could you just execute "break;" here and remove |found_signature|? Or did you
mean to use |form_signature| after the for loop to test for success?

[Mathieu, 2015/11/23 17:27:07]

Of course, I did this too quickly, brain fart.

       }
     }
   }
+  std::reverse(queried_forms.begin(), queried_forms.end());
+
   // If there are no current forms corresponding to the queried signatures, drop
   // the query response.
   if (queried_forms.empty())
     return;
 
   // Parse and store the server predictions.
   FormStructure::ParseQueryResponse(response_xml, queried_forms,
                                     client_->GetRapporService());
 
   // Forward form structures to the password generation manager to detect
@@ skipping 866 matching lines @@
     if (i > 0)
       fputs("Next oldest form:\n", file);
   }
   fputs("\n", file);
 
   fclose(file);
 }
 #endif  // ENABLE_FORM_DEBUG_DUMP
 
 }  // namespace autofill