Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2261723002:
Copy PasswordForm in FormSaverImpl::UpdatePreferredLoginState (Closed)
|
Created:
4 years, 4 months ago by vabr (Chromium) Modified:
4 years, 4 months ago Reviewers:
dvadym CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCopy PasswordForm in FormSaverImpl::UpdatePreferredLoginState
UpdatePreferredLoginState needs to update PasswordStore with a slightly
modified version of some of the forms in PasswordFormManager's |best_matches_|.
Currently it does so by modifying |best_matches_| directly, and then updating
the PasswordStore.
That has the advantage of keeping an up-to-date copy in |best_matches_| without
reloading from PasswordStore (and also sparing PasswordForm copies inside
UpdatePreferredLoginState.
It also has a disadvantage: |best_matches_| cannot be const. That will become
a problem once the ownership of |best_matches_| will be transferred from
PasswordFormManager to a new class, FormFetcher, shared by multiple
PasswordFormManager instances. While it is possible to let all
PasswordFormManager instances modify the forms owned by a FormFetcher, that
will make the code hard to understand and prone to unexpected side-effects.
The advantage seems limited -- at the point UpdatePreferredLoginState is called,
the PasswordFormManager is saving a password form, and will be deleted soon.
It will not need the |best_matches_|. Once FormFetcher starts owning them,
it will need to wait for a refresh from PasswordStore to get them updated.
But that's not worse than today, when different PasswordFormManager instances do
not share their |best_matches_| at all.
BUG=621355
Committed: https://crrev.com/f6c4d75aab7ba327efc8e6e05d414b446fa63ecd
Cr-Commit-Position: refs/heads/master@{#413441}
Patch Set 1 #Patch Set 2 : Just create a copy of the best match to update. #Patch Set 3 : Just rebased #Patch Set 4 : Just rebased #Messages
Total messages: 29 (21 generated)
The CQ bit was checked by vabr@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...
vabr@chromium.org changed reviewers: + dvadym@chromium.org
Hi Vadym, Could you PTAL? Thanks! Vaclav
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 vabr@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...
Hi Vadym, I changed the CL after your off-line advice. Feel free to take another look while I update the CL description. Thanks! Vaclav
Description was changed from ========== Remove FormSaverImpl::UpdatePreferredLoginState The method was used to update PasswordFormManager's |best_matches_| at the point of saving a form to the PasswordStore. There seems to be no use of |best_matches_| at this point inside PasswordFormManager or FormSaverImpl, and saving is typically the last action performed by a PasswordFormManager (which owns the FormSaverImpl) before being destroyed. Therefore it seems like an unnecessary work to update the |best_matches_|. Furthermore, this was the only edit made to forms in |best_matches_|. Getting rid of it will make it possible to make forms in |best_matches_| constant. That will prepare PasswordFormManager for transferring its ownership to the future class of FormFetcher (hence the association to the BUG mentioned below). BUG=621355 ========== to ========== Copy PasswordForm in FormSaverImpl::UpdatePreferredLoginState UpdatePreferredLoginState needs to update PasswordStore with a slightly modified version of some of the forms in PasswordFormManager's |best_matches_|. Currently it does so by modifying |best_matches_| directly, and then updating the PasswordStore. That has the advantage of keeping an up-to-date copy in |best_matches_| without reloading from PasswordStore (and also sparing PasswordForm copies inside UpdatePreferredLoginState. It also has a disadvantage: |best_matches_| cannot be const. That will become a problem once the ownership of |best_matches_| will be transferred from PasswordFormManager to a new class, FormFetcher, shared by multiple PasswordFormManager instances. While it is possible to let all PasswordFormManager instances modify the forms owned by a FormFetcher, that will make the code hard to understand and prone to unexpected side-effects. The advantage seems limited -- at the point UpdatePreferredLoginState is called, the PasswordFormManager is saving a password form, and will be deleted soon. It will not need the |best_matches_|. Once FormFetcher starts owning them, it will need to wait for a refresh from PasswordStore to get them updated. But that's not worse than today, when different PasswordFormManager instances do not share their |best_matches_| at all. BUG=621355 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just rebased
The CQ bit was checked by vabr@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 vabr@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...
Hi Vadym, A gentle ping on this CL, as it is starting to hold a nice chain of dependent CLs. :) I know that we did not finish discussing this on Friday, so I'm happy to talk any time if you want to. Thanks! Vaclav
On 2016/08/22 14:17:07, vabr (Chromium) wrote: > Hi Vadym, > > A gentle ping on this CL, as it is starting to hold a nice chain of dependent > CLs. :) > > I know that we did not finish discussing this on Friday, so I'm happy to talk > any time if you want to. > > Thanks! > Vaclav LGTM
The CQ bit was unchecked by vabr@chromium.org
The CQ bit was checked by vabr@chromium.org
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 vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Copy PasswordForm in FormSaverImpl::UpdatePreferredLoginState UpdatePreferredLoginState needs to update PasswordStore with a slightly modified version of some of the forms in PasswordFormManager's |best_matches_|. Currently it does so by modifying |best_matches_| directly, and then updating the PasswordStore. That has the advantage of keeping an up-to-date copy in |best_matches_| without reloading from PasswordStore (and also sparing PasswordForm copies inside UpdatePreferredLoginState. It also has a disadvantage: |best_matches_| cannot be const. That will become a problem once the ownership of |best_matches_| will be transferred from PasswordFormManager to a new class, FormFetcher, shared by multiple PasswordFormManager instances. While it is possible to let all PasswordFormManager instances modify the forms owned by a FormFetcher, that will make the code hard to understand and prone to unexpected side-effects. The advantage seems limited -- at the point UpdatePreferredLoginState is called, the PasswordFormManager is saving a password form, and will be deleted soon. It will not need the |best_matches_|. Once FormFetcher starts owning them, it will need to wait for a refresh from PasswordStore to get them updated. But that's not worse than today, when different PasswordFormManager instances do not share their |best_matches_| at all. BUG=621355 ========== to ========== Copy PasswordForm in FormSaverImpl::UpdatePreferredLoginState UpdatePreferredLoginState needs to update PasswordStore with a slightly modified version of some of the forms in PasswordFormManager's |best_matches_|. Currently it does so by modifying |best_matches_| directly, and then updating the PasswordStore. That has the advantage of keeping an up-to-date copy in |best_matches_| without reloading from PasswordStore (and also sparing PasswordForm copies inside UpdatePreferredLoginState. It also has a disadvantage: |best_matches_| cannot be const. That will become a problem once the ownership of |best_matches_| will be transferred from PasswordFormManager to a new class, FormFetcher, shared by multiple PasswordFormManager instances. While it is possible to let all PasswordFormManager instances modify the forms owned by a FormFetcher, that will make the code hard to understand and prone to unexpected side-effects. The advantage seems limited -- at the point UpdatePreferredLoginState is called, the PasswordFormManager is saving a password form, and will be deleted soon. It will not need the |best_matches_|. Once FormFetcher starts owning them, it will need to wait for a refresh from PasswordStore to get them updated. But that's not worse than today, when different PasswordFormManager instances do not share their |best_matches_| at all. BUG=621355 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Copy PasswordForm in FormSaverImpl::UpdatePreferredLoginState UpdatePreferredLoginState needs to update PasswordStore with a slightly modified version of some of the forms in PasswordFormManager's |best_matches_|. Currently it does so by modifying |best_matches_| directly, and then updating the PasswordStore. That has the advantage of keeping an up-to-date copy in |best_matches_| without reloading from PasswordStore (and also sparing PasswordForm copies inside UpdatePreferredLoginState. It also has a disadvantage: |best_matches_| cannot be const. That will become a problem once the ownership of |best_matches_| will be transferred from PasswordFormManager to a new class, FormFetcher, shared by multiple PasswordFormManager instances. While it is possible to let all PasswordFormManager instances modify the forms owned by a FormFetcher, that will make the code hard to understand and prone to unexpected side-effects. The advantage seems limited -- at the point UpdatePreferredLoginState is called, the PasswordFormManager is saving a password form, and will be deleted soon. It will not need the |best_matches_|. Once FormFetcher starts owning them, it will need to wait for a refresh from PasswordStore to get them updated. But that's not worse than today, when different PasswordFormManager instances do not share their |best_matches_| at all. BUG=621355 ========== to ========== Copy PasswordForm in FormSaverImpl::UpdatePreferredLoginState UpdatePreferredLoginState needs to update PasswordStore with a slightly modified version of some of the forms in PasswordFormManager's |best_matches_|. Currently it does so by modifying |best_matches_| directly, and then updating the PasswordStore. That has the advantage of keeping an up-to-date copy in |best_matches_| without reloading from PasswordStore (and also sparing PasswordForm copies inside UpdatePreferredLoginState. It also has a disadvantage: |best_matches_| cannot be const. That will become a problem once the ownership of |best_matches_| will be transferred from PasswordFormManager to a new class, FormFetcher, shared by multiple PasswordFormManager instances. While it is possible to let all PasswordFormManager instances modify the forms owned by a FormFetcher, that will make the code hard to understand and prone to unexpected side-effects. The advantage seems limited -- at the point UpdatePreferredLoginState is called, the PasswordFormManager is saving a password form, and will be deleted soon. It will not need the |best_matches_|. Once FormFetcher starts owning them, it will need to wait for a refresh from PasswordStore to get them updated. But that's not worse than today, when different PasswordFormManager instances do not share their |best_matches_| at all. BUG=621355 Committed: https://crrev.com/f6c4d75aab7ba327efc8e6e05d414b446fa63ecd Cr-Commit-Position: refs/heads/master@{#413441} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f6c4d75aab7ba327efc8e6e05d414b446fa63ecd Cr-Commit-Position: refs/heads/master@{#413441} |
