Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844963003:
Introduce extensions::CreateUrlCollectionFromForm (Closed)
|
Created:
3 years, 8 months ago by jdoerrie Modified:
3 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, extensions-reviews_chromium.org, hcarmona Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce extensions::CreateUrlCollectionFromForm
During the development of r466661 |GetHumanReadableOrigin| was replaced
by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to
handle Android credentials in an appropriate manner. However, there was
an undetected reliance on using |GetHumanReadableOrigin| in order to
display user passwords. Since the result of |CreateURLFromForm| is
slightly different, the frontend was unable to match origins when
displaying passwords.
This change addresses this issue by replacing other calls to
|GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring
consistency between results.
TBR=rdevlin.cronin@chromium.org
BUG=717454
Review-Url: https://codereview.chromium.org/2844963003
Cr-Commit-Position: refs/heads/master@{#472702}
Committed: https://chromium.googlesource.com/chromium/src/+/2d3f81e4a7d9839d5183dbabf17655fbbf0e5cb7
Patch Set 1 #Patch Set 2 : Add Unittest File #
Total comments: 2
Patch Set 3 : Fix typo and don't include header on Android #Patch Set 4 : Boolean logic is hard :/ #
Total comments: 5
Patch Set 5 : Revert One-Liner #Messages
Total messages: 54 (33 generated)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jdoerrie@chromium.org changed reviewers: + stevenjb@chromium.org, vabr@chromium.org
vabr@chromium.org: Please review changes in chrome/browser/ui/passwords/ stevenjb@chromium.org: Please review the CL as a whole.
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...
LGTM. Do the old (non-MD) settings work fine after this patch? Cheers, Vaclav https://codereview.chromium.org/2844963003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h (right): https://codereview.chromium.org/2844963003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h:55: // |origin_url| The origin for the URL where the password is used/ should be typo: / -> ;
On 2017/04/27 11:49:37, vabr (Chromium) wrote: > LGTM. > Do the old (non-MD) settings work fine after this patch? Thanks, Vaclav! Yes, the old settings page ignores the passed in origin_url, thus this change has no effect there and it continues working fine.
On 2017/04/27 11:59:28, jdoerrie wrote: > On 2017/04/27 11:49:37, vabr (Chromium) wrote: > > LGTM. > > Do the old (non-MD) settings work fine after this patch? > > Thanks, Vaclav! Yes, the old settings page ignores the passed in origin_url, > thus this change has no effect there and it continues working fine. Relevant piece of code: https://codesearch.chromium.org/chromium/src/chrome/browser/ui/webui/options/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Vaclav, do you mind having another look at chrome/browser/ui/passwords/password_manager_presenter.cc? https://codereview.chromium.org/2844963003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h (right): https://codereview.chromium.org/2844963003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h:55: // |origin_url| The origin for the URL where the password is used/ should be On 2017/04/27 11:49:37, vabr (Chromium) wrote: > typo: / -> ; Done.
On 2017/04/27 14:49:32, jdoerrie wrote: > Hi Vaclav, do you mind having another look at > chrome/browser/ui/passwords/password_manager_presenter.cc? Still LGTM!
Thanks, vabr@! A friendly ping to stevenjb@ to review the rest.
Description was changed from ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. BUG=715866 ========== to ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. BUG=715866 ==========
On 2017/04/28 08:22:21, jdoerrie wrote: > Thanks, vabr@! > A friendly ping to stevenjb@ to review the rest. Thanks Jan for fixing this. The password page doesn't show passwords at this point :) That's awful.
stevenjb@chromium.org changed reviewers: + hcarmona@chromium.org
+hcarmona@ who may understand the url details better than I do, but this looks fine as far as I can tell. owner lgtm. https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_utils.cc (right): https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_utils.cc:37: // If the origin is not clickable, link to the PlayStore. Use that s/Use that/Ensure that/ ?
This LGTM! Thanks for fixing this so quickly :-)
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org
At our stand up we talked about clicking commit on this CL so it lands before the weekend. dbeam@ wanted to take a quick look before we commit this :-)
https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:126: base::Unretained(this), origin_url, username, web_contents)); why can't we just do GURL(origin_url).GetOrigin().spec()? doesn't that accomplish the normalization we need for a mergeable CL?
I provided more information why the change in its current form is necessary, I hope this clears things up. Please go ahead and click the commit button yourself in case you agree with this. It is late here, so I won't be able to respond today to further questions anymore, but I also would like to get this in before the weekend :) https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:126: base::Unretained(this), origin_url, username, web_contents)); On 2017/04/28 19:41:23, Dan Beam wrote: > why can't we just do > > GURL(origin_url).GetOrigin().spec()? > > doesn't that accomplish the normalization we need for a mergeable CL? No, that wouldn't work. The origin_url parameter here is simply used to create a lookup key into the login to index map (see line 133 and 149 below). It is not used for the equality check that is currently failing in JavaScript: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/passwo... It is important that origin_url here matches the origin that was used to create the index in line 183, thus we can't change it here. What causes the current mismatch is that |CreateURLsFromForm| is used when building the initial list of passwords (line 228 prior to this change), but GetHumanReadableOrigin is used to create the corresponding origin during the RequestShowPassword logic (see chrome/browser/ui/passwords/password_manager_presenter.cc:244 prior to this change). Prior to r466661 GetHumanReadableOrigin was used in both places, so things were working fine. r466661 then introduced an inconsistency by using CreateURLsFromForm in one place and GetHumanReadableOrigin in another. This change now remedies the situation by using CreateURLsFromForm in both places, restoring consistency. https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_utils.cc (right): https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_utils.cc:37: // If the origin is not clickable, link to the PlayStore. Use that On 2017/04/28 15:54:25, stevenjb wrote: > s/Use that/Ensure that/ ? No, we don't actually check that urls.shown contains no hash anymore, we rely on the fact that this is done as part of the call to password_manager::GetShownOriginAndLinkUrl for Android credentials: https://codesearch.chromium.org/chromium/src/components/password_manager/core...
https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc (right): https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:126: base::Unretained(this), origin_url, username, web_contents)); On 2017/04/28 21:12:52, jdoerrie wrote: > On 2017/04/28 19:41:23, Dan Beam wrote: > > why can't we just do > > > > GURL(origin_url).GetOrigin().spec()? > > > > doesn't that accomplish the normalization we need for a mergeable CL? > > No, that wouldn't work. The origin_url parameter here is simply used to create a > lookup key into the login to index map (see line 133 and 149 below). It is not > used for the equality check that is currently failing in JavaScript: > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/passwo... > It is important that origin_url here matches the origin that was used to create > the index in line 183, thus we can't change it here. > > What causes the current mismatch is that |CreateURLsFromForm| is used when > building the initial list of passwords (line 228 prior to this change), but > GetHumanReadableOrigin is used to create the corresponding origin during the > RequestShowPassword logic (see > chrome/browser/ui/passwords/password_manager_presenter.cc:244 prior to this > change). > > Prior to r466661 GetHumanReadableOrigin was used in both places, so things were > working fine. r466661 then introduced an inconsistency by using > CreateURLsFromForm in one place and GetHumanReadableOrigin in another. This > change now remedies the situation by using CreateURLsFromForm in both places, > restoring consistency. i agree that this solution is preferred in the long run to reduce the number of ways to handle origins. sorry that I didn't make that clearer. in the short term, merging the last CL broke something important on a release candidate. that's because it changed a lot, and we didn't have the right tests. i might break something with this, but it's much more surgical and also fixes the specific issue at hand: https://codereview.chromium.org/2848983002/
Description was changed from ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. BUG=715866 ========== to ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. BUG= ==========
On 2017/04/28 23:03:27, Dan Beam wrote: > https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... > File > chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc > (right): > > https://codereview.chromium.org/2844963003/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc:126: > base::Unretained(this), origin_url, username, web_contents)); > On 2017/04/28 21:12:52, jdoerrie wrote: > > On 2017/04/28 19:41:23, Dan Beam wrote: > > > why can't we just do > > > > > > GURL(origin_url).GetOrigin().spec()? > > > > > > doesn't that accomplish the normalization we need for a mergeable CL? > > > > No, that wouldn't work. The origin_url parameter here is simply used to create > a > > lookup key into the login to index map (see line 133 and 149 below). It is > not > > used for the equality check that is currently failing in JavaScript: > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/passwo... > > It is important that origin_url here matches the origin that was used to > create > > the index in line 183, thus we can't change it here. > > > > What causes the current mismatch is that |CreateURLsFromForm| is used when > > building the initial list of passwords (line 228 prior to this change), but > > GetHumanReadableOrigin is used to create the corresponding origin during the > > RequestShowPassword logic (see > > chrome/browser/ui/passwords/password_manager_presenter.cc:244 prior to this > > change). > > > > Prior to r466661 GetHumanReadableOrigin was used in both places, so things > were > > working fine. r466661 then introduced an inconsistency by using > > CreateURLsFromForm in one place and GetHumanReadableOrigin in another. This > > change now remedies the situation by using CreateURLsFromForm in both places, > > restoring consistency. > > i agree that this solution is preferred in the long run to reduce the number of > ways to handle origins. sorry that I didn't make that clearer. > > in the short term, merging the last CL broke something important on a release > candidate. that's because it changed a lot, and we didn't have the right tests. > > i might break something with this, but it's much more surgical and also fixes > the specific issue at hand: > https://codereview.chromium.org/2848983002/ so i landed the above 2-liner ^ to simply canonicalize the origin when sent to JS. let's use a different BUG= to not confuse TPMs as I'm going to request a merge for the 2-liner on that bug.
this was the one-liner I landed on the M59 branch: https://codereview.chromium.org/2848983002/ please feel free to remove the code if it's not necessary any more with this patch. as a general FYI: TPMs discourage large changes when merging, engineers encourage holistic changes for code health when NOT merging. I'm very happy with this change landing on HEAD to make the code better (yay!) but it's too bad a large CL that we merged earlier caused this (unlucky).
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
On 2017/05/01 19:09:13, Dan Beam wrote: > this was the one-liner I landed on the M59 branch: > https://codereview.chromium.org/2848983002/ > > please feel free to remove the code if it's not necessary any more with this > patch. > > as a general FYI: TPMs discourage large changes when merging, engineers > encourage holistic changes for code health when NOT merging. > > I'm very happy with this change landing on HEAD to make the code better (yay!) > but it's too bad a large CL that we merged earlier caused this (unlucky). Thank you for landing and merging the simple fix! This addresses the issue for most cases, but unfortunately fails for Android credentials. I filed http://crbug.com/717454 for this, and will use it as the tracking bug for this CL. I don't think not showing passwords for Android credentials is critical enough in order to merge this CL into M59 as well, so I will aim for M60. I'll also try to improve the test coverage in order to catch these bugs earlier. However, Please feel free to update the priority of the bug in case you disagree with me.
Description was changed from ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. BUG= ========== to ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. BUG=717454 ==========
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 ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. BUG=717454 ========== to ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. TBR=rdevlin.cronin@chromium.org BUG=717454 ==========
jdoerrie@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@chromium.org: TBR for chrome/browser/extensions/BUILD.gn
On 2017/05/17 14:53:59, jdoerrie wrote: > mailto:rdevlin.cronin@chromium.org: TBR for chrome/browser/extensions/BUILD.gn chrome/browser/extensions/BUILD.gn lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, stevenjb@chromium.org, hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2844963003/#ps80001 (title: "Revert One-Liner")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495087850482610,
"parent_rev": "6e1e25689547ab7a811c9989857c5adadd04a5d9", "commit_rev":
"2d3f81e4a7d9839d5183dbabf17655fbbf0e5cb7"}
Message was sent while issue was closed.
Description was changed from ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. TBR=rdevlin.cronin@chromium.org BUG=717454 ========== to ========== Introduce extensions::CreateUrlCollectionFromForm During the development of r466661 |GetHumanReadableOrigin| was replaced by |CreateURLFromForm| in passwords_private_delegate_impl.cc to be to handle Android credentials in an appropriate manner. However, there was an undetected reliance on using |GetHumanReadableOrigin| in order to display user passwords. Since the result of |CreateURLFromForm| is slightly different, the frontend was unable to match origins when displaying passwords. This change addresses this issue by replacing other calls to |GetHumanReadableOrigin| with |CreateUrlCollectionFromForm| restoring consistency between results. TBR=rdevlin.cronin@chromium.org BUG=717454 Review-Url: https://codereview.chromium.org/2844963003 Cr-Commit-Position: refs/heads/master@{#472702} Committed: https://chromium.googlesource.com/chromium/src/+/2d3f81e4a7d9839d5183dbabf176... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2d3f81e4a7d9839d5183dbabf176... |
