Autofill's "Clear Form" should clear only autofilled fields

Autofill's "Clear Form" should clear only autofilled fields

Updated ClearFormWithElement() to ignore fields that were not
autofilled.

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

[Nikhil, 2014-06-04 14:13:16]

PTAL. Thanks!

If you've any review comments, then please let me know. I'll incorporate them :)

[Ilya Sherman, 2014-06-04 20:33:31]

Please add test coverage as well.  Otherwise LG -- thanks!

[Ilya Sherman, 2014-06-06 21:15:20]

Sorry, I probably should have pointed you at where the existing tests live,
since it's not obvious:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au...

[Nikhil, 2014-06-11 11:36:11]

On 2014/06/06 21:15:20, Ilya Sherman wrote:
> Sorry, I probably should have pointed you at where the existing tests live,
> since it's not obvious:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au...

Thanks! I'll take a look. Sorry for the delay in getting back to you.

[Nikhil, 2014-06-13 10:55:34]

Test case added. I hope I added in the right file. PTAL :)

If you've any review comments, then please let me know. I'll incorporate them.

Also, I noticed that after my change following two test cases are failing -

FormAutofillTest.ClearFormWithNode
FormAutofillTest.ClearFormWithNodeContainingSelectOne

[Ilya Sherman, 2014-06-13 20:55:29]

On 2014/06/13 10:55:34, Nikhil wrote:
> I noticed that after my change following two test cases are failing -
> 
> FormAutofillTest.ClearFormWithNode
> FormAutofillTest.ClearFormWithNodeContainingSelectOne

Please update these tests to match the new behavior.  I think I'd prefer to set
all of the fields as autofilled for these tests, and test the behavior of
clearing non-autofilled fields in the new test case that you've added.

https://codereview.chromium.org/316853006/diff/20001/chrome/renderer/autofill...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/316853006/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:3542:
EXPECT_TRUE(form_cache.ClearFormWithElement(phone));
Hmm, ClearFormWithElement clears the entire form, not just a single element, so
there shouldn't be a need to call it four times.

https://codereview.chromium.org/316853006/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:3557: }
Please move this so that it is adjacent to the other tests around "Clear Form"
behavior.

[Nikhil, 2014-06-16 13:54:28]

Review comments incorporated. All tests passed.

PTAL. Thanks!

https://codereview.chromium.org/316853006/diff/20001/chrome/renderer/autofill...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/316853006/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:3542:
EXPECT_TRUE(form_cache.ClearFormWithElement(phone));
On 2014/06/13 20:55:29, Ilya Sherman wrote:
> Hmm, ClearFormWithElement clears the entire form, not just a single element,
so
> there shouldn't be a need to call it four times.

Done.

https://codereview.chromium.org/316853006/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:3557: }
On 2014/06/13 20:55:29, Ilya Sherman wrote:
> Please move this so that it is adjacent to the other tests around "Clear Form"
> behavior.

Done.

[Ilya Sherman, 2014-06-16 21:19:37]

Thanks, Nikhil.

https://codereview.chromium.org/316853006/diff/40001/chrome/renderer/autofill...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/316853006/diff/40001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:2967: 
nit: I'd avoid these blank lines for the various elements for which you're
setting the autofilled attribute to be true, since the comment on line 2963 is
meant to describe the entire block.

https://codereview.chromium.org/316853006/diff/40001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:2974:
no_autocomplete.setAutofilled(true);
It doesn't really make sense to set autofilled to true for the
autocomplete="off" elements, because those should never be autofilled.  So, this
part of the behavior of the existing test cases should actually change, as
they're otherwise testing something unrealistic.

[Nikhil, 2014-06-17 08:59:44]

Updated the behavior of existing test case. PTAL. 

Let me know whether this is okay now. 

Thanks!

https://codereview.chromium.org/316853006/diff/40001/chrome/renderer/autofill...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/316853006/diff/40001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:2967: 
On 2014/06/16 21:19:37, Ilya Sherman wrote:
> nit: I'd avoid these blank lines for the various elements for which you're
> setting the autofilled attribute to be true, since the comment on line 2963 is
> meant to describe the entire block.

Done.

https://codereview.chromium.org/316853006/diff/40001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:2974:
no_autocomplete.setAutofilled(true);
Right! I've updated the behavior. Since these fields won't be cleared now
because of change in behavior by this change, I'm now comparing against original
value instead of empty string. Please let me know whether this is okay.

On 2014/06/16 21:19:37, Ilya Sherman wrote:
> It doesn't really make sense to set autofilled to true for the
> autocomplete="off" elements, because those should never be autofilled.  So,
this
> part of the behavior of the existing test cases should actually change, as
> they're otherwise testing something unrealistic.

[Ilya Sherman, 2014-06-17 17:43:49]

LGTM.  Thanks, Nikhil!

[Ilya Sherman, 2014-06-17 17:43:53]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:44:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/316853006/60001

[commit-bot: I haz the power, 2014-06-17 23:30:08]

Change committed as 277902