[Password Generation] Allow generation when saved credentials aren't valid

[Password Generation] Allow generation when saved credentials aren't valid

We wait to verify that the user hasn't blacklisted a form before offering
generation. This fixes a bug where we may exit early due to no credentials
being autofillable and not inform the renderer that generation is allowed.

BUG=412496
Committed: https://crrev.com/bcc3cf8bdb46c0c9ef2628a89d4d3fba629b2259
Cr-Commit-Position: refs/heads/master@{#294565}

[Garrett Casto, 2014-09-09 20:41:47]

gcasto@chromium.org changed reviewers:
+ engedy@chromium.org

[engedy, 2014-09-10 08:19:29]

LGTM -- I have only added 2 comments to the neighboring code, not the actual
changes introduced in this CL. I do think we should address those, but it can be
done in a follow-up CL.

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:444: if
(best_score <= 0) {
While this check may very well implement the correct behavior, it does look
fishy: (best_score <= 0) is not equivalent to (best_matches_.empty()), although
the latter implies the former.

I have only skimmed through the code, but it seems possible that ScoreResult()
returns a score of 0 for some terribly bad PSL-matches, so |best_matches_| might
have elements even if |best_score| is 0.

I think we should add a comment to clarify this.

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager_unittest.cc:736:
EXPECT_CALL(*(client()->GetMockDriver()), AllowPasswordGenerationForForm(_))
Do we want to also expect here that PasswordAutofillWasBlocked() is called?

[Garrett Casto, 2014-09-12 07:55:48]

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:444: if
(best_score <= 0) {
On 2014/09/10 08:19:29, engedy wrote:
> While this check may very well implement the correct behavior, it does look
> fishy: (best_score <= 0) is not equivalent to (best_matches_.empty()),
although
> the latter implies the former.
> 
> I have only skimmed through the code, but it seems possible that ScoreResult()
> returns a score of 0 for some terribly bad PSL-matches, so |best_matches_|
might
> have elements even if |best_score| is 0.
> 

I don't think that it's possible to have a zero score match, though it doesn't
look intentional. base::SplitString(..., '/', ...) will always at least return
the empty string, so there will always be at least a partial 
origin match. Added a TODO() and I'll address it in a different CL, just in case
there is some unintended consequences of this change.
> I think we should add a comment to clarify this.

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager_unittest.cc:736:
EXPECT_CALL(*(client()->GetMockDriver()), AllowPasswordGenerationForForm(_))
On 2014/09/10 08:19:29, engedy wrote:
> Do we want to also expect here that PasswordAutofillWasBlocked() is called?

I think no. It's just adding an additional constraint on the implementation that
isn't particularly pertinent to this test.

[Garrett Casto, 2014-09-12 07:55:59]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-09-12 07:57:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/550563002/20001

[commit-bot: I haz the power, 2014-09-12 09:49:59]

Committed patchset #2 (id:20001) as 642ddf6e17e625d4c25e175f75f083b9a6d937a7

[commit-bot: I haz the power, 2014-09-12 09:57:08]

Patchset 2 (id:??) landed as
https://crrev.com/bcc3cf8bdb46c0c9ef2628a89d4d3fba629b2259
Cr-Commit-Position: refs/heads/master@{#294565}

[engedy, 2014-09-15 09:41:46]

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/550563002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:444: if
(best_score <= 0) {
On 2014/09/12 07:55:48, Garrett Casto wrote:
> On 2014/09/10 08:19:29, engedy wrote:
> > While this check may very well implement the correct behavior, it does look
> > fishy: (best_score <= 0) is not equivalent to (best_matches_.empty()),
> although
> > the latter implies the former.
> > 
> > I have only skimmed through the code, but it seems possible that
ScoreResult()
> > returns a score of 0 for some terribly bad PSL-matches, so |best_matches_|
> might
> > have elements even if |best_score| is 0.
> > 
> 
> I don't think that it's possible to have a zero score match, though it doesn't
> look intentional. base::SplitString(..., '/', ...) will always at least return
> the empty string, so there will always be at least a partial 
> origin match. Added a TODO() and I'll address it in a different CL, just in
case
> there is some unintended consequences of this change.
> > I think we should add a comment to clarify this.
> 

Yes, that seems true, but for this reasoning, you needed to know that some other
part of the code will put the preceding slash in the URL, even if it did now
have a path at all (this is permitted by GURL). To me, at least, this was not
obvious. And I think exactly these kind of dependencies are the reason why I
have complained in the first place. :)

I agree think the cleanest is to swap this with the next for loop, and check if
|best_matches_| is empty instead.

Thanks for adding the comment.