Skip to content

GitLab

T
tangled
Commit 0b6da642 authored by Moe Ahmadi's avatar Moe Ahmadi Committed by Commit Bot
Browse files
Options

Revert "[AF] Do not discard form data after dynamic form submission" 

This reverts commit e93874be.

Reason for revert: http://crbug.com/863954#c3

Original change's description:
> [AF] Do not discard form data after dynamic form submission
> 
> Prior to this change form data were being discarded after a dynamic form
> submission. This was because the dynamically changed form was being added
> to the cache with its user entered/autofilled values. At the time of
> submission, form data were being discarded as they were thought to be the
> initial values of the fields at page load. This CL changes
> AutofillHandler::OnFormsSeen (which is called on dynamic form changes) to
> attempt to find a previously cached version of the form first. Prior to
> addition to the cache, form values are overridden by the cached values.
> This prevents those values from getting confused with the initial form
> values at the time of submission.
> 
> Bug: 863954
> 
> Change-Id: Iba4771ee87b9777e66a6b10b6b369379dc123a46
> Reviewed-on: https://chromium-review.googlesource.com/1137021
> Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577033}

TBR=sebsg@chromium.org,mahmadi@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 863954
Change-Id: I71d0970d10fe1d4857e98ba558b9253b13ad053d
Reviewed-on: https://chromium-review.googlesource.com/1151911Reviewed-by: default avatarMoe Ahmadi <mahmadi@chromium.org>
Reviewed-by: default avatarSebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579858}
parent 4a1c70a7
Hide whitespace changes
Inline Side-by-side
Showing with 7 additions and 119 deletions
chrome/browser/autofill/autofill_interactive_uitest.cc
View file @ 0b6da642
...@@ -2991,35 +2991,6 @@ IN_PROC_BROWSER_TEST_P(AutofillDynamicFormInteractiveTest, ...@@ -2991,35 +2991,6 @@ IN_PROC_BROWSER_TEST_P(AutofillDynamicFormInteractiveTest,
ExpectFieldValue("phone", "15125551234"); ExpectFieldValue("phone", "15125551234");
} }
// Test that form data gets saved after submitting dynamically changing form.
IN_PROC_BROWSER_TEST_P(AutofillDynamicFormInteractiveTest,
Submit_DynamicChangingFormFill) {
CreateTestProfile();
GURL url = embedded_test_server()->GetURL(
"a.com", "/autofill/dynamic_form_new_field.html");
ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
TriggerFormFill("firstname");
// Wait for the re-fill to happen.
bool has_refilled = false;
ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
GetWebContents(), "hasRefilled()", &has_refilled));
ASSERT_TRUE(has_refilled);
// Edit the company field.
ASSERT_TRUE(content::ExecuteScript(
GetWebContents(), "document.getElementById('company').value = 'NASA';"));
// Submit the form.
ASSERT_TRUE(content::ExecuteScript(
GetWebContents(), "document.getElementById('form1').submit();"));
// A new autofill profile gets added.
ASSERT_EQ(2u, GetProfiles(browser()).size());
}
INSTANTIATE_TEST_CASE_P(All, AutofillInteractiveTest, testing::Bool()); INSTANTIATE_TEST_CASE_P(All, AutofillInteractiveTest, testing::Bool());
INSTANTIATE_TEST_CASE_P(All, INSTANTIATE_TEST_CASE_P(All,
AutofillCreditCardInteractiveTest, AutofillCreditCardInteractiveTest,
......
chrome/browser/autofill/autofill_uitest_util.cc
View file @ 0b6da642
...@@ -99,12 +99,4 @@ void AddTestAutofillData(Browser* browser, ...@@ -99,12 +99,4 @@ void AddTestAutofillData(Browser* browser,
observer.Wait(); observer.Wait();
} }
std::vector<AutofillProfile*> GetProfiles(Browser* browser) {
// Wait for asynchronous updates to PersonalDataManager to complete.
PdmChangeWaiter observer(browser);
observer.Wait();
return GetPersonalDataManager(browser->profile())->GetProfiles();
}
} // namespace autofill } // namespace autofill
chrome/browser/autofill/autofill_uitest_util.h
View file @ 0b6da642
...@@ -21,7 +21,6 @@ void AddTestCreditCard(Browser* browser, const CreditCard& card); ...@@ -21,7 +21,6 @@ void AddTestCreditCard(Browser* browser, const CreditCard& card);
void AddTestAutofillData(Browser* browser, void AddTestAutofillData(Browser* browser,
const AutofillProfile& profile, const AutofillProfile& profile,
const CreditCard& card); const CreditCard& card);
std::vector<AutofillProfile*> GetProfiles(Browser* browser);
} // namespace autofill } // namespace autofill
#endif // CHROME_BROWSER_AUTOFILL_AUTOFILL_UITEST_UTIL_H_ #endif // CHROME_BROWSER_AUTOFILL_AUTOFILL_UITEST_UTIL_H_
chrome/test/data/autofill/dynamic_form_new_field.html deleted 100644 → 0
View file @ 4a1c70a7
<!-- A page that is used to test that a dynamic form fill feature works properly. -->
<body>
<form name="addr1.1" id="form1" action="https://example.com/" method="post">
Name: <input type="text" name="firstname" id="firstname"><br>
Address: <input type="text" name="address1" id="address1"><br>
City: <input type="text" name="city" id="city"><br>
State: <select name="state" id="state">
<option value="CA">CA</option>
<option value="MA">MA</option>
<option value="NY">NY</option>
<option value="MD">MD</option>
<option value="OR">OR</option>
<option value="OH">OH</option>
<option value="IL">IL</option>
<option value="DC">DC</option>
</select> <br>
Country: <select name="country" id="country" onchange="CountryChanged()">
<option value="CA">Canada</option>
<option value="US">United States</option>
</select> <br>
Company: <input name="company" id="company"> <br>
Email: <input name="email" id="email"> <br>
Phone: <input name="phone" id="phone"> <br>
<input type="reset" value="Reset">
<input type="submit" value="Submit" id="profile_submit">
</form>
</body>
<script src="dynamic_form_utils.js"></script>
<script>
var notify_on_first_name_input_change = false;
var first_name_input_changed = false;
function CountryChanged() {
form = document.getElementById('form1');
var first_name_input = form.elements[0];
first_name_input.value = '';
first_name_input.onchange = function() {
if (notify_on_first_name_input_change)
window.domAutomationController.send(first_name_input.value != '');
else
first_name_input_changed = true;
}
var zip_input = document.createElement('input');
zip_input.setAttribute('type', 'text');
zip_input.setAttribute('name', 'zip');
zip_input.setAttribute('id', 'zip');
form.insertBefore(zip_input, form.elements[form.elements.length - 2]);
}
function hasRefilled() {
var first_name_input = document.getElementById('firstname');
if (first_name_input && first_name_input_changed) {
window.domAutomationController.send(first_name_input.value != '');
} else {
notify_on_first_name_input_change = true;
}
}
</script>
components/autofill/core/browser/autofill_handler.cc
View file @ 0b6da642
...@@ -51,10 +51,7 @@ void AutofillHandler::OnFormsSeen(const std::vector<FormData>& forms, ...@@ -51,10 +51,7 @@ void AutofillHandler::OnFormsSeen(const std::vector<FormData>& forms,
for (const FormData& form : forms) { for (const FormData& form : forms) {
const auto parse_form_start_time = TimeTicks::Now(); const auto parse_form_start_time = TimeTicks::Now();
FormStructure* form_structure = nullptr; FormStructure* form_structure = nullptr;
// Try to find the FormStructure that corresponds to |form|. if (!ParseForm(form, /*cached_form=*/nullptr, &form_structure))
// |form_structure| may still be nullptr after this call.
ignore_result(FindCachedForm(form, &form_structure));
if (!ParseForm(form, form_structure, &form_structure))
continue; continue;
DCHECK(form_structure); DCHECK(form_structure);
if (form_structure == nullptr) if (form_structure == nullptr)
...@@ -185,7 +182,6 @@ bool AutofillHandler::ParseForm(const FormData& form, ...@@ -185,7 +182,6 @@ bool AutofillHandler::ParseForm(const FormData& form,
// We need to keep the server data if available. We need to use them while // We need to keep the server data if available. We need to use them while
// determining the heuristics. // determining the heuristics.
form_structure->RetrieveFromCache(*cached_form, form_structure->RetrieveFromCache(*cached_form,
/*apply_value=*/true,
/*apply_is_autofilled=*/true, /*apply_is_autofilled=*/true,
/*only_server_and_autofill_state=*/true); /*only_server_and_autofill_state=*/true);
} }
......
components/autofill/core/browser/autofill_manager.cc
View file @ 0b6da642
...@@ -1406,7 +1406,6 @@ std::unique_ptr<FormStructure> AutofillManager::ValidateSubmittedForm( ...@@ -1406,7 +1406,6 @@ std::unique_ptr<FormStructure> AutofillManager::ValidateSubmittedForm(
return std::unique_ptr<FormStructure>(); return std::unique_ptr<FormStructure>();
submitted_form->RetrieveFromCache(*cached_submitted_form, submitted_form->RetrieveFromCache(*cached_submitted_form,
/*apply_value=*/false,
/*apply_is_autofilled=*/false, /*apply_is_autofilled=*/false,
/*only_server_and_autofill_state=*/false); /*only_server_and_autofill_state=*/false);
return submitted_form; return submitted_form;
......
components/autofill/core/browser/form_structure.cc
View file @ 0b6da642
...@@ -723,7 +723,6 @@ bool FormStructure::ShouldBeUploaded() const { ...@@ -723,7 +723,6 @@ bool FormStructure::ShouldBeUploaded() const {
void FormStructure::RetrieveFromCache( void FormStructure::RetrieveFromCache(
const FormStructure& cached_form, const FormStructure& cached_form,
const bool apply_value,
const bool apply_is_autofilled, const bool apply_is_autofilled,
const bool only_server_and_autofill_state) { const bool only_server_and_autofill_state) {
// Map from field signatures to cached fields. // Map from field signatures to cached fields.
...@@ -748,14 +747,11 @@ void FormStructure::RetrieveFromCache( ...@@ -748,14 +747,11 @@ void FormStructure::RetrieveFromCache(
if (apply_is_autofilled) { if (apply_is_autofilled) {
field->is_autofilled = cached_field->second->is_autofilled; field->is_autofilled = cached_field->second->is_autofilled;
} }
if (field->form_control_type != "select-one") { if (field->form_control_type != "select-one" &&
if (apply_value) { field->value == cached_field->second->value) {
field->value = cached_field->second->value; // From the perspective of learning user data, text fields containing
} else if (field->value == cached_field->second->value) { // default values are equivalent to empty fields.
// From the perspective of learning user data, text fields containing field->value = base::string16();
// default values are equivalent to empty fields.
field->value = base::string16();
}
} }
field->set_server_type(cached_field->second->server_type()); field->set_server_type(cached_field->second->server_type());
field->set_previously_autofilled( field->set_previously_autofilled(
......
components/autofill/core/browser/form_structure.h
View file @ 0b6da642
...@@ -135,10 +135,8 @@ class FormStructure { ...@@ -135,10 +135,8 @@ class FormStructure {
// server. // server.
bool ShouldBeUploaded() const; bool ShouldBeUploaded() const;
// Sets the field types to be those set for |cached_form|. if |apply_value| is // Sets the field types to be those set for |cached_form|.
// true, value is copied over from |cached_form| for non-select elements only.
void RetrieveFromCache(const FormStructure& cached_form, void RetrieveFromCache(const FormStructure& cached_form,
const bool apply_value,
const bool apply_is_autofilled, const bool apply_is_autofilled,
const bool only_server_and_autofill_state); const bool only_server_and_autofill_state);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment