Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
921873002:
Clarify PasswordFormManager::OnRequestDone (Closed)
|
Created:
5 years, 10 months ago by vabr (Chromium) Modified:
5 years, 10 months ago Reviewers:
vasilii CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@457646_fix_preferred_match Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClarify PasswordFormManager::OnRequestDone
As recent crashes proved, OnRequestDone was a bowl of spaghetti.
This is an attempt at straightening it up, by separating particular computational steps from one another.
BUG=457646, 451018
Committed: https://crrev.com/511209ccaec37cef530c52974b5edf8335ec2905
Cr-Commit-Position: refs/heads/master@{#317575}
Patch Set 1 #Patch Set 2 : Rebased on ToT #
Total comments: 8
Patch Set 3 : Just rebased #
Total comments: 1
Patch Set 4 : Comments addressed #
Total comments: 8
Patch Set 5 : Further comments addressed #Patch Set 6 : Just rebased #Messages
Total messages: 15 (3 generated)
vabr@chromium.org changed reviewers: + vasilii@chromium.org
Vasilii, Please check if the new version of OnRequestDone looks better to you. Note the clean score: 67 lines added, 67 lines removed. :) Cheers, Vaclav
I think your approach is fine. Though when we talked yesterday I imagined
another algorithm:
int best_credentials_to_keep_score = 0;
int best_best_matches_score = 0;
ScopedVector<PasswordForm> credentials_to_keep;
for (/* iterate */ logins_result) {
if (ShouldIgnoreResult(...))
continue;
if (/*should put to credentials_to_keep*/) {
// update best_credentials_to_keep_score
credentials_to_keep.push_back();
continue;
}
if (current_score < max(best_credentials_to_keep_score,
best_best_matches_score);
continue;
// update best_best_matches_score, best_matches,
// preferred_match_
}
if (best_best_matches_score <
best_credentials_to_keep_score) {
// drop best_matches, preferred_match_
}
// merge credentials_to_keep to best_matches
}
Though your algorithm is easier to understand.
https://codereview.chromium.org/921873002/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):
https://codereview.chromium.org/921873002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:425:
std::vector<int> credential_scores;
Call vector.reserve().
https://codereview.chromium.org/921873002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:444: return;
Are we sure that |best_matches_| and |preferred_match_| don't contain some trash
at this point?
https://codereview.chromium.org/921873002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:459: //
instead of explicitly handling empty path matches.
I see the significant difference from the original. Before we just ignored the
forms with ShouldIgnoreResult() == true. Now they can propagate to
|protected_credentials|. I checked the callee and I think it's a bug.
https://codereview.chromium.org/921873002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:462:
StartsWithASCII("/", logins_result[i]->origin.path(), true) &&
Why did you change this condition from the original?
Hi Vasilii, I addressed your comments, please have a look. But don't hurry, I won't be landing this sooner than the last week of February. I agree that this could have been implemented differently, my focus was on the code being easy to follow. I would prefer it that way, but am happy to consider any changes if you see why there would be an improvement. Cheers, Vaclav https://codereview.chromium.org/921873002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/921873002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:425: std::vector<int> credential_scores; On 2015/02/13 10:44:45, vasilii wrote: > Call vector.reserve(). Done. https://codereview.chromium.org/921873002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:444: return; On 2015/02/13 10:44:45, vasilii wrote: > Are we sure that |best_matches_| and |preferred_match_| don't contain some trash > at this point? Even if yes, it is better to be clear and explicit on that. I moved the clearing lines to the beginning of the method. Done. https://codereview.chromium.org/921873002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:459: // instead of explicitly handling empty path matches. On 2015/02/13 10:44:45, vasilii wrote: > I see the significant difference from the original. Before we just ignored the > forms with ShouldIgnoreResult() == true. Now they can propagate to > |protected_credentials|. I checked the callee and I think it's a bug. Good catch! You are right, this would allow, e.g., bypassing sync credentials protection if they are generated. I added an explicit check for non-negative score at the beginning of the loop. Done. https://codereview.chromium.org/921873002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:462: StartsWithASCII("/", logins_result[i]->origin.path(), true) && On 2015/02/13 10:44:45, vasilii wrote: > Why did you change this condition from the original? The original phrasing needed a comment to explain that the equality of the origin to the realm means that the origin has no path. Just checking for the empty path instead does not rely on the format of the realm, and is clear without the need for comments. There is a slight nuance in whether "/" is an empty path or not. The previous code actually only recognised "/" as an empty path, because the realm string ends with "/". I believe it makes sense to also allow the very empty path, so I changed the test to whether the path is a prefix of "/". https://codereview.chromium.org/921873002/diff/40001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/921873002/diff/40001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:498: corresponding_best_match = protege; Note that I could also try to populate the preferred_match_ if protege->preferred is true. But that would be a change against the old behaviour, which might have been intentional (low scoring could contradict being preferred).
https://codereview.chromium.org/921873002/diff/60001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:424: // First, |login_result| is filtered by score. The best scoring credentials This comment doesn't describe the loop below. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:435: if (ShouldIgnoreResult(*login)) Again, these prohibited forms shouldn't affect |best_score|. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:443: // equivalent in the future. The comment is obsolete. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:465: StartsWithASCII("/", logins_result[i]->origin.path(), true) && Please make sure it produces the right result for empty logins_result[i]->origin.path()
https://codereview.chromium.org/921873002/diff/60001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:424: // First, |login_result| is filtered by score. The best scoring credentials This comment doesn't describe the loop below. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:435: if (ShouldIgnoreResult(*login)) Again, these prohibited forms shouldn't affect |best_score|. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:443: // equivalent in the future. The comment is obsolete. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:465: StartsWithASCII("/", logins_result[i]->origin.path(), true) && Please make sure it produces the right result for empty logins_result[i]->origin.path()
Thanks, Vasilii. I answered your comments, PTAL. Cheers, Vaclav https://codereview.chromium.org/921873002/diff/60001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:424: // First, |login_result| is filtered by score. The best scoring credentials On 2015/02/13 14:10:21, vasilii wrote: > This comment doesn't describe the loop below. Done. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:435: if (ShouldIgnoreResult(*login)) On 2015/02/13 14:10:21, vasilii wrote: > Again, these prohibited forms shouldn't affect |best_score|. Ah, I missed that. Thanks for pointing that out. Done. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:443: // equivalent in the future. On 2015/02/13 14:10:21, vasilii wrote: > The comment is obsolete. Done. https://codereview.chromium.org/921873002/diff/60001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:465: StartsWithASCII("/", logins_result[i]->origin.path(), true) && On 2015/02/13 14:10:21, vasilii wrote: > Please make sure it produces the right result for empty > logins_result[i]->origin.path() It returns true if and only if the path is empty or "/". That is different from the old code (which only accepted "/"), but I believe correct (i.e., the old code was wrong). Do you have different opinion on what is the right result?
lgtm
Thanks! I will hold on with landing it until I'm back to ensure quick revert on issues. Cheers, Vaclav
New patchsets have been uploaded after l-g-t-m from vasilii@chromium.org
I'm back, and just rebased the last patch set. If the trybots look good, I'll CQ this. Thanks, Vaclav
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/921873002/90001
Message was sent while issue was closed.
Committed patchset #6 (id:90001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/511209ccaec37cef530c52974b5edf8335ec2905 Cr-Commit-Position: refs/heads/master@{#317575} |
