Fix for no password save when action URL is modified

This CL fixes the issue that password save prompt is suppressed when the action URL is modified prior to submit.

BUG=27246
TEST= Login to the website http://ikariam.com/ and verify password save prompt is shown after navigation.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=158301

[guohui, 2012-08-31 20:29:05]

A simple design doc at
https://docs.google.com/a/google.com/document/d/1yHvPIZABMjKhwh0TnnB9CAZJMZM8...
.

[tim (not reviewing), 2012-09-05 20:43:37]

Looks good, style comment below

http://codereview.chromium.org/10892011/diff/4002/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/10892011/diff/4002/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:66: bool
require_action_match) const {
nit - please use an enum instead of bool here, e.g.
ACTION_MATCH_REQUIRED
ACTION_MATCH_NOT_REQUIRED

and don't overload the method -- I think we should call it explicitly inline?
Otherwise we should rename the method to distinguish.

[guohui, 2012-09-05 21:30:14]

http://codereview.chromium.org/10892011/diff/4002/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/10892011/diff/4002/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager.cc:66: bool
require_action_match) const {
On 2012/09/05 20:43:37, timsteele wrote:
> nit - please use an enum instead of bool here, e.g.
> ACTION_MATCH_REQUIRED
> ACTION_MATCH_NOT_REQUIRED
> 
> and don't overload the method -- I think we should call it explicitly inline?
> Otherwise we should rename the method to distinguish.

Done.

[tim (not reviewing), 2012-09-05 21:40:59]

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
chrome/browser/password_manager/password_form_manager.cc:78: if (action_match ==
kActionMatchRequired)
nit - chromium style diverges from Google style here and prefers MACRO_STYLE for
enum values. (see http://dev.chromium.org/developers/coding-style)

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
chrome/browser/password_manager/password_form_manager.cc:207:
pending_credentials_.action = observed_form_.action;
Could we add a test that verifies this gets updated on the "new username" case
as well? See TestEmptyAction.

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager.cc:133: form,
PasswordFormManager::kActionMatchNotRequired)) {
Can we add a PasswordManagerTest that verifies matches with action mismatch?

[tim (not reviewing), 2012-09-17 03:22:50]

On 2012/09/05 21:40:59, timsteele wrote:
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> File chrome/browser/password_manager/password_form_manager.cc (right):
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> chrome/browser/password_manager/password_form_manager.cc:78: if (action_match
==
> kActionMatchRequired)
> nit - chromium style diverges from Google style here and prefers MACRO_STYLE
for
> enum values. (see http://dev.chromium.org/developers/coding-style)
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> chrome/browser/password_manager/password_form_manager.cc:207:
> pending_credentials_.action = observed_form_.action;
> Could we add a test that verifies this gets updated on the "new username" case
> as well? See TestEmptyAction.
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> File chrome/browser/password_manager/password_manager.cc (right):
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> chrome/browser/password_manager/password_manager.cc:133: form,
> PasswordFormManager::kActionMatchNotRequired)) {
> Can we add a PasswordManagerTest that verifies matches with action mismatch?

friendly ping?

[guohui, 2012-09-17 13:28:55]

Sorry for the delay, has been distracted by other projects.

Enum style fixed, as unit tests added.

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
chrome/browser/password_manager/password_form_manager.cc:78: if (action_match ==
kActionMatchRequired)
On 2012/09/05 21:40:59, timsteele wrote:
> nit - chromium style diverges from Google style here and prefers MACRO_STYLE
for
> enum values. (see http://dev.chromium.org/developers/coding-style)

Done.

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
chrome/browser/password_manager/password_form_manager.cc:207:
pending_credentials_.action = observed_form_.action;
On 2012/09/05 21:40:59, timsteele wrote:
> Could we add a test that verifies this gets updated on the "new username" case
> as well? See TestEmptyAction.

Updated TestNewLogin to verify the action gets populated correctly on the new
username case. Also created a new test case TestUpdateAction to verify the
action get updated on the case when username and password are the same as saved
but action URL has changed.

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager.cc:133: form,
PasswordFormManager::kActionMatchNotRequired)) {
On 2012/09/05 21:40:59, timsteele wrote:
> Can we add a PasswordManagerTest that verifies matches with action mismatch?

Done.

[guohui, 2012-09-20 20:26:02]

On 2012/09/17 13:28:55, guohui wrote:
> Sorry for the delay, has been distracted by other projects.
> 
> Enum style fixed, as unit tests added.
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> File chrome/browser/password_manager/password_form_manager.cc (right):
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> chrome/browser/password_manager/password_form_manager.cc:78: if (action_match
==
> kActionMatchRequired)
> On 2012/09/05 21:40:59, timsteele wrote:
> > nit - chromium style diverges from Google style here and prefers MACRO_STYLE
> for
> > enum values. (see http://dev.chromium.org/developers/coding-style)
> 
> Done.
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> chrome/browser/password_manager/password_form_manager.cc:207:
> pending_credentials_.action = observed_form_.action;
> On 2012/09/05 21:40:59, timsteele wrote:
> > Could we add a test that verifies this gets updated on the "new username"
case
> > as well? See TestEmptyAction.
> 
> Updated TestNewLogin to verify the action gets populated correctly on the new
> username case. Also created a new test case TestUpdateAction to verify the
> action get updated on the case when username and password are the same as
saved
> but action URL has changed.
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> File chrome/browser/password_manager/password_manager.cc (right):
> 
>
http://codereview.chromium.org/10892011/diff/15005/chrome/browser/password_ma...
> chrome/browser/password_manager/password_manager.cc:133: form,
> PasswordFormManager::kActionMatchNotRequired)) {
> On 2012/09/05 21:40:59, timsteele wrote:
> > Can we add a PasswordManagerTest that verifies matches with action mismatch?
> 
> Done.

ping :)

[tim (not reviewing), 2012-09-24 02:36:01]

LGTM

[commit-bot: I haz the power, 2012-09-24 14:56:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/10892011/8012