Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1615653005:
[Password manager] Human readable origins for Android credentials on chrome://settings/passwords (Closed)
|
Created:
4 years, 11 months ago by kolos1 Modified:
4 years, 8 months ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password manager] Human readable origins for Android credentials on chrome://settings/passwords
PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. The request doesn't issue an on-demand network request. If a request to cache fails, no web realm will be injected into corresponding form.
If there are a number of affiliated web realms, an arbitrary realm is injected.
Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm.
BUG=593341
Committed: https://crrev.com/983af5012da174a6a50c38ea956e2e1422646995
Cr-Commit-Position: refs/heads/master@{#384236}
Patch Set 1 : #Patch Set 2 : #
Total comments: 1
Patch Set 3 : Transfer multi-request logic from PasswordStore to AffiliatedMatchHelper #
Total comments: 8
Patch Set 4 : GetShownOrigin also returns boolean is_android_uri. Appending " (Android)" was transfered from GetS… #Patch Set 5 : Fixed tests for GetShownOriginAndLinkUrl #
Total comments: 22
Patch Set 6 : Changes addressed to reviewer comments #Patch Set 7 : Some polishing changes #Patch Set 8 : Added "DCHECK(forms);" to AffiliatedMatchHelper/MockAffiliatedMatchHelper::InjectAffiliatedWebReal… #Patch Set 9 : Patch with debug output to fix a failure on Win bot. #Patch Set 10 : One more debug patch #Patch Set 11 : Debug patch (ScopedVector<PasswordForm>* forms2 = forms.get()) #Patch Set 12 : Fixes failure of Windows bot #
Total comments: 13
Patch Set 13 : Fixes another failure of Win bot #
Total comments: 11
Patch Set 14 : #
Total comments: 8
Patch Set 15 : Small changes addressed to reviewer comments #
Total comments: 6
Patch Set 16 : Adapt to new implementation of left elided origins #
Total comments: 8
Patch Set 17 : Introduced localization for "(Android)". Created createUrlDiv() to remove duplicate code #Patch Set 18 : Removed unused setters in PasswordExceptionsListItem and PasswordListItem. #
Total comments: 6
Patch Set 19 : Inlined createAndroidUriSuffix. Added the check for cellWidth #Patch Set 20 : Added a space to (Android) in generated_resources.grd. Removed CSS class android-uri-suffix #
Total comments: 1
Patch Set 21 : Inlined the variable androidUriSuffix #Messages
Total messages: 92 (29 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
kolos@chromium.org changed reviewers: + engedy@chromium.org
Hi Balazs, Please review this CL for human readable origins for Android credentials. Files for review: affiliated_match_helper.* password_store.* password_store_consumer.* password_ui_utils.* At this point, you are the only reviewer. I will add others when we check the most important Android API part. I will also finish tests soon. Regards, Maxim
Patchset #1 (id:120001) has been deleted
Sorry for the delay. I will go through the CL again tomorrow in more detail, but overall, I think this is the right approach. It is also a very nice usage of BarrierClosures! On the other hand, it does seem to add quite some complexity to PasswordStore. Have you considered going down the road of adding support to AffiliatedMatchHelper and AffiliationService to fetch data for multiple forms at once? I initially thought that doing that would be more work, but now I am not so sure. https://codereview.chromium.org/1615653005/diff/160001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1615653005/diff/160001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:42: if (password_form.affiliated_web_realm.empty()) { I think it would be interesting to add boolean UMA histogram to measure how often we hit this branch.
On 2016/02/19 01:25:52, engedy wrote: > Sorry for the delay. > > I will go through the CL again tomorrow in more detail, but overall, I think > this is the right approach. It is also a very nice usage of BarrierClosures! Thanks. > On the other hand, it does seem to add quite some complexity to PasswordStore. Yes, I considered your note on moving logic to PasswordManagerPresenter. But it seems that this logic should be in PasswordStore, because a request for affiliated web realms is similar to other requests (e.g. GetAutofillableLogins, GetSiteStats). > Have you considered going down the road of adding support to > AffiliatedMatchHelper and AffiliationService to fetch data for multiple forms at > once? I initially thought that doing that would be more work, but now I am not > so sure. I considered this note too. Yes, we have to change a chain of calls (AffiliatedMatchHelper, AffiliationService, AffiliationBackend, etc.). I concluded that it is better to collect all realms at PasswordStore. It was my conclusions, but I am open to discussion on how to improve the code.
Sorry for the delay again. As discussed offline, the overall approach looks good to me, the one thing I'd like to change is to reduce the amount of code and responsibilities that we add into PasswordStore, in an effort to prevent it becoming a "centralized monstrosity that does everything other classes were reluctant to do" (TM). To this effect, we should push the BarrierClosure logic (which collects affiliations for multiple Android forms) down to at least AffiliatedMatchHelper. That is, we could introduce a version of AffiliatedMatchHelper::GetAffiliatedWebRealms that takes a vector of autofill::PasswordForm's, and returns a map from Android signon_realms to Web signon_realms. It is probably best to implement this in terms of the existing AffiliatedMatchHelper::GetAffiliatedWebRealms, but I leave that up to you. Ultimately, the logic for querying multiple realms at once should live in AffiliationBackend/FacetManager, but that's quite tricky. For now, doing it in AffiliatedMatchHelper is indeed somewhat less efficient (especially the extra posted tasks), but given that most users will only have few Android logins, this will be far from being a performance bottleneck, and is much simpler to implement. On a related note, back in PasswordStore, we may consider optimizing out the NotifyWithAffiliatedWebRealms() part as well. We could load the affiliated Web realms into PasswordForms in GetAutofillableLoginsImpl, in a way similar to how GetLoginsWithAffiliationsImpl currently works. That is, it would do an extra asynchronous round-trip to AffiliatedMatchHelper, and only notify the GetLoginRequest once that data has been received as well. We could also let PasswordStore::GetAutofillableLoginsImpl take an extra boolean indicating whether the caller is interested in getting the affiliated web sites looked up. For instance, when Sync calls into FillAutofillableLogins, then it probably does not need this information.
Patchset #3 (id:180001) has been deleted
Patchset #5 (id:240001) has been deleted
Patchset #3 (id:200001) has been deleted
Patchset #3 (id:220001) has been deleted
Hi Balazs, As we discussed, I transferred multi-request logic from PasswordStore to AffiliationMatchHelper. Please review the changes. Regards, Maxim
Looks really good, a couple a minor comments. Could you please also extend the CL description a bit? https://codereview.chromium.org/1615653005/diff/260001/components/autofill/co... File components/autofill/core/common/password_form.h (right): https://codereview.chromium.org/1615653005/diff/260001/components/autofill/co... components/autofill/core/common/password_form.h:112: // realms affiliated with the application, random realm is chosen. nit: +web+ realm Furthermore, could you please mention that this is not always filled out, and elaborate on when it is filled out? https://codereview.chromium.org/1615653005/diff/260001/components/password_ma... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/1615653005/diff/260001/components/password_ma... components/password_manager/core/browser/password_store.cc:413: scoped_ptr<ScopedVector<PasswordForm>> obtained_forms( Note that ScopedVector is already a move-only type, so you should be able to use it in std::move() and base::Passed(), and not need to wrap it in a scoped_ptr. https://codereview.chromium.org/1615653005/diff/260001/components/password_ma... components/password_manager/core/browser/password_store.cc:425: void PasswordStore::FillAffiliatedWebRealms( Naming nit: At least in the scope of the PasswordStore, "fill" refers to synchronous operations that do the actual work, and are called from the asynchronous wrapper methods. How about incorporate, include, inject? https://codereview.chromium.org/1615653005/diff/260001/components/password_ma... components/password_manager/core/browser/password_store.cc:437: void PasswordStore::NotifyLoginsWithAffiliatedRealms( I think we could make this a non-member function defined in an anonymous namespace in this CC file. Alternatively, I think you could write: base::Bind(&PS::GLR::NotifyConsumerWithResults, base::Owned(request.release()), ...) https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:69: DCHECK(link_url.is_valid()); Hang on, how does this not fail for empty |link_url| for Android credentials without matching web realms? https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:76: entry->SetBoolean(kIsSecureField, content::IsOriginSecure(link_url)); Nit: FWIW, Android credentials are always secure too, even if we don't know the affiliated web realms. https://codereview.chromium.org/1615653005/diff/300001/components/autofill/co... File components/autofill/core/common/password_form.h (right): https://codereview.chromium.org/1615653005/diff/300001/components/autofill/co... components/autofill/core/common/password_form.h:112: // realms affiliated with the application, random realm is chosen. nit: ... an arbitrary realm https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper.h (right): https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.h:87: void FillAffiliatedWebRealms( Have you considered making this method take and return (in the callback) a ScopedVector by value (it's a move-only type). I think that could make the semantics of this method cleaner. Also, please add a unittest both for this method (in affiliated_match_helper_unittest), and also the machinery that uses it in PasswordStore (in password_store_unittest). nit: Could you please mention in the comment that the methods works with whatever's in the cache, and no on-demand network requests will be issued? nit: I would also suggest renaming this method, see my other comment about connotations of "fill". https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:34: password_manager::FacetURI facet_uri = You could use a shortcut here: IsValidAndroidFacetURI(password_form.signon_realm); https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:37: *is_android_uri = facet_uri.IsValidAndroidFacetURI(); How about: if(IsValidAndroidFacetURI(password_form.signon_realm)) { *is_android_uri = true; ... Don't forget to assign *is_android_uri = false otherwise. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:43: } else { nit: The style guide discourages "else after return", please promote the body of the else branch one indentation level. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:47: android_form.affiliated_web_realm = ""; nit: Avoid using empty string literals. Some STL implementations used to create std::string's with buffers of size 1 to store the terminating zero. I'm not sure if that's still the case, but normally it is advised to use either " = std::string()", or ".clear()". https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:52: EXPECT_EQ(GetShownOriginAndLinkUrl(android_form, "", &is_android_uri, Swap order of arguments, first the expectation, then the actual value. Same below, 3 places overall.
Patchset #6 (id:320001) has been deleted
Description was changed from ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords ========== to ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. ==========
Description was changed from ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. ========== to ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. The request doesn't issue an on-demand network request. If a request to cache fails, no web realm will be injected into corresponding form. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. ==========
kolos@chromium.org changed reviewers: + vabr@chromium.org
vabr@chromium.org: Please review changes in password_manager_presenter.cc and password_form.* engedy@chromium.org: Please review changes addressed to your previous comments. https://codereview.chromium.org/1615653005/diff/260001/components/autofill/co... File components/autofill/core/common/password_form.h (right): https://codereview.chromium.org/1615653005/diff/260001/components/autofill/co... components/autofill/core/common/password_form.h:112: // realms affiliated with the application, random realm is chosen. On 2016/03/02 14:22:10, engedy wrote: > nit: +web+ realm > > Furthermore, could you please mention that this is not always filled out, and > elaborate on when it is filled out? Done. https://codereview.chromium.org/1615653005/diff/260001/components/password_ma... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/1615653005/diff/260001/components/password_ma... components/password_manager/core/browser/password_store.cc:413: scoped_ptr<ScopedVector<PasswordForm>> obtained_forms( Yes, it's is possible to pass ScopedVector, but I need a wrapped vector in PasswordStore::FillAffiliatedWebRealms(InjectAffiliatedWebRealms). I need to provide a pointer to the vector into affiliated_match_helper_->FillAffiliatedWebRealms(InjectAffiliatedWebRealms), but ownership of the vector should be passed to the callback (PasswordStore::NotifyLoginsWithAffiliatedRealms). It is why I wrapped ScopedVector. https://codereview.chromium.org/1615653005/diff/260001/components/password_ma... components/password_manager/core/browser/password_store.cc:425: void PasswordStore::FillAffiliatedWebRealms( Renamed to Inject... https://codereview.chromium.org/1615653005/diff/260001/components/password_ma... components/password_manager/core/browser/password_store.cc:437: void PasswordStore::NotifyLoginsWithAffiliatedRealms( On 2016/03/02 14:22:10, engedy wrote: > I think we could make this a non-member function defined in an anonymous > namespace in this CC file. I would prefer to left it here since we have similar methods in PasswordStore (NotifySiteStats, NotifyLoginsChanged). > base::Bind(&PS::GLR::NotifyConsumerWithResults, base::Owned(request.release()), > ...) Yes, it would be nice to get rid of NotifyLoginsWithAffiliatedRealms, but I need to keep the wrapper of ScopedVector (i.e. scoped_ptr). Otherwise, affiliated_match_helper_->FillAffiliatedWebRealms wouldn't receive the content of ScopedVector (i.e. |*forms|). So, I need some method to unwrap ScopedVector. https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:69: DCHECK(link_url.is_valid()); It is not always PasswordForm.affiliated_web_realm. See into GetShownOriginAndLinkUrl: a) For non-Android forms: link_url is PasswordForm.origin. b) For Android forms with non-empty affiliated_web_realm: it is affiliated_web_realm. c) For Android forms with empty affiliated_web_realm: it is PasswordForm.signon_realm. Could signon_realm be invalid URL? https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:76: entry->SetBoolean(kIsSecureField, content::IsOriginSecure(link_url)); Thanks. Is it possible that the scheme of affiliated web realm is http? link_url is the URL where user will go to if he clicks on the origin on password page. If we are going to show where the ulr is secure, let just check it. But don't consider various cases. https://codereview.chromium.org/1615653005/diff/300001/components/autofill/co... File components/autofill/core/common/password_form.h (right): https://codereview.chromium.org/1615653005/diff/300001/components/autofill/co... components/autofill/core/common/password_form.h:112: // realms affiliated with the application, random realm is chosen. On 2016/03/02 14:22:10, engedy wrote: > nit: ... an arbitrary realm Done. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper.h (right): https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.h:87: void FillAffiliatedWebRealms( On 2016/03/02 14:22:10, engedy wrote: > Have you considered making this method take and return (in the callback) a > ScopedVector by value (it's a move-only type). I think that could make the > semantics of this method cleaner. > > Also, please add a unittest both for this method (in > affiliated_match_helper_unittest), and also the machinery that uses it in > PasswordStore (in password_store_unittest). > > nit: Could you please mention in the comment that the methods works with > whatever's in the cache, and no on-demand network requests will be issued? > > nit: I would also suggest renaming this method, see my other comment about > connotations of "fill". All done. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:34: password_manager::FacetURI facet_uri = I will need facet_uri later (for GetHumanReadableOriginForAndroidUri) https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:37: *is_android_uri = facet_uri.IsValidAndroidFacetURI(); To be sure we always initialize *is_android_uri, I keep assignment before if. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:43: } else { On 2016/03/02 14:22:10, engedy wrote: > nit: The style guide discourages "else after return", please promote the body of > the else branch one indentation level. Done. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:47: android_form.affiliated_web_realm = ""; On 2016/03/02 14:22:10, engedy wrote: > nit: Avoid using empty string literals. Some STL implementations used to create > std::string's with buffers of size 1 to store the terminating zero. I'm not > sure if that's still the case, but normally it is advised to use either " = > std::string()", or ".clear()". Done. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:52: EXPECT_EQ(GetShownOriginAndLinkUrl(android_form, "", &is_android_uri, On 2016/03/02 14:22:10, engedy wrote: > Swap order of arguments, first the expectation, then the actual value. Same > below, 3 places overall. Done.
Changes in password_manager_presenter.cc and password_form.* LGTM. Cheers, Vaclav
LGTM on components/password_manager/* modulo comments. Thanks! https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:69: DCHECK(link_url.is_valid()); On 2016/03/07 10:47:00, kolos1 wrote: > It is not always PasswordForm.affiliated_web_realm. See into > GetShownOriginAndLinkUrl: > a) For non-Android forms: link_url is PasswordForm.origin. > b) For Android forms with non-empty affiliated_web_realm: it is > affiliated_web_realm. > c) For Android forms with empty affiliated_web_realm: it is > PasswordForm.signon_realm. Could signon_realm be invalid URL? Please see my other comment. GetShownOriginAndLinkUrl() should return an empty GURL as |link_url| in case (c). https://codereview.chromium.org/1615653005/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:76: entry->SetBoolean(kIsSecureField, content::IsOriginSecure(link_url)); On 2016/03/07 10:47:00, kolos1 wrote: > Thanks. > > Is it possible that the scheme of affiliated web realm is http? No, HTTP URLs are not valid facet URIs. > link_url is the URL where user will go to if he clicks on the origin on password > page. If we are going to show where the ulr is secure, let just check it. But > don't consider various cases. Let's discuss this offline tomorrow. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:34: password_manager::FacetURI facet_uri = On 2016/03/07 10:47:00, kolos1 wrote: > I will need facet_uri later (for GetHumanReadableOriginForAndroidUri) Acknowledged. https://codereview.chromium.org/1615653005/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:37: *is_android_uri = facet_uri.IsValidAndroidFacetURI(); On 2016/03/07 10:47:00, kolos1 wrote: > To be sure we always initialize *is_android_uri, I keep assignment before if. Acknowledged. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper.cc (right): https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.cc:94: for (auto* form : *forms) style nit: Please put the body of the loop/conditional into {}'es whenever the body or the () part of the loop/conditional statement is more that one line. In multiple places in this file. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.cc:108: void AffiliatedMatchHelper::InjectAffiliatedWebRealm( nit: For consistency, could you please rename this to: CompleteInjectAffiliatedWebRealm() https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:436: // InjectAffiliatedWebRealms test verifies that InjectAffiliatedWebRealms() nit: -"InjectAffiliatedWebRealms test" https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:438: // into password forms. nit: ... , if any. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:465: EXPECT_EQ(number_of_forms, forms.size()); Please make this an ASSERT_EQ, so that if |forms| is shorter than expected, the test still won't crash. tiny nit: In general, tests should contain as little logic as possible to avoid silent not-doing-anything tests because of a bug in the tests themselves. Here, I would just hardcode the constant: ASSERT_EQ(4u, forms.size()); https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:468: EXPECT_THAT(forms[1]->affiliated_web_realm, kTestWebRealmBeta1); nit: EXPECT_EQ, or EXPECT_THAT(..., testing:Eq(...)), and below https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper.cc (right): https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.cc:89: void AffiliatedMatchHelper::InjectAffiliatedWebRealms( As discussed offline, it would probably be nicer to pass the ScopedVector<> into this method first (and take ownership), and also pass in a Callback that awaits to-be-owned ScopedVector as the result. Then, we could first collect the list of PasswordForm*'s that represent Android credentials, and then pass/bind the ScopedVector into the |result_callback|. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_store.h:161: // Same as above, but also fetches web realms affiliated with Android phrasing nit: I would refrain using the words "fetch" and "request" in thes comments, as there are no network request being made (and should not be made). How about: // Same as above, but also fills in |affiliated_web_realm| for Android credentials. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_store.h:374: // Same as above, but also makes a request to fill affiliated web realms in Same phrasing nit here. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_store.h:404: // Send a request to AffiliationMatchHelper to fetch affiliated web realms and Phrasing nit: How about: Retrieves and fills in |affiliated_web_realm| values for Android credentials in |forms|. Called on the main thread. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:41: *link_url = GURL(password_form.signon_realm); We should return an empty |link_url| is not |origin_is_clickable|. An Android signon_realm is semantically not a valid GURL (technically it is, but that's an accident). https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:27: // |*origin_is_clickable| is set to true, except for Android forms with empty nit: Also mention that |link_url| will be empty in this case.
Great thanks for your comment, Balazs. Please review new changes. Regards, Maxim https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper.cc (right): https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.cc:94: for (auto* form : *forms) On 2016/03/08 17:36:38, engedy wrote: > style nit: Please put the body of the loop/conditional into {}'es whenever the > body or the () part of the loop/conditional statement is more that one line. > > In multiple places in this file. Done. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.cc:108: void AffiliatedMatchHelper::InjectAffiliatedWebRealm( On 2016/03/08 17:36:38, engedy wrote: > nit: For consistency, could you please rename this to: > > CompleteInjectAffiliatedWebRealm() Done. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:436: // InjectAffiliatedWebRealms test verifies that InjectAffiliatedWebRealms() On 2016/03/08 17:36:38, engedy wrote: > nit: -"InjectAffiliatedWebRealms test" Done. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:438: // into password forms. On 2016/03/08 17:36:38, engedy wrote: > nit: ... , if any. Done. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:465: EXPECT_EQ(number_of_forms, forms.size()); On 2016/03/08 17:36:38, engedy wrote: > Please make this an ASSERT_EQ, so that if |forms| is shorter than expected, the > test still won't crash. Done. > tiny nit: In general, tests should contain as little logic as possible to avoid > silent not-doing-anything tests because of a bug in the tests themselves. Here, > I would just hardcode the constant: > > ASSERT_EQ(4u, forms.size()); I believe that it is better to use a variable. Because: - It is clearer what this number means. - It says that the number of forms shouldn't be changed. - There is no need to change the constant if somebody added new forms to |forms|. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:468: EXPECT_THAT(forms[1]->affiliated_web_realm, kTestWebRealmBeta1); On 2016/03/08 17:36:38, engedy wrote: > nit: EXPECT_EQ, or EXPECT_THAT(..., testing:Eq(...)), and below Done. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_store.h:161: // Same as above, but also fetches web realms affiliated with Android On 2016/03/08 17:36:38, engedy wrote: > phrasing nit: I would refrain using the words "fetch" and "request" in thes > comments, as there are no network request being made (and should not be made). > > How about: > > // Same as above, but also fills in |affiliated_web_realm| for Android > credentials. Done. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_store.h:374: // Same as above, but also makes a request to fill affiliated web realms in On 2016/03/08 17:36:38, engedy wrote: > Same phrasing nit here. Done. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_store.h:404: // Send a request to AffiliationMatchHelper to fetch affiliated web realms and On 2016/03/08 17:36:38, engedy wrote: > Phrasing nit: How about: > > Retrieves and fills in |affiliated_web_realm| values for Android credentials in > |forms|. Called on the main thread. Done. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:41: *link_url = GURL(password_form.signon_realm); |link_url| is what we show in the tooltip. A user should be able to see the full url (even if it is not clickable).
Really nicely done, thank you! LGTM on components/password_manager/*, with 3 naming nits. Note that I am not an OWNER of chrome/browser/. https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/460001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:465: EXPECT_EQ(number_of_forms, forms.size()); On 2016/03/09 13:23:55, kolos1 wrote: > On 2016/03/08 17:36:38, engedy wrote: > > Please make this an ASSERT_EQ, so that if |forms| is shorter than expected, > the > > test still won't crash. > Done. > > > tiny nit: In general, tests should contain as little logic as possible to > avoid > > silent not-doing-anything tests because of a bug in the tests themselves. > Here, > > I would just hardcode the constant: > > > > ASSERT_EQ(4u, forms.size()); > I believe that it is better to use a variable. Because: > - It is clearer what this number means. > - It says that the number of forms shouldn't be changed. > - There is no need to change the constant if somebody added new forms to > |forms|. Acknowledged. https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1615653005/diff/480001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:41: *link_url = GURL(password_form.signon_realm); On 2016/03/09 13:23:55, kolos1 wrote: > |link_url| is what we show in the tooltip. A user should be able to see the full > url (even if it is not clickable). > Acknowledged. https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper.h (right): https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.h:58: FormsCallback; nit: PasswordFormsCallback https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:471: size_t number_of_forms = forms.size(); nit: expected_form_count https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:856: // affiliated realms to UI thread, don't shutdown UI thread nit: Premature line break. https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:857: // until there is no tasks in the UI queue. nit: there are
One more nit from vabr: Could we get a bug for this and a link to it in the CL description? Thanks! Vaclav
Description was changed from ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. The request doesn't issue an on-demand network request. If a request to cache fails, no web realm will be injected into corresponding form. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. ========== to ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. The request doesn't issue an on-demand network request. If a request to cache fails, no web realm will be injected into corresponding form. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. BUG=593341 ==========
kolos@chromium.org changed reviewers: + estade@chromium.org
estade@chromium.org: Please review changes in password_manager_handler.cc and password_manager*.js. vabr@chromium.org: added a bug for this issue. engedy@chromium.org: made some changes address to your recent comments. https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper.h (right): https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper.h:58: FormsCallback; On 2016/03/09 14:15:04, engedy wrote: > nit: PasswordFormsCallback Done. https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... File components/password_manager/core/browser/affiliated_match_helper_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... components/password_manager/core/browser/affiliated_match_helper_unittest.cc:471: size_t number_of_forms = forms.size(); On 2016/03/09 14:15:04, engedy wrote: > nit: expected_form_count Done. https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:856: // affiliated realms to UI thread, don't shutdown UI thread On 2016/03/09 14:15:04, engedy wrote: > nit: Premature line break. Done. https://codereview.chromium.org/1615653005/diff/500001/components/password_ma... components/password_manager/core/browser/password_store_unittest.cc:857: // until there is no tasks in the UI queue. On 2016/03/09 14:15:05, engedy wrote: > nit: there are Done.
estade@chromium.org: friendly ping.
https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:86: linkText += ' )Android('; how do you know this is RTL? And wouldn't proper bidi handling mean that the parens take on the directionality of the closest strongly directional character ('A' or 'd')
https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:86: linkText += ' )Android('; This is always RTL, because of CSS class left_elided_url (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...) Sorry, I don't understand the rest of comment. What do you mean?
https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:86: linkText += ' )Android('; I think I understand. The parens should inherit direction from the closest character, right? I didn't see anything like that.
estade@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:86: linkText += ' )Android('; On 2016/03/15 19:36:16, kolos1 wrote: > I think I understand. The parens should inherit direction from the closest > character, right? I didn't see anything like that. OK, so, you're reversing the string in JS then overriding the bidi handling. Now you have to use the wrong semantic brackets. I don't like this solution. How does it affect screen readers? Maybe dbeam has some ideas.
https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:86: linkText += ' )Android('; Yes, the solution is a bit confusing, because we had to override bidi for hostnames elided from the left. For screen readers, it will look like "accounts.nytimes.com (Android)". As I understand, it will be a temporary solution since the password page will be converted into material design.
On 2016/03/15 21:00:41, kolos1 wrote: > https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... > File chrome/browser/resources/options/password_manager_list.js (right): > > https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... > chrome/browser/resources/options/password_manager_list.js:86: linkText += ' > )Android('; > Yes, the solution is a bit confusing, because we had to override bidi for > hostnames elided from the left. > > For screen readers, it will look like "accounts.nytimes.com (Android)". As I > understand, it will be a temporary solution since the password page will be > converted into material design. you tested this with a screen reader? How does the screen reader know to un-reverse the text?
I misunderstood what screen reader means. I have just tested CL with ChromeVox. Since we reversed the direction of origins, screen reader pronounces origins in reversed order, that is wrong. I asked ChromeVox team whether we could override text pronounced by screen readers.
Since another CL introduced wrong behavior of screen readers, I created a separate bug (crbug.com/595276) and resolve it in upcoming weeks. We are going to have a meeting with Material Design team to discuss a number of upcoming changes on chrome://settings/passwords. Maybe we will not need left elided origins in MD, so origins come back to LTR order. Or maybe we just roll back CL introduced left elided origins (i.e. RTL direction) and remove all related tricky hacks. Could you review changes related to new view of Android credentials, please? So, we will not mix a number of bugs in the same issue. Thank you very much to point at issues with screen readers. Best regards, Maxim
Created CL (https://codereview.chromium.org/1803353002) to fix screen reader issue.
ping dbeam --- is there an easy fix apparent to you? > I asked ChromeVox team whether we could override text pronounced by screen readers. even if you can, I'm unsettled by the growing number of default behaviors you have to override. Surely we will forget about one and something will be broken, besides the added complexity this creates.
On 2016/03/16 19:32:58, Evan Stade wrote: > ping dbeam --- is there an easy fix apparent to you? > > > I asked ChromeVox team whether we could override text pronounced by screen > readers. > > even if you can, I'm unsettled by the growing number of default behaviors you > have to override. Surely we will forget about one and something will be broken, > besides the added complexity this creates. I would rather see some semi-hokey javascript way of measuring and left-eliding text because that's better than all the trickery we have right now.
On 2016/03/16 19:32:58, Evan Stade wrote: > ping dbeam --- is there an easy fix apparent to you? can either of you send a screenshot? why are we reverse()ing to begin with? > > > I asked ChromeVox team whether we could override text pronounced by screen > readers. > > even if you can, I'm unsettled by the growing number of default behaviors you > have to override. Surely we will forget about one and something will be broken, > besides the added complexity this creates.
On 2016/03/16 19:39:46, Dan Beam wrote: > On 2016/03/16 19:32:58, Evan Stade wrote: > > ping dbeam --- is there an easy fix apparent to you? > > can either of you send a screenshot? why are we reverse()ing to begin with? > > > > > > I asked ChromeVox team whether we could override text pronounced by screen > > readers. > > > > even if you can, I'm unsettled by the growing number of default behaviors you > > have to override. Surely we will forget about one and something will be > broken, > > besides the added complexity this creates. you can see this in action with chrome://settings/passwords (may take some modifying of row element widths to get elision). We're reversing() as part of a series of hacks to allow for elide-from-left, i.e. www.google.com should become ...google.com when there's not enough space.
I tried javascript solutions. It is much more tricky then the current solution where we add new CSS classes. Let's continue discussing this issue in a separate bug (crbug.com/595662). Could we discuss changes related to the current CL, please? Thanks.
can you just change the UI? or use <canvas>, maybe?
On 2016/03/17 18:34:43, Dan Beam wrote: > can you just change the UI? or use <canvas>, maybe? Please, post your comment here crbug.com/595662. Thanks.
> On 2016/03/17 18:34:43, Dan Beam wrote: > > can you just change the UI? or use <canvas>, maybe? > Please, post your comment here crbug.com/595662. Thanks.@ @dbeam: friendly ping, please explain your idea about <canvas> here crbug.com/595662 It is better not to mix discussions of several issues. Do you mean to insert <canvas> instead of <div> and draw origin on it? @estade: friendly ping. please review this CL.
On 2016/03/17 21:00:53, kolos1 wrote: > > On 2016/03/17 18:34:43, Dan Beam wrote: > > > can you just change the UI? or use <canvas>, maybe? > > Please, post your comment here crbug.com/595662. Thanks.@ > @dbeam: friendly ping, please explain your idea about <canvas> here > crbug.com/595662 It is better not to mix discussions of several issues. Do you > mean to insert <canvas> instead of <div> and draw origin on it? > > @estade: friendly ping. please review this CL. I am not comfortable with this level of hack and will not approve this CL. You can feel free to find a different webui owner with a different stance to review it.
https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/520001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:86: linkText += ' )Android('; Ultimately, I think anything involving text concatenation should be left up to translators and this should use loadTimeData.getStringF(). As in: the " (Android)" should be in a translation, like: <message ...> <ph name="URL">$1<ex>android.com</ex></ph> (Android) </message> P.S. I have written a JS-based ellider and it does kinda suck. https://github.com/danbeam/ellipsis (this could be updated to do left-side elliding, but meh)
> I am not comfortable with this level of hack and will not approve this CL. You
> can feel free to find a different webui owner with a different stance to
review
> it.
estade@: I would prefer to follow your advice on reducing level of hack rather
than hop between reviewers. Left elided origin was a security requirement, that
we couldn't just roll back. Unfortunately, eliding is hacky and causes more
tricky changes. And as I understand Dan's comment and link, alternative
solutions will be probably tricky too.
Please clarify your opinion. Is it ok for you if we keep origins elided from the
left, but don't add more hacks (i.e adding ")Android(")?
On 2016/03/18 09:57:11, kolos1 wrote:
> > I am not comfortable with this level of hack and will not approve this CL.
You
> > can feel free to find a different webui owner with a different stance to
> review
> > it.
> estade@: I would prefer to follow your advice on reducing level of hack rather
> than hop between reviewers. Left elided origin was a security requirement,
that
> we couldn't just roll back. Unfortunately, eliding is hacky and causes more
> tricky changes. And as I understand Dan's comment and link, alternative
> solutions will be probably tricky too.
>
> Please clarify your opinion. Is it ok for you if we keep origins elided from
the
> left, but don't add more hacks (i.e adding ")Android(")?
here's another cool bug: select text, copy, paste.
So it doesn't seem that hard to me to figure out how much space you have to work
with.
favicon-cell's offsetWidth - getComputedStyle().paddingLeft -
getComputedStyle().marginLeft (can't find an easier way to do this).
Then you can put the string you want to display in the <a> and call offsetWidth
on it. If it's too big, lop off everything until the first '.' and try again.
Keep doing this till it fits, prepend '...', make sure it still fits, and stick
it in there. You only have to do this once as the space available never changes
(due to browser resize or whatever). Dan's code was probably a lot more
complicated because it had to handle dynamic resizing and other more elaborate
scenarios.
js is an option this is imperfect, but might work: https://jsfiddle.net/a3wuet1a/ i'm trying to think of more ways...
On 2016/03/18 17:54:40, Dan Beam wrote: > js is an option > > this is imperfect, but might work: > https://jsfiddle.net/a3wuet1a/ > > i'm trying to think of more ways... estade@: 1) Do you mean webkitPaddingStart and webkitMarginStart instead of paddingLeft and marginLeft? Otherwise, it will not work for RTL languages (https://bugs.chromium.org/p/chromium/issues/detail?id=593341#c2). 2) Do you want me to create a separate CL for new eliding and land it before the current CL? dbeam@: Thanks. I considered this solution, but didn't solve wrong overlapping. Please, let me know if you have more ideas. Have a good weekend! Regards, Maxim
On 2016/03/18 21:18:13, kolos1 wrote: > On 2016/03/18 17:54:40, Dan Beam wrote: > > js is an option > > > > this is imperfect, but might work: > > https://jsfiddle.net/a3wuet1a/ > > > > i'm trying to think of more ways... > > estade@: 1) Do you mean webkitPaddingStart and webkitMarginStart instead of > paddingLeft and marginLeft? Otherwise, it will not work for RTL languages > (https://bugs.chromium.org/p/chromium/issues/detail?id=593341#c2). sure. > 2) Do you want me to create a separate CL for new eliding and land it before the > current CL? that would be excellent > > dbeam@: Thanks. I considered this solution, but didn't solve wrong overlapping. > Please, let me know if you have more ideas. > > Have a good weekend! > > Regards, > Maxim
https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:97: androidSpan.textContent = ' (Android)'; I think this needs to be left up to translators. Android is probably the same everywhere but () aren't universal. Dan, can you put C++-provided translations into a CSS rule? This would be nice: .android-uri.clickable::after { text-content: ${androidUriSuffix}; } https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:306: set isAndroidUri(isAndroidUri) { is this used? https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:317: set isClickable(isClickable) { is this used? https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:451: get isAndroidUri() { is this used somewhere?
https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:97: androidSpan.textContent = ' (Android)'; On 2016/03/22 20:13:37, Evan Stade wrote: > I think this needs to be left up to translators. Android is probably the same > everywhere but () aren't universal. > > Dan, can you put C++-provided translations into a CSS rule? This would be nice: > > .android-uri.clickable::after { > text-content: ${androidUriSuffix}; > } content: "$i18n{...}"; might work in the near future but we'd need to escape any " found in the content of the replacement. we're hitting similar issues inside of value="$i18n{...}" in HTML
https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:97: androidSpan.textContent = ' (Android)'; On 2016/03/22 20:36:49, Dan Beam wrote: > On 2016/03/22 20:13:37, Evan Stade wrote: > > I think this needs to be left up to translators. Android is probably the same > > everywhere but () aren't universal. > > > > Dan, can you put C++-provided translations into a CSS rule? This would be > nice: > > > > .android-uri.clickable::after { > > text-content: ${androidUriSuffix}; > > } > > content: "$i18n{...}"; might work in the near future but we'd need to escape any > " found in the content of the replacement. we're hitting similar issues inside > of value="$i18n{...}" in HTML Where shall I define androidUriSuffix} that will be "(Android)"?
On 2016/03/22 21:00:52, kolos1 wrote: > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > File chrome/browser/resources/options/password_manager_list.js (right): > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > chrome/browser/resources/options/password_manager_list.js:97: > androidSpan.textContent = ' (Android)'; > On 2016/03/22 20:36:49, Dan Beam wrote: > > On 2016/03/22 20:13:37, Evan Stade wrote: > > > I think this needs to be left up to translators. Android is probably the > same > > > everywhere but () aren't universal. > > > > > > Dan, can you put C++-provided translations into a CSS rule? This would be > > nice: > > > > > > .android-uri.clickable::after { > > > text-content: ${androidUriSuffix}; > > > } > > > > content: "$i18n{...}"; might work in the near future but we'd need to escape > any > > " found in the content of the replacement. we're hitting similar issues > inside > > of value="$i18n{...}" in HTML > Where shall I define androidUriSuffix} that will be "(Android)"? PasswordManagerHandler::GetLocalizedValues
Maxim, one other thing you could do for unblocking most of this CL would be carving out the non-UI backend changes, and landing them separately. Happy to rubberstamp such a CL.
Patchset #17 (id:560001) has been deleted
Patchset #17 (id:580001) has been deleted
Introduced localization via loadTimeData.getString().
"content $i18n{...}" doesn't work. Also tried the HTML attribute i18n-content,
but it doesn't work for elements created in Javascript (only for templates like
chrome/browser/resources/options/password_manager.html)
Please review.
Regards,
Maxim
https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc...
File chrome/browser/resources/options/password_manager_list.js (right):
https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc...
chrome/browser/resources/options/password_manager_list.js:306: set
isAndroidUri(isAndroidUri) {
On 2016/03/22 20:13:37, Evan Stade wrote:
> is this used?
Yes, we need these properties.
isAndroidUri to check whether to add android suffix, i.e "(Android)".
isClickable to check whether <span> or <a> should be created to display origin.
https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:306: set isAndroidUri(isAndroidUri) { On 2016/03/23 11:59:46, kolos1 wrote: > On 2016/03/22 20:13:37, Evan Stade wrote: > > is this used? > Yes, we need these properties. > isAndroidUri to check whether to add android suffix, i.e "(Android)". > isClickable to check whether <span> or <a> should be created to display origin. > where?
On 2016/03/23 18:54:29, Evan Stade wrote: > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > File chrome/browser/resources/options/password_manager_list.js (right): > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > chrome/browser/resources/options/password_manager_list.js:306: set > isAndroidUri(isAndroidUri) { > On 2016/03/23 11:59:46, kolos1 wrote: > > On 2016/03/22 20:13:37, Evan Stade wrote: > > > is this used? > > Yes, we need these properties. > > isAndroidUri to check whether to add android suffix, i.e "(Android)". > > isClickable to check whether <span> or <a> should be created to display > origin. > > > > where? See lines 118, 75 and 54 in password_manager_list.js
On 2016/03/23 19:49:19, kolos1 wrote: > On 2016/03/23 18:54:29, Evan Stade wrote: > > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > > File chrome/browser/resources/options/password_manager_list.js (right): > > > > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > > chrome/browser/resources/options/password_manager_list.js:306: set > > isAndroidUri(isAndroidUri) { > > On 2016/03/23 11:59:46, kolos1 wrote: > > > On 2016/03/22 20:13:37, Evan Stade wrote: > > > > is this used? > > > Yes, we need these properties. > > > isAndroidUri to check whether to add android suffix, i.e "(Android)". > > > isClickable to check whether <span> or <a> should be created to display > > origin. > > > > > > > where? > > See lines 118, 75 and 54 in password_manager_list.js I just checked all those places and don't see where the setter is used.
On 2016/03/23 21:38:08, Evan Stade wrote: > On 2016/03/23 19:49:19, kolos1 wrote: > > On 2016/03/23 18:54:29, Evan Stade wrote: > > > > > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > > > File chrome/browser/resources/options/password_manager_list.js (right): > > > > > > > > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > > > chrome/browser/resources/options/password_manager_list.js:306: set > > > isAndroidUri(isAndroidUri) { > > > On 2016/03/23 11:59:46, kolos1 wrote: > > > > On 2016/03/22 20:13:37, Evan Stade wrote: > > > > > is this used? > > > > Yes, we need these properties. > > > > isAndroidUri to check whether to add android suffix, i.e "(Android)". > > > > isClickable to check whether <span> or <a> should be created to display > > > origin. > > > > > > > > > > where? > > > > See lines 118, 75 and 54 in password_manager_list.js > > I just checked all those places and don't see where the setter is used. That's interesting question, because we don't use any setters. Do you want me to remove them all?
On 2016/03/24 08:02:06, kolos1 wrote: > On 2016/03/23 21:38:08, Evan Stade wrote: > > On 2016/03/23 19:49:19, kolos1 wrote: > > > On 2016/03/23 18:54:29, Evan Stade wrote: > > > > > > > > > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > > > > File chrome/browser/resources/options/password_manager_list.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > > > > chrome/browser/resources/options/password_manager_list.js:306: set > > > > isAndroidUri(isAndroidUri) { > > > > On 2016/03/23 11:59:46, kolos1 wrote: > > > > > On 2016/03/22 20:13:37, Evan Stade wrote: > > > > > > is this used? > > > > > Yes, we need these properties. > > > > > isAndroidUri to check whether to add android suffix, i.e "(Android)". > > > > > isClickable to check whether <span> or <a> should be created to display > > > > origin. > > > > > > > > > > > > > where? > > > > > > See lines 118, 75 and 54 in password_manager_list.js > > > > I just checked all those places and don't see where the setter is used. > > That's interesting question, because we don't use any setters. Do you want me to > remove them all? The setters were added in the initial commit of password_manager_list.js four years ago (https://codereview.chromium.org/8895023/#). We set properties of password items in PasswordManagerHandler. I had never used the setters. I suppose there is no need to change origins, usernames, passwords and other stuff in UI part. Removed them.
On 2016/03/24 09:34:10, kolos1 wrote: > On 2016/03/24 08:02:06, kolos1 wrote: > > On 2016/03/23 21:38:08, Evan Stade wrote: > > > On 2016/03/23 19:49:19, kolos1 wrote: > > > > On 2016/03/23 18:54:29, Evan Stade wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > > > > > File chrome/browser/resources/options/password_manager_list.js (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1615653005/diff/540001/chrome/browser/resourc... > > > > > chrome/browser/resources/options/password_manager_list.js:306: set > > > > > isAndroidUri(isAndroidUri) { > > > > > On 2016/03/23 11:59:46, kolos1 wrote: > > > > > > On 2016/03/22 20:13:37, Evan Stade wrote: > > > > > > > is this used? > > > > > > Yes, we need these properties. > > > > > > isAndroidUri to check whether to add android suffix, i.e "(Android)". > > > > > > isClickable to check whether <span> or <a> should be created to > display > > > > > origin. > > > > > > > > > > > > > > > > where? > > > > > > > > See lines 118, 75 and 54 in password_manager_list.js > > > > > > I just checked all those places and don't see where the setter is used. > > > > That's interesting question, because we don't use any setters. Do you want me > to > > remove them all? > > The setters were added in the initial commit of password_manager_list.js four > years ago (https://codereview.chromium.org/8895023/). We set properties of > password items in PasswordManagerHandler. I had never used the setters. I > suppose there is no need to change origins, usernames, passwords and other stuff > in UI part. Removed them. that wasn't even really the initial commit, that was just a file move. The origin of the setters is even older than that. They may or may not have ever been used, but I think their presence harms understanding, so it's good to remove them (even though you personally didn't add them).
https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.css:76: -webkit-padding-start: 2px; perhaps .android-uri-suffix::before { text-content = " "; } https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:91: * Helper function that creates an HTML element that is appended to the not sure if helper is useful. Just inline it?
https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.css:76: -webkit-padding-start: 2px; On 2016/03/24 16:50:36, Evan Stade wrote: > perhaps > > .android-uri-suffix::before { > text-content = " "; > } i think estade means: .android-uri-suffix::before { content: ' '; } note: it's likely you'd need to set a `display:` as well (and might want to use '\u00A0', non breaking space)
https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.css:76: -webkit-padding-start: 2px; do we need ::before to add some space? in updatedOriginsEliding_, I would prefer to have just one element. Otherwise, we have to count width of both elements.
https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.css:76: -webkit-padding-start: 2px; Could I replace "2px" with "0.5em" or something like that?
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
On 2016/03/24 22:54:22, kolos1 wrote: > https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... > File chrome/browser/resources/options/password_manager_list.css (right): > > https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... > chrome/browser/resources/options/password_manager_list.css:76: > -webkit-padding-start: 2px; > Could I replace "2px" with "0.5em" or something like that? How about you put the space in the translated string? Some languages might not even want a space or might use a different separator character.
On 2016/03/28 21:53:39, Evan Stade wrote: > On 2016/03/24 22:54:22, kolos1 wrote: > > > https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... > > File chrome/browser/resources/options/password_manager_list.css (right): > > > > > https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... > > chrome/browser/resources/options/password_manager_list.css:76: > > -webkit-padding-start: 2px; > > Could I replace "2px" with "0.5em" or something like that? > > How about you put the space in the translated string? Some languages might not > even want a space or might use a different separator character. I suppose it is not possible to add space in this file chrome/app/generated_resources.grd. Moreover, the space should come on the left in LTR and on the right in RTL.
https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:91: * Helper function that creates an HTML element that is appended to the On 2016/03/24 16:50:36, Evan Stade wrote: > not sure if helper is useful. Just inline it? Done.
Patchset #20 (id:650001) has been deleted
On 2016/03/29 07:21:04, kolos1 wrote: > On 2016/03/28 21:53:39, Evan Stade wrote: > > On 2016/03/24 22:54:22, kolos1 wrote: > > > > > > https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... > > > File chrome/browser/resources/options/password_manager_list.css (right): > > > > > > > > > https://codereview.chromium.org/1615653005/diff/570020/chrome/browser/resourc... > > > chrome/browser/resources/options/password_manager_list.css:76: > > > -webkit-padding-start: 2px; > > > Could I replace "2px" with "0.5em" or something like that? > > > > How about you put the space in the translated string? Some languages might not > > even want a space or might use a different separator character. > > I suppose it is not possible to add space in this file > chrome/app/generated_resources.grd. Moreover, the space should come on the left > in LTR and on the right in RTL. I searched for the word "space" in generated_resources.grd and found how to do it. See IDS_EXTENSION_INSTALLED_SIGNIN_PROMO
Patchset #20 (id:670001) has been deleted
Oh, thanks! Added a space in generated_resources.grd, removed CSS class android-uri-suffix.
Patchset #20 (id:690001) has been deleted
lgtm https://codereview.chromium.org/1615653005/diff/710001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1615653005/diff/710001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:107: var androidUriSuffix = cr.doc.createElement('span'); nit: no need for this local var, you can just assign directly into item.androidUriSuffix
The CQ bit was checked by kolos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, engedy@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1615653005/#ps730001 (title: "Inlined the variable androidUriSuffix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615653005/730001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615653005/730001
Message was sent while issue was closed.
Description was changed from ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. The request doesn't issue an on-demand network request. If a request to cache fails, no web realm will be injected into corresponding form. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. BUG=593341 ========== to ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. The request doesn't issue an on-demand network request. If a request to cache fails, no web realm will be injected into corresponding form. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. BUG=593341 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:730001)
Message was sent while issue was closed.
Description was changed from ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. The request doesn't issue an on-demand network request. If a request to cache fails, no web realm will be injected into corresponding form. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. BUG=593341 ========== to ========== [Password manager] Human readable origins for Android credentials on chrome://settings/passwords PasswordStore::GetAutofillableLoginsWithAffiliatedRealms is introduced in this CL. It is same as GetAutofillableLogins, but also send a request to AffiliatedMatchHelper to fetch affiliated web realm and inject it into |form.affiliated_web_realm|. The request doesn't issue an on-demand network request. If a request to cache fails, no web realm will be injected into corresponding form. If there are a number of affiliated web realms, an arbitrary realm is injected. Origin for Android credentials will be shown like "example.com (Android)", where "example.com" is the fetched web realm. BUG=593341 Committed: https://crrev.com/983af5012da174a6a50c38ea956e2e1422646995 Cr-Commit-Position: refs/heads/master@{#384236} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/983af5012da174a6a50c38ea956e2e1422646995 Cr-Commit-Position: refs/heads/master@{#384236} |
