Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2651663003:
Show human readable origin for Android apps (Closed)
DescriptionShow human readable origin for Android apps
Prior to this change Android credentials were displayed in a human
unfriendly way, e.g. "android://com.nytimes.android/". This change
addresses this issue by trying to obtain the affiliated name, while
still making clear it is an Android credential.
This requires a change to the extensions API. In order to have a user
friendly UI three URLs are transmitted from the backend:
- origin: This URL comes straight from the password store and contains
implementation specific logic. This URL is never surfaced to the user
and only serves to do logic such as editing or deleting passwords.
- shown: The string that is shown in the UI. It hides the scheme and
common host prefixes (e.g. "www") and indicates explicitly if a
credential corresponds to an Android app.
- link: The URL that is linked from the UI. This is mostly equivalent to
origin for Desktop credentials, but differs for Android credentials.
If possible, there is a link to an affiliated website, otherwise this
contains a link to the PlayStore.
BUG=679434
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2651663003
Cr-Commit-Position: refs/heads/master@{#466661}
Committed: https://chromium.googlesource.com/chromium/src/+/f120a871d23619bd90c973bc25b0007ab834c0c1
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressed comments. #
Total comments: 4
Patch Set 3 : New Round #
Total comments: 17
Patch Set 4 : Addressed comments #Patch Set 5 : CreateURLFromForm #
Total comments: 11
Patch Set 6 : Fix tests and nits #
Total comments: 2
Messages
Total messages: 52 (26 generated)
jdoerrie@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven, please take a look. This is just a proof of concept of how it could be done. It would be possible to do some of this logic in the JavaScript of the UI, but this would require changes to chrome/common/extensions/api/passwords_private.idl. What option do you prefer? I attached a screenshot in http://crbug.com/679434#c4
stevenjb@chromium.org changed reviewers: + hcarmona@chromium.org
This approach seems fine to me, but + hcarmona@ who is more familiar with this code. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:44: origin_url += l10n_util::GetStringUTF8(IDS_PASSWORDS_ANDROID_URI_SUFFIX); This should really be done using string substitution instead of appending (since that does not work well in all languages), i.e. <ph name="URL">$1<ex>foo.com</ex></ph> (Android) We should really fix everywhere that we use IDS_PASSWORDS_ANDROID_URI_SUFFIX, but that could be done in a separate CL. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:203: login_pair_to_index_map_[std::move(key)] = i; I'm not a huge fan of using move(string) like this. It is far too easy for someone to come along and use 'key' further down without realizing that it is now empty. The error potential seems to outweigh the minor optimization here. If you really want the minor optimization here, use a more specific name like login_pair_to_index_map_key. Alternately you could inline the function, but that would be pretty unreadable. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:206: entry.login_pair.origin_url = std::move(origin_url); This and the move below are even more confusing since the variables are declared much further up, I would just do a copy. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:249: exception_url_to_index_map_[std::move(key)] = i; This is super confusing since we are quietly copying origin_url above just so we can use move() here. Just make this [origin_url]. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:253: pair.link_url = std::move(link_url); Again, move here is more confusing than it is worth. Please also rename pair to something more specific like current_exceptions_pair while you are in here.
https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:37: std::string origin_url = password_manager::GetShownOriginAndLinkUrl( This eventually calls url_formatter::FormatUrlForSecurityDisplay using OMIT_HTTP_AND_HTTPS, but the old code used GetHumanReadableOrigin which calls url_formatter::FormatUrlForSecurityDisplay using SHOW (default). Is this an intended change? Can we add a test to ensure that it doesn't accidentally break in the future https://cs.chromium.org/chromium/src/components/password_manager/core/browser... vs https://cs.chromium.org/chromium/src/components/password_manager/core/browser... https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:42: if (!origin_is_clickable) Is the link_url always clickable?
Thanks for the reviews, do you mind taking another look? Tests will definitely follow before landing, but I would like to address the CSS / RTL issue first. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:37: std::string origin_url = password_manager::GetShownOriginAndLinkUrl( On 2017/01/23 20:24:21, hcarmona wrote: > This eventually calls url_formatter::FormatUrlForSecurityDisplay using > OMIT_HTTP_AND_HTTPS, but the old code used GetHumanReadableOrigin which > calls url_formatter::FormatUrlForSecurityDisplay using SHOW (default). > > Is this an intended change? Can we add a test to ensure that it doesn't > accidentally break in the future > > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... > > vs > > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... Thanks for catching this, I did not to intend to change this. However, |GetShownOriginAndLinkUrl| is already used by both the current settings UI, as well as on Android: - https://codesearch.chromium.org/chromium/src/chrome/browser/ui/webui/options/... - https://codesearch.chromium.org/chromium/src/chrome/browser/android/password_... In both of these cases the scheme seems to be omitted. Should this be changed? Was there a discussion around this for the new settings UI you could point me to? https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:42: if (!origin_is_clickable) On 2017/01/23 20:24:21, hcarmona wrote: > Is the link_url always clickable? The API does not make any claim whether |link_url| is clickable or not: https://codesearch.chromium.org/chromium/src/components/password_manager/core... By default it is set to |form.origin|. In case of Android credentials it is set to |form.affiliated_web_realm| if this is present, and |form.signon_realm| otherwise. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:44: origin_url += l10n_util::GetStringUTF8(IDS_PASSWORDS_ANDROID_URI_SUFFIX); On 2017/01/23 18:36:42, stevenjb wrote: > This should really be done using string substitution instead of appending (since > that does not work well in all languages), i.e. > > <ph name="URL">$1<ex>foo.com</ex></ph> (Android) > > We should really fix everywhere that we use IDS_PASSWORDS_ANDROID_URI_SUFFIX, > but that could be done in a separate CL. Agreed, the current situation is certainly not optimal. In order to elide long origins from the left I landed a CL that changes the direction of the text: http://crrev.com/2442333003 This works nicely for origins, but breaks with appending the " (Android)" string (screenshot: https://bugs.chromium.org/p/chromium/issues/attachment?aid=268070&inline=1). Note how the closing brace is at the beginning of the string. The same problem currently exists in the old settings UI, so it would be great to hear your opinion on how this could be fixed. A possible solution that came into my mind is to wrap the string in a bdo element: <bdo dir="ltr"> (Android)</bdo> https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:203: login_pair_to_index_map_[std::move(key)] = i; On 2017/01/23 18:36:42, stevenjb wrote: > I'm not a huge fan of using move(string) like this. It is far too easy for > someone to come along and use 'key' further down without realizing that it is > now empty. The error potential seems to outweigh the minor optimization here. > > If you really want the minor optimization here, use a more specific name like > login_pair_to_index_map_key. Alternately you could inline the function, but that > would be pretty unreadable. Done. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:206: entry.login_pair.origin_url = std::move(origin_url); On 2017/01/23 18:36:42, stevenjb wrote: > This and the move below are even more confusing since the variables are declared > much further up, I would just do a copy. Done. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:249: exception_url_to_index_map_[std::move(key)] = i; On 2017/01/23 18:36:42, stevenjb wrote: > This is super confusing since we are quietly copying origin_url above just so we > can use move() here. Just make this [origin_url]. Done. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:253: pair.link_url = std::move(link_url); On 2017/01/23 18:36:42, stevenjb wrote: > Again, move here is more confusing than it is worth. Please also rename pair to > something more specific like current_exceptions_pair while you are in here. Done. What is your opinion on renaming origin_url to exception_url and / or replacing this whole block by a single |current_exceptions_.emplace_back(origin_url, link_url);|?
Code changes lgtm, but please wait for hcarmona@ also. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:253: pair.link_url = std::move(link_url); On 2017/01/24 10:28:49, jdoerrie wrote: > On 2017/01/23 18:36:42, stevenjb wrote: > > Again, move here is more confusing than it is worth. Please also rename pair > to > > something more specific like current_exceptions_pair while you are in here. > > Done. What is your opinion on renaming origin_url to exception_url and / or > replacing this whole block by a single > |current_exceptions_.emplace_back(origin_url, link_url);|? That seems fine too.
https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:37: std::string origin_url = password_manager::GetShownOriginAndLinkUrl( On 2017/01/24 10:28:49, jdoerrie wrote: > On 2017/01/23 20:24:21, hcarmona wrote: > > This eventually calls url_formatter::FormatUrlForSecurityDisplay using > > OMIT_HTTP_AND_HTTPS, but the old code used GetHumanReadableOrigin which > > calls url_formatter::FormatUrlForSecurityDisplay using SHOW (default). > > > > Is this an intended change? Can we add a test to ensure that it doesn't > > accidentally break in the future > > > > > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... > > > > vs > > > > > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... > > Thanks for catching this, I did not to intend to change this. However, > |GetShownOriginAndLinkUrl| is already used by both the current settings UI, as > well as on Android: > - > https://codesearch.chromium.org/chromium/src/chrome/browser/ui/webui/options/... > > - > https://codesearch.chromium.org/chromium/src/chrome/browser/android/password_... > > In both of these cases the scheme seems to be omitted. Should this be changed? > Was there a discussion around this for the new settings UI you could point me > to? There hasn't been a discussion as far as I know, this is just something I noticed. There may be a security reason to differentiate between http and https. I'm OK landing this and creating a followup bug to address this everywhere since it's already in android and the options page. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:42: if (!origin_is_clickable) On 2017/01/24 10:28:49, jdoerrie wrote: > On 2017/01/23 20:24:21, hcarmona wrote: > > Is the link_url always clickable? > > The API does not make any claim whether |link_url| is clickable or not: > https://codesearch.chromium.org/chromium/src/components/password_manager/core... > > By default it is set to |form.origin|. In case of Android credentials it is set > to |form.affiliated_web_realm| if this is present, and |form.signon_realm| > otherwise. Sounds good. It looks like we should have a meaningful URL to link to. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:44: origin_url += l10n_util::GetStringUTF8(IDS_PASSWORDS_ANDROID_URI_SUFFIX); On 2017/01/24 10:28:49, jdoerrie wrote: > On 2017/01/23 18:36:42, stevenjb wrote: > > This should really be done using string substitution instead of appending > (since > > that does not work well in all languages), i.e. > > > > <ph name="URL">$1<ex>foo.com</ex></ph> (Android) > > > > We should really fix everywhere that we use IDS_PASSWORDS_ANDROID_URI_SUFFIX, > > but that could be done in a separate CL. > > Agreed, the current situation is certainly not optimal. In order to elide long > origins from the left I landed a CL that changes the direction of the text: > http://crrev.com/2442333003 > > This works nicely for origins, but breaks with appending the " (Android)" string > (screenshot: > https://bugs.chromium.org/p/chromium/issues/attachment?aid=268070&inline=1). > Note how the closing brace is at the beginning of the string. The same problem > currently exists in the old settings UI, so it would be great to hear your > opinion on how this could be fixed. A possible solution that came into my mind > is to wrap the string in a bdo element: <bdo dir="ltr"> (Android)</bdo> Maybe we should add the "Android" label in HTML, sort of like Autofill: http://screen/dQpS9auNZvR
https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:37: std::string origin_url = password_manager::GetShownOriginAndLinkUrl( On 2017/01/24 18:24:58, hcarmona wrote: > On 2017/01/24 10:28:49, jdoerrie wrote: > > On 2017/01/23 20:24:21, hcarmona wrote: > > > This eventually calls url_formatter::FormatUrlForSecurityDisplay using > > > OMIT_HTTP_AND_HTTPS, but the old code used GetHumanReadableOrigin which > > > calls url_formatter::FormatUrlForSecurityDisplay using SHOW (default). > > > > > > Is this an intended change? Can we add a test to ensure that it doesn't > > > accidentally break in the future > > > > > > > > > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... > > > > > > vs > > > > > > > > > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... > > > > Thanks for catching this, I did not to intend to change this. However, > > |GetShownOriginAndLinkUrl| is already used by both the current settings UI, as > > well as on Android: > > - > > > https://codesearch.chromium.org/chromium/src/chrome/browser/ui/webui/options/... > > > > - > > > https://codesearch.chromium.org/chromium/src/chrome/browser/android/password_... > > > > In both of these cases the scheme seems to be omitted. Should this be changed? > > Was there a discussion around this for the new settings UI you could point me > > to? > > There hasn't been a discussion as far as I know, this is just something > I noticed. There may be a security reason to differentiate between http > and https. I'm OK landing this and creating a followup bug to address > this everywhere since it's already in android and the options page. Acknowledged. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:42: if (!origin_is_clickable) On 2017/01/24 18:24:58, hcarmona wrote: > On 2017/01/24 10:28:49, jdoerrie wrote: > > On 2017/01/23 20:24:21, hcarmona wrote: > > > Is the link_url always clickable? > > > > The API does not make any claim whether |link_url| is clickable or not: > > > https://codesearch.chromium.org/chromium/src/components/password_manager/core... > > > > By default it is set to |form.origin|. In case of Android credentials it is > set > > to |form.affiliated_web_realm| if this is present, and |form.signon_realm| > > otherwise. > > Sounds good. It looks like we should have a meaningful URL to link to. Acknowledged. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:44: origin_url += l10n_util::GetStringUTF8(IDS_PASSWORDS_ANDROID_URI_SUFFIX); On 2017/01/24 18:24:58, hcarmona wrote: > On 2017/01/24 10:28:49, jdoerrie wrote: > > On 2017/01/23 18:36:42, stevenjb wrote: > > > This should really be done using string substitution instead of appending > > (since > > > that does not work well in all languages), i.e. > > > > > > <ph name="URL">$1<ex>foo.com</ex></ph> (Android) > > > > > > We should really fix everywhere that we use > IDS_PASSWORDS_ANDROID_URI_SUFFIX, > > > but that could be done in a separate CL. > > > > Agreed, the current situation is certainly not optimal. In order to elide long > > origins from the left I landed a CL that changes the direction of the text: > > http://crrev.com/2442333003 > > > > This works nicely for origins, but breaks with appending the " (Android)" > string > > (screenshot: > > https://bugs.chromium.org/p/chromium/issues/attachment?aid=268070&inline=1). > > Note how the closing brace is at the beginning of the string. The same problem > > currently exists in the old settings UI, so it would be great to hear your > > opinion on how this could be fixed. A possible solution that came into my mind > > is to wrap the string in a bdo element: <bdo dir="ltr"> (Android)</bdo> > > Maybe we should add the "Android" label in HTML, sort of like Autofill: > http://screen/dQpS9auNZvR That looks like a good idea. I reached out to a UX designer to get feedback here. https://codereview.chromium.org/2651663003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:253: pair.link_url = std::move(link_url); On 2017/01/24 18:08:14, stevenjb wrote: > On 2017/01/24 10:28:49, jdoerrie wrote: > > On 2017/01/23 18:36:42, stevenjb wrote: > > > Again, move here is more confusing than it is worth. Please also rename pair > > to > > > something more specific like current_exceptions_pair while you are in here. > > > > Done. What is your opinion on renaming origin_url to exception_url and / or > > replacing this whole block by a single > > |current_exceptions_.emplace_back(origin_url, link_url);|? > > That seems fine too. Turns out emplacing is actually not possible, only the default constructor and move constructor are defined. Leaving this as is.
tommycli@chromium.org changed reviewers: + tommycli@chromium.org
https://codereview.chromium.org/2651663003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:202: LoginPairToMapKey(origin_url, base::UTF16ToUTF8(form->username_value)); So our internal keys look like origin_url + "," + username. After this change, it's going to look like "netflix.com (Android),johndoe". This has got a bit of a code smell, especially since "Android" is going to be localized. I came across this while looking at stripping the schema from the origins. I suggest adding a third field to the LoginPair IDL, maybe called "displayOrigin", and updating the comment on "origin_url" to make clear that it's used as a key for all the other API calls. I also recommend just calling it "origin" instead of "origin_url". Finally, I think this change is going to also fix the 707296, which is a good thing. :) Good luck, I think this is going to be a bit tricky to get right, but it will be a good thing.
https://codereview.chromium.org/2651663003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:202: LoginPairToMapKey(origin_url, base::UTF16ToUTF8(form->username_value)); On 2017/04/20 23:41:00, tommycli wrote: > So our internal keys look like origin_url + "," + username. > > After this change, it's going to look like "netflix.com (Android),johndoe". This > has got a bit of a code smell, especially since "Android" is going to be > localized. > > I came across this while looking at stripping the schema from the origins. I > suggest adding a third field to the LoginPair IDL, maybe called "displayOrigin", > and updating the comment on "origin_url" to make clear that it's used as a key > for all the other API calls. > > I also recommend just calling it "origin" instead of "origin_url". > > Finally, I think this change is going to also fix the 707296, which is a good > thing. :) > > Good luck, I think this is going to be a bit tricky to get right, but it will be > a good thing. One more thing I mentioned to Hector, it's very very suspicious to me that all the API calls use "origin_url" as the key, yet "origin_url" is supposedly the GetHumanReadableOrigin. It feels like to me that the API calls should use the origin in canonical form rather than in human readable form... I don't expect you to change it in this CL, but just pointing out something that looked dangerous to me when I looked into changing this code...
https://codereview.chromium.org/2651663003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:202: LoginPairToMapKey(origin_url, base::UTF16ToUTF8(form->username_value)); On 2017/04/20 23:54:02, tommycli wrote: > On 2017/04/20 23:41:00, tommycli wrote: > > So our internal keys look like origin_url + "," + username. > > > > After this change, it's going to look like "netflix.com (Android),johndoe". > This > > has got a bit of a code smell, especially since "Android" is going to be > > localized. > > > > I came across this while looking at stripping the schema from the origins. I > > suggest adding a third field to the LoginPair IDL, maybe called > "displayOrigin", > > and updating the comment on "origin_url" to make clear that it's used as a key > > for all the other API calls. > > > > I also recommend just calling it "origin" instead of "origin_url". > > > > Finally, I think this change is going to also fix the 707296, which is a good > > thing. :) > > > > Good luck, I think this is going to be a bit tricky to get right, but it will > be > > a good thing. > > One more thing I mentioned to Hector, it's very very suspicious to me that all > the API calls use "origin_url" as the key, yet "origin_url" is supposedly the > GetHumanReadableOrigin. > > It feels like to me that the API calls should use the origin in canonical form > rather than in human readable form... > > I don't expect you to change it in this CL, but just pointing out something that > looked dangerous to me when I looked into changing this code... You are right, I don't think ' (Android)' should be made part of the origin_url, having a display origin sounds good to me as well. Also you are right regarding the use of an human readable URL as a key, this does seem like a bad idea. Another flaw I just discovered is that passwords are ignored for the key, meaning different credentials on the same site with same usernames but different passwords will be mapped to the same key. If I understand this code correctly, only one of the credentials would be shown in the settings page. Only after deletion of the first one the second one would be visible. The old settings page goes through some effort to create a very unique sort key including the password if possible (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/passwords/pass...), but I don't think this will work here, given the lack of a password field in the LoginPair IDL. This is very likely out of the scope for this CL, but it's another thing that could be dangerous in the future and should be addressed.
https://codereview.chromium.org/2651663003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:202: LoginPairToMapKey(origin_url, base::UTF16ToUTF8(form->username_value)); On 2017/04/21 06:54:06, jdoerrie wrote: > On 2017/04/20 23:54:02, tommycli wrote: > > On 2017/04/20 23:41:00, tommycli wrote: > > > So our internal keys look like origin_url + "," + username. > > > > > > After this change, it's going to look like "netflix.com (Android),johndoe". > > This > > > has got a bit of a code smell, especially since "Android" is going to be > > > localized. > > > > > > I came across this while looking at stripping the schema from the origins. I > > > suggest adding a third field to the LoginPair IDL, maybe called > > "displayOrigin", > > > and updating the comment on "origin_url" to make clear that it's used as a > key > > > for all the other API calls. > > > > > > I also recommend just calling it "origin" instead of "origin_url". > > > > > > Finally, I think this change is going to also fix the 707296, which is a > good > > > thing. :) > > > > > > Good luck, I think this is going to be a bit tricky to get right, but it > will > > be > > > a good thing. > > > > One more thing I mentioned to Hector, it's very very suspicious to me that all > > the API calls use "origin_url" as the key, yet "origin_url" is supposedly the > > GetHumanReadableOrigin. > > > > It feels like to me that the API calls should use the origin in canonical form > > rather than in human readable form... > > > > I don't expect you to change it in this CL, but just pointing out something > that > > looked dangerous to me when I looked into changing this code... > > You are right, I don't think ' (Android)' should be made part of the origin_url, > having a display origin sounds good to me as well. > > Also you are right regarding the use of an human readable URL as a key, this > does seem like a bad idea. > > Another flaw I just discovered is that passwords are ignored for the key, > meaning different credentials on the same site with same usernames but different > passwords will be mapped to the same key. If I understand this code correctly, > only one of the credentials would be shown in the settings page. Only after > deletion of the first one the second one would be visible. The old settings page > goes through some effort to create a very unique sort key including the password > if possible > (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/passwords/pass...), > but I don't think this will work here, given the lack of a password field in the > LoginPair IDL. This is very likely out of the scope for this CL, but it's > another thing that could be dangerous in the future and should be addressed. Cool... I just wanted to see if we were on the same page. Regarding passwords ignored for key, that sounds like it could be another issue. I agree that at some point we will need to address them, but yeah, out of scope for this CL.
Description was changed from ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. BUG=679434 ========== to ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by jdoerrie@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...
New round, probably missed a couple of spots, though. https://codereview.chromium.org/2651663003/diff/40001/third_party/closure_com... File third_party/closure_compiler/externs/passwords_private.js (right): https://codereview.chromium.org/2651663003/diff/40001/third_party/closure_com... third_party/closure_compiler/externs/passwords_private.js:46: chrome.passwordsPrivate.ExceptionEntry; Modifying this manually is definitely the wrong way to go, but I didn't manage to find out what script exactly I need to run to generate this. tools/json_schema_compiler/compiler.py did not do the trick for me. Any insights?
https://codereview.chromium.org/2651663003/diff/40001/third_party/closure_com... File third_party/closure_compiler/externs/passwords_private.js (right): https://codereview.chromium.org/2651663003/diff/40001/third_party/closure_com... third_party/closure_compiler/externs/passwords_private.js:46: chrome.passwordsPrivate.ExceptionEntry; On 2017/04/21 17:07:18, jdoerrie wrote: > Modifying this manually is definitely the wrong way to go, but I didn't manage > to find out what script exactly I need to run to generate this. > tools/json_schema_compiler/compiler.py did not do the trick for me. Any > insights? Check dis out: https://cs.chromium.org/chromium/src/chrome/common/extensions/api/passwords_p... Running a normal build should regenerate the auto-generated cpp files too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Overall this looks good https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:31: struct URLs { Can we drop the _url from these? struct URLs { std::string origin; std::string shown; std::string link; } https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:40: URLs GetOriginAndLinkUrlAndPrettifyAndroid(const autofill::PasswordForm& form) { Maybe rename to CreateURLFromForm? https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... File chrome/common/extensions/api/passwords_private.idl (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... chrome/common/extensions/api/passwords_private.idl:8: dictionary UrlCollection { Can we drop the Url from these data members? dictionary UrlCollection { DOMString origin; DOMString shown; DOMString link; } https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... chrome/common/extensions/api/passwords_private.idl:54: callback ExceptionListCallback = void(ExceptionEntry[] exceptions); Can we use the |UrlCollection| directly and remove the |ExceptionEntry|? https://codereview.chromium.org/2651663003/diff/40001/third_party/closure_com... File third_party/closure_compiler/externs/passwords_private.js (right): https://codereview.chromium.org/2651663003/diff/40001/third_party/closure_com... third_party/closure_compiler/externs/passwords_private.js:46: chrome.passwordsPrivate.ExceptionEntry; On 2017/04/21 17:18:49, tommycli wrote: > On 2017/04/21 17:07:18, jdoerrie wrote: > > Modifying this manually is definitely the wrong way to go, but I didn't manage > > to find out what script exactly I need to run to generate this. > > tools/json_schema_compiler/compiler.py did not do the trick for me. Any > > insights? > > Check dis out: > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/passwords_p... > > Running a normal build should regenerate the auto-generated cpp files too. Doesn't seem to regen on my machine automatically. IIRCC there's a script we need to run, but i don't remember where it is :-\ I'm looking for it and will update when I find it
https://codereview.chromium.org/2651663003/diff/40001/third_party/closure_com... File third_party/closure_compiler/externs/passwords_private.js (right): https://codereview.chromium.org/2651663003/diff/40001/third_party/closure_com... third_party/closure_compiler/externs/passwords_private.js:46: chrome.passwordsPrivate.ExceptionEntry; On 2017/04/21 17:52:19, hcarmona wrote: > On 2017/04/21 17:18:49, tommycli wrote: > > On 2017/04/21 17:07:18, jdoerrie wrote: > > > Modifying this manually is definitely the wrong way to go, but I didn't > manage > > > to find out what script exactly I need to run to generate this. > > > tools/json_schema_compiler/compiler.py did not do the trick for me. Any > > > insights? > > > > Check dis out: > > > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/passwords_p... > > > > Running a normal build should regenerate the auto-generated cpp files too. > > Doesn't seem to regen on my machine automatically. > IIRCC there's a script we need to run, but i don't remember where it is :-\ I'm > looking for it and will update when I find it go/ndgmw worked for me :-)
https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:43: value="[[item.loginPair.urls.originUrl]]" readonly always-float-label Unfortunately this will stay ugly for Android credentials, but the only alternative I saw was to use shownUrl here, which would hide the Scheme for regular credentials. I think showing the scheme and having ugly Android credentials here is the better option for now. WDYT? https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... File chrome/common/extensions/api/passwords_private.idl (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... chrome/common/extensions/api/passwords_private.idl:54: callback ExceptionListCallback = void(ExceptionEntry[] exceptions); On 2017/04/21 17:52:19, hcarmona wrote: > Can we use the |UrlCollection| directly and remove the |ExceptionEntry|? Yeah, I thought about doing this but wanted to somehow keep the Exception name. I tried typedeffing ExceptionEntry to UrlCollection, but wasn't able to figure it out how to do it in this IDL, it might be impossible. Using |UrlCollection| directly is also fine with me.
https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:43: value="[[item.loginPair.urls.originUrl]]" readonly always-float-label On 2017/04/21 18:48:48, jdoerrie wrote: > Unfortunately this will stay ugly for Android credentials, but the only > alternative I saw was to use shownUrl here, which would hide the Scheme for > regular credentials. I think showing the scheme and having ugly Android > credentials here is the better option for now. WDYT? I'm about 50/50 on this. |shownUrl| sounds good b/c it's readable. |originUrl| also sounds good because a user can copy it if they need to. I defer to your judgement here on what is best. https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... File chrome/common/extensions/api/passwords_private.idl (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... chrome/common/extensions/api/passwords_private.idl:54: callback ExceptionListCallback = void(ExceptionEntry[] exceptions); On 2017/04/21 18:48:49, jdoerrie wrote: > On 2017/04/21 17:52:19, hcarmona wrote: > > Can we use the |UrlCollection| directly and remove the |ExceptionEntry|? > > Yeah, I thought about doing this but wanted to somehow keep the Exception name. > I tried typedeffing ExceptionEntry to UrlCollection, but wasn't able to figure > it out how to do it in this IDL, it might be impossible. > > Using |UrlCollection| directly is also fine with me. Keeping ExceptionEntry for readability as is sounds good.
The CQ bit was checked by jdoerrie@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...
The CQ bit was checked by jdoerrie@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...
https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:31: struct URLs { On 2017/04/21 17:52:19, hcarmona wrote: > Can we drop the _url from these? > > struct URLs { > std::string origin; > std::string shown; > std::string link; > } Done. https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:40: URLs GetOriginAndLinkUrlAndPrettifyAndroid(const autofill::PasswordForm& form) { On 2017/04/21 17:52:19, hcarmona wrote: > Maybe rename to CreateURLFromForm? Done. https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:43: value="[[item.loginPair.urls.originUrl]]" readonly always-float-label On 2017/04/21 19:07:55, hcarmona wrote: > On 2017/04/21 18:48:48, jdoerrie wrote: > > Unfortunately this will stay ugly for Android credentials, but the only > > alternative I saw was to use shownUrl here, which would hide the Scheme for > > regular credentials. I think showing the scheme and having ugly Android > > credentials here is the better option for now. WDYT? > > I'm about 50/50 on this. |shownUrl| sounds good b/c it's readable. > |originUrl| also sounds good because a user can copy it if they need to. > > I defer to your judgement here on what is best. I went for |link| in the end, because it gives us the best of both worlds. It hides implementation details from |origin|, but still shows full URLs. I added now also the link to the play store for apps where we don't have information about the affiliated website. https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... File chrome/common/extensions/api/passwords_private.idl (right): https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... chrome/common/extensions/api/passwords_private.idl:8: dictionary UrlCollection { On 2017/04/21 17:52:19, hcarmona wrote: > Can we drop the Url from these data members? > > dictionary UrlCollection { > DOMString origin; > DOMString shown; > DOMString link; > } Done. https://codereview.chromium.org/2651663003/diff/40001/chrome/common/extension... chrome/common/extensions/api/passwords_private.idl:54: callback ExceptionListCallback = void(ExceptionEntry[] exceptions); On 2017/04/21 19:07:55, hcarmona wrote: > On 2017/04/21 18:48:49, jdoerrie wrote: > > On 2017/04/21 17:52:19, hcarmona wrote: > > > Can we use the |UrlCollection| directly and remove the |ExceptionEntry|? > > > > Yeah, I thought about doing this but wanted to somehow keep the Exception > name. > > I tried typedeffing ExceptionEntry to UrlCollection, but wasn't able to figure > > it out how to do it in this IDL, it might be impossible. > > > > Using |UrlCollection| directly is also fine with me. > > Keeping ExceptionEntry for readability as is sounds good. Acknowledged.
jdoerrie@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@: Please review chrome/common/extensions/api/passwords_private.idl
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Can you update the CL description to mention changing the extensions API? From the description alone, it's very ambiguous.
LGTM w/ some nits https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:111: title="[[item.loginPair.urls.link]]"> nit: indent 2 more spaces https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:277: /** @type {function(!Array<PasswordManager.ExceptionEntry>):void} */ ( nit: indent is off here and no space before the ( https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:324: password.loginPair.username.includes(filter); nit: spacing https://codereview.chromium.org/2651663003/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2651663003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:63: assertEquals( nit: keep on same line as assert for consistency w/ other lines here. https://codereview.chromium.org/2651663003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:89: assertEquals( nit: ditto https://codereview.chromium.org/2651663003/diff/80001/third_party/closure_com... File third_party/closure_compiler/externs/passwords_private.js (right): https://codereview.chromium.org/2651663003/diff/80001/third_party/closure_com... third_party/closure_compiler/externs/passwords_private.js:1: // Copyright 2017 The Chromium Authors. All rights reserved. We don't usually update the year, but this file is auto generated... If no one feels strongly about this, I'm fine updating to 2017 so we don't fight the script.
Description was changed from ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. This requires a change to the extensions API. In order to have a user friendly UI three URLs are transmitted from the backend: - origin: This URL comes straight from the password store and contains implementation specific logic. This URL is never surfaced to the user and only serves to do logic such as editing or deleting passwords. - shown: The string that is shown in the UI. It hides the scheme and common host prefixes (e.g. "www") and indicates explicitly if a credential corresponds to an Android app. - link: The URL that is linked from the UI. This is mostly equivalent to origin for Desktop credentials, but differs for Android credentials. If possible, there is a link to an affiliated website, otherwise this contains a link to the PlayStore. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Next round. I added new screenshots to the bug: http://crbug.com/679434#c13. rdevlin.cronin@: I updated the description and explained why the API change is necessary, could you have another look? https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:111: title="[[item.loginPair.urls.link]]"> On 2017/04/21 22:45:46, hcarmona wrote: > nit: indent 2 more spaces Done, thanks for catching all of these. I ran 'git cl format --js', but apparently this does not produce the desired output. https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:277: /** @type {function(!Array<PasswordManager.ExceptionEntry>):void} */ ( On 2017/04/21 22:45:46, hcarmona wrote: > nit: indent is off here and no space before the ( Done. https://codereview.chromium.org/2651663003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:324: password.loginPair.username.includes(filter); On 2017/04/21 22:45:46, hcarmona wrote: > nit: spacing Done. https://codereview.chromium.org/2651663003/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2651663003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:63: assertEquals( On 2017/04/21 22:45:46, hcarmona wrote: > nit: keep on same line as assert for consistency w/ other lines here. Done. https://codereview.chromium.org/2651663003/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:89: assertEquals( On 2017/04/21 22:45:46, hcarmona wrote: > nit: ditto Done. https://codereview.chromium.org/2651663003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2651663003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:323: return password.loginPair.urls.shown.includes(filter) || I changed this because we really should match against the strings we show to the user instead of the hidden implementation detail.
The CQ bit was checked by jdoerrie@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...
Description was changed from ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. This requires a change to the extensions API. In order to have a user friendly UI three URLs are transmitted from the backend: - origin: This URL comes straight from the password store and contains implementation specific logic. This URL is never surfaced to the user and only serves to do logic such as editing or deleting passwords. - shown: The string that is shown in the UI. It hides the scheme and common host prefixes (e.g. "www") and indicates explicitly if a credential corresponds to an Android app. - link: The URL that is linked from the UI. This is mostly equivalent to origin for Desktop credentials, but differs for Android credentials. If possible, there is a link to an affiliated website, otherwise this contains a link to the PlayStore. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. This requires a change to the extensions API. In order to have a user friendly UI three URLs are transmitted from the backend: - origin: This URL comes straight from the password store and contains implementation specific logic. This URL is never surfaced to the user and only serves to do logic such as editing or deleting passwords. - shown: The string that is shown in the UI. It hides the scheme and common host prefixes (e.g. "www") and indicates explicitly if a credential corresponds to an Android app. - link: The URL that is linked from the UI. This is mostly equivalent to origin for Desktop credentials, but differs for Android credentials. If possible, there is a link to an affiliated website, otherwise this contains a link to the PlayStore. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. This requires a change to the extensions API. In order to have a user friendly UI three URLs are transmitted from the backend: - origin: This URL comes straight from the password store and contains implementation specific logic. This URL is never surfaced to the user and only serves to do logic such as editing or deleting passwords. - shown: The string that is shown in the UI. It hides the scheme and common host prefixes (e.g. "www") and indicates explicitly if a credential corresponds to an Android app. - link: The URL that is linked from the UI. This is mostly equivalent to origin for Desktop credentials, but differs for Android credentials. If possible, there is a link to an affiliated website, otherwise this contains a link to the PlayStore. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. This requires a change to the extensions API. In order to have a user friendly UI three URLs are transmitted from the backend: - origin: This URL comes straight from the password store and contains implementation specific logic. This URL is never surfaced to the user and only serves to do logic such as editing or deleting passwords. - shown: The string that is shown in the UI. It hides the scheme and common host prefixes (e.g. "www") and indicates explicitly if a credential corresponds to an Android app. - link: The URL that is linked from the UI. This is mostly equivalent to origin for Desktop credentials, but differs for Android credentials. If possible, there is a link to an affiliated website, otherwise this contains a link to the PlayStore. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
chrome/common/extensions/api/passwords_private.idl lgtm
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2651663003/#ps100001 (title: "Fix tests and nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2651663003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2651663003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:323: return password.loginPair.urls.shown.includes(filter) || On 2017/04/24 08:35:59, jdoerrie wrote: > I changed this because we really should match against the strings we show to the > user instead of the hidden implementation detail. 👍
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493049274356330,
"parent_rev": "cda13a9e3a043b37ecc7f28a60333091a80bdf3e", "commit_rev":
"f120a871d23619bd90c973bc25b0007ab834c0c1"}
Message was sent while issue was closed.
Description was changed from ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. This requires a change to the extensions API. In order to have a user friendly UI three URLs are transmitted from the backend: - origin: This URL comes straight from the password store and contains implementation specific logic. This URL is never surfaced to the user and only serves to do logic such as editing or deleting passwords. - shown: The string that is shown in the UI. It hides the scheme and common host prefixes (e.g. "www") and indicates explicitly if a credential corresponds to an Android app. - link: The URL that is linked from the UI. This is mostly equivalent to origin for Desktop credentials, but differs for Android credentials. If possible, there is a link to an affiliated website, otherwise this contains a link to the PlayStore. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Show human readable origin for Android apps Prior to this change Android credentials were displayed in a human unfriendly way, e.g. "android://com.nytimes.android/". This change addresses this issue by trying to obtain the affiliated name, while still making clear it is an Android credential. This requires a change to the extensions API. In order to have a user friendly UI three URLs are transmitted from the backend: - origin: This URL comes straight from the password store and contains implementation specific logic. This URL is never surfaced to the user and only serves to do logic such as editing or deleting passwords. - shown: The string that is shown in the UI. It hides the scheme and common host prefixes (e.g. "www") and indicates explicitly if a credential corresponds to an Android app. - link: The URL that is linked from the UI. This is mostly equivalent to origin for Desktop credentials, but differs for Android credentials. If possible, there is a link to an affiliated website, otherwise this contains a link to the PlayStore. BUG=679434 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2651663003 Cr-Commit-Position: refs/heads/master@{#466661} Committed: https://chromium.googlesource.com/chromium/src/+/f120a871d23619bd90c973bc25b0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/f120a871d23619bd90c973bc25b0... |
