Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2523593006:
Show PSL-matched credentials in the account chooser. (Closed)
DescriptionShow PSL-matched credentials in the account chooser.
They are shown only when there is nothing else to show. PSL matches are never returned without user mediation.
BUG=666340
Committed: https://crrev.com/47514a069de8bf885e6d0bcdb6585ad69b5cb9ed
Cr-Commit-Position: refs/heads/master@{#434345}
Patch Set 1 #
Total comments: 13
Patch Set 2 : add tests #
Total comments: 2
Patch Set 3 : nit #
Messages
Total messages: 27 (15 generated)
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vasilii@chromium.org changed reviewers: + jdoerrie@chromium.org, vabr@chromium.org
Hi guys, please review.
LGTM, thanks! https://codereview.chromium.org/2523593006/diff/1/components/password_manager... File components/password_manager/content/browser/credential_manager_impl_unittest.cc (right): https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/content/browser/credential_manager_impl_unittest.cc:741: subdomain_form_.skip_zero_click = false; Is the skip_zero_click bit being false a part of the contract? Or is the get() call expected to ignore that value and pretend it is false? In the first case, we should maybe point the contract out in a comment. In the second case, we may need to do two tests, for both values of the flag, to verify that the get() call handles them correctly. https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/content/browser/credential_manager_impl_unittest.cc:768: // Add 8 credentials. Two buckets of duplicates, one empty username and one I'm wondering if this particular test is trying to test too much at once. It has used PSL matches, but did not even mention them in the name or the comment here. Would it be clearer to have a separate test that would verify that in a mix of PSL and non-PSL matches only the latter is shown, without also testing duplicates etc.? https://codereview.chromium.org/2523593006/diff/1/components/password_manager... File components/password_manager/core/browser/credential_manager_pending_request_task.cc (right): https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/core/browser/credential_manager_pending_request_task.cc:193: get_result = metrics_util::CREDENTIAL_MANAGER_GET_NONE_MANY_CREDENTIALS; This did not change in this CL, but should we extend the meaning of CREDENTIAL_MANAGER_GET_NONE_MANY_CREDENTIALS to also include the case of having just 1 credential stored, but auto sign-in not being allowed? Or is there a reason why we know that at this point !can_use_autosignin always means local_results.size() > 1u? https://codereview.chromium.org/2523593006/diff/1/components/password_manager... File components/password_manager/core/browser/test_password_store.cc (right): https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/core/browser/test_password_store.cc:93: bool is_psl = optional nit: const bool (Since you did a similar thing elsewhere in this patch, and I personally like making constness clear. No strong feelings, though, hence optional.) https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/core/browser/test_password_store.cc:94: (elements.first != form.signon_realm && nit: Are the outer parentheses needed? https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/core/browser/test_password_store.cc:94: (elements.first != form.signon_realm && nit: Would you consider caching the results of the test on line 89 (repeated on 94) and on line 92 (repeated on 95) in two bool constants? My concern is mostly readability (rather than performance) -- replacing them with one-word expressions like, e.g., |realm_matches| and |realm_psl_matches| could make the conditions in the if and the is_psl definition simpler, and their relation easier to understand.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I rewrote the tests a bit. https://codereview.chromium.org/2523593006/diff/1/components/password_manager... File components/password_manager/content/browser/credential_manager_impl_unittest.cc (right): https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/content/browser/credential_manager_impl_unittest.cc:741: subdomain_form_.skip_zero_click = false; On 2016/11/23 18:49:11, vabr (Chromium) wrote: > Is the skip_zero_click bit being false a part of the contract? Or is the get() > call expected to ignore that value and pretend it is false? > > In the first case, we should maybe point the contract out in a comment. > > In the second case, we may need to do two tests, for both values of the flag, to > verify that the get() call handles them correctly. It's the second. The test should pass with any value of the flag. skip_zero_click == false makes it more likely to fail. I can't think of an implementation where the test passes with skip_zero_click == false and fails with skip_zero_click == true. I added a test to check that PSL credentials are not returned for zero-click only get(). https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/content/browser/credential_manager_impl_unittest.cc:768: // Add 8 credentials. Two buckets of duplicates, one empty username and one On 2016/11/23 18:49:11, vabr (Chromium) wrote: > I'm wondering if this particular test is trying to test too much at once. It has > used PSL matches, but did not even mention them in the name or the comment here. > > Would it be clearer to have a separate test that would verify that in a mix of > PSL and non-PSL matches only the latter is shown, without also testing > duplicates etc.? Done. See CredentialManagerOnRequestCredentialWithPSLAndNormalCredentials https://codereview.chromium.org/2523593006/diff/1/components/password_manager... File components/password_manager/core/browser/credential_manager_pending_request_task.cc (right): https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/core/browser/credential_manager_pending_request_task.cc:193: get_result = metrics_util::CREDENTIAL_MANAGER_GET_NONE_MANY_CREDENTIALS; On 2016/11/23 18:49:11, vabr (Chromium) wrote: > This did not change in this CL, but should we extend the meaning of > CREDENTIAL_MANAGER_GET_NONE_MANY_CREDENTIALS to also include the case of having > just 1 credential stored, but auto sign-in not being allowed? Or is there a > reason why we know that at this point !can_use_autosignin always means > local_results.size() > 1u? It's a very good question! Yes, we know that local_results.size() > 1u. The case when zero_click_only_ == true and !delegate_->IsZeroClickAllowed() is handled in CredentialManagerImpl::Get(). https://codereview.chromium.org/2523593006/diff/1/components/password_manager... File components/password_manager/core/browser/test_password_store.cc (right): https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/core/browser/test_password_store.cc:93: bool is_psl = On 2016/11/23 18:49:11, vabr (Chromium) wrote: > optional nit: > const bool > (Since you did a similar thing elsewhere in this patch, and I personally like > making constness clear. No strong feelings, though, hence optional.) Done. https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/core/browser/test_password_store.cc:94: (elements.first != form.signon_realm && On 2016/11/23 18:49:11, vabr (Chromium) wrote: > nit: Would you consider caching the results of the test on line 89 (repeated on > 94) and on line 92 (repeated on 95) in two bool constants? My concern is mostly > readability (rather than performance) -- replacing them with one-word > expressions like, e.g., |realm_matches| and |realm_psl_matches| could make the > conditions in the if and the is_psl definition simpler, and their relation > easier to understand. Done. https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/core/browser/test_password_store.cc:94: (elements.first != form.signon_realm && On 2016/11/23 18:49:11, vabr (Chromium) wrote: > nit: Are the outer parentheses needed? Done.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the rewrite, LGTM! Vaclav https://codereview.chromium.org/2523593006/diff/1/components/password_manager... File components/password_manager/content/browser/credential_manager_impl_unittest.cc (right): https://codereview.chromium.org/2523593006/diff/1/components/password_manager... components/password_manager/content/browser/credential_manager_impl_unittest.cc:741: subdomain_form_.skip_zero_click = false; On 2016/11/24 12:44:10, vasilii wrote: > On 2016/11/23 18:49:11, vabr (Chromium) wrote: > > Is the skip_zero_click bit being false a part of the contract? Or is the get() > > call expected to ignore that value and pretend it is false? > > > > In the first case, we should maybe point the contract out in a comment. > > > > In the second case, we may need to do two tests, for both values of the flag, > to > > verify that the get() call handles them correctly. > > It's the second. The test should pass with any value of the flag. > skip_zero_click == false makes it more likely to fail. I can't think of an > implementation where the test passes with skip_zero_click == false and fails > with skip_zero_click == true. > I added a test to check that PSL credentials are not returned for zero-click > only get(). Acknowledged. https://codereview.chromium.org/2523593006/diff/20001/components/password_man... File components/password_manager/core/browser/test_password_store.cc (right): https://codereview.chromium.org/2523593006/diff/20001/components/password_man... components/password_manager/core/browser/test_password_store.cc:89: const bool realm_matches = (elements.first == form.signon_realm); optional nit: Parentheses not needed (unless you find them helpful to parse the sequence = == correctly).
https://codereview.chromium.org/2523593006/diff/20001/components/password_man... File components/password_manager/core/browser/test_password_store.cc (right): https://codereview.chromium.org/2523593006/diff/20001/components/password_man... components/password_manager/core/browser/test_password_store.cc:89: const bool realm_matches = (elements.first == form.signon_realm); On 2016/11/24 13:58:14, vabr (Chromium) wrote: > optional nit: Parentheses not needed (unless you find them helpful to parse the > sequence = == correctly). Done.
The CQ bit was checked by vasilii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2523593006/#ps40001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480004473210760,
"parent_rev": "00e7f2694cdaf610dcbe257ca4239207f400211d", "commit_rev":
"051bde4c1d4402ed68dfa22f2ef25bcce7e99d0b"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Show PSL-matched credentials in the account chooser. They are shown only when there is nothing else to show. PSL matches are never returned without user mediation. BUG=666340 ========== to ========== Show PSL-matched credentials in the account chooser. They are shown only when there is nothing else to show. PSL matches are never returned without user mediation. BUG=666340 Committed: https://crrev.com/47514a069de8bf885e6d0bcdb6585ad69b5cb9ed Cr-Commit-Position: refs/heads/master@{#434345} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/47514a069de8bf885e6d0bcdb6585ad69b5cb9ed Cr-Commit-Position: refs/heads/master@{#434345} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
