Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

PasswordFormManager ensures that outdated sync credentials are deleted, to help eliminate the situation when the sync credential is stored in the password store it protects.

This CL moves the logic determining which forms to drop and the Remove call to PasswordStore from PasswordFormManager to FormSaver, the delegate supposed to be handling store operations for PFM. This leaves only reading calls to PasswordStore in PasswordFormManager. The CL also adds and removes tests and modifies test support as appropriate.

This addresses a TODO from https://codereview.chromium.org/2090583003/. It concludes the first transition step from the design https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtPAo/edit?usp=sharing .

BUG=621355
Committed: https://crrev.com/8afac70d8152dafd3a4e539a095c12324e34d63d
Cr-Commit-Position: refs/heads/master@{#402745}

[vabr (Chromium), 2016-06-27 14:34:45]

Description was changed from

==========
Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

This is still WIP.
COMMIT=false

This addresses a TODO from https://codereview.chromium.org/2090583003/. This CL
moves the logic determining which forms to drop as well as the Remove call to
PasswordStore from PasswordFormManager. This leaves only reading calls to
PasswordStore in PasswordFormManager. The CL also adds and removes tests and
modifies test support as appropriate.

BUG=621355
==========

to

==========
Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

PasswordFormManager ensures that outdated sync credentials are deleted, to help
eliminate the situation when the sync credential is stored in the password store
it protects.

This CL moves the logic determining which forms to drop and the Remove call to
PasswordStore from PasswordFormManager to FormSaver, the delegate supposed to be
handling store operations for PFM. This leaves only reading calls to
PasswordStore in PasswordFormManager. The CL also adds and removes tests and
modifies test support as appropriate.

This addresses a TODO from https://codereview.chromium.org/2090583003/.

BUG=621355
==========

[vabr (Chromium), 2016-06-28 06:08:35]

vabr@chromium.org changed reviewers:
+ vasilii@chromium.org

[vabr (Chromium), 2016-06-28 06:08:36]

Hi Vasilii,

Could you please take a look?

Thanks,
Vaclav

[vabr (Chromium), 2016-06-28 06:10:40]

Description was changed from

==========
Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

PasswordFormManager ensures that outdated sync credentials are deleted, to help
eliminate the situation when the sync credential is stored in the password store
it protects.

This CL moves the logic determining which forms to drop and the Remove call to
PasswordStore from PasswordFormManager to FormSaver, the delegate supposed to be
handling store operations for PFM. This leaves only reading calls to
PasswordStore in PasswordFormManager. The CL also adds and removes tests and
modifies test support as appropriate.

This addresses a TODO from https://codereview.chromium.org/2090583003/.

BUG=621355
==========

to

==========
Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

PasswordFormManager ensures that outdated sync credentials are deleted, to help
eliminate the situation when the sync credential is stored in the password store
it protects.

This CL moves the logic determining which forms to drop and the Remove call to
PasswordStore from PasswordFormManager to FormSaver, the delegate supposed to be
handling store operations for PFM. This leaves only reading calls to
PasswordStore in PasswordFormManager. The CL also adds and removes tests and
modifies test support as appropriate.

This addresses a TODO from https://codereview.chromium.org/2090583003/. It
concludes the first transition step from the design
https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtP...
.

BUG=621355
==========

[vasilii, 2016-06-28 09:42:29]

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver.h (right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver.h:61: // |*preferred_match|
is nulled. It makes no sense to call this if |pending|
You should mention that the implementation enforces that |pending| is GAIA.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl.cc (right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl.cc:108: // Move the
|best_matches| to a vector to avoid rebalancing the map's search
Not sure what "rebalancing" here means. map::erase(it) is a const operation.

I'm puzzled about complexity here. Why don't you iterate through the map once
and erase what you need from the store and from |best_matches|
http://stackoverflow.com/questions/8234779/how-to-remove-from-a-map-while-ite...
?

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:54:
PasswordForm CreatePendingGAIA(const char* username, const char* password) {
Though I'm OK with these parameters, I'm still curious why you don't use
StringPiece here?
They are constructed anyway below.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:530:
EXPECT_CALL(*mock_store_, RemoveLogin(_)).WillOnce(SaveArg<0>(&saved));
EXPECT_CALL(*mock_store_, RemoveLogin(other))?

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:581:
.WillOnce(SaveArg<0>(&saved[2]));
Three EXPECT_CALLs would work?

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:582:
form.origin = GURL("https://accounts.google.com");
Doesn't the previous origin work? If so, why not changing MakeSimpleForm()?

[vabr (Chromium), 2016-06-28 11:43:40]

Thanks for the review!

Could you please have a look again?

Cheers,
Vaclav

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver.h (right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver.h:61: // |*preferred_match|
is nulled. It makes no sense to call this if |pending|
On 2016/06/28 09:42:28, vasilii wrote:
> You should mention that the implementation enforces that |pending| is GAIA.

Done.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl.cc (right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl.cc:108: // Move the
|best_matches| to a vector to avoid rebalancing the map's search
On 2016/06/28 09:42:28, vasilii wrote:
> Not sure what "rebalancing" here means. map::erase(it) is a const operation.

I do not think erase is const, and neither does the reference:
http://www.cplusplus.com/reference/map/map/erase/. Assuming that std::map is
implemented with a red-black tree, every removal potentially involves
rebalancing it: https://en.wikipedia.org/wiki/Red%E2%80%93black_tree#Removal . I
believe this is also the reason why erase invalidates the iterator. 

> 
> I'm puzzled about complexity here. Why don't you iterate through the map once
> and erase what you need from the store and from |best_matches|
>
http://stackoverflow.com/questions/8234779/how-to-remove-from-a-map-while-ite...
> ?

The old code used such for-loop with explicit in-body iterator updates and
comments about invalidation. Such code seems prone to error.

It's hard to argue about efficiency, because I'm not sure how typical
|best_matches| look like here. In most of the cases this takes the early return
on line 97. The cases where there is something to delete have hardly more than a
dozen of credentials, so the overall impact is minor. If most of them are wiped
(which seems plausible to me), the approach with the vector is faster (less
rebalancing, and the new map is much smaller). If most of them stay, working
inside the map would be faster.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:54:
PasswordForm CreatePendingGAIA(const char* username, const char* password) {
On 2016/06/28 09:42:28, vasilii wrote:
> Though I'm OK with these parameters, I'm still curious why you don't use
> StringPiece here?
> They are constructed anyway below.

No objections to StringPiece. Just to make sure the whole file is consistent,
I'll do it in a separate CL https://codereview.chromium.org/2101943003/ and
change the function above as well.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:530:
EXPECT_CALL(*mock_store_, RemoveLogin(_)).WillOnce(SaveArg<0>(&saved));
On 2016/06/28 09:42:28, vasilii wrote:
> EXPECT_CALL(*mock_store_, RemoveLogin(other))?

Done.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:581:
.WillOnce(SaveArg<0>(&saved[2]));
On 2016/06/28 09:42:28, vasilii wrote:
> Three EXPECT_CALLs would work?

I suppose they could, but what would be the advantage? This is less redundant
and a valid use of the GMock API.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:582:
form.origin = GURL("https://accounts.google.com");
On 2016/06/28 09:42:28, vasilii wrote:
> Doesn't the previous origin work?
No, it has to be accounts.google.com, not any other *.google.com domain.

> If so, why not changing MakeSimpleForm()?
MakeSimpleForm does not always need to be a GAIA form, and the name does not
create that expectation. I created a MakeSimpleGAIAForm for the three tests
which needed it. We could change the origin in MakeSimpleForm to, say,
example.org, but that seems outside of the scope of this CL.

[vasilii, 2016-06-28 13:07:55]

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl.cc (right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl.cc:108: // Move the
|best_matches| to a vector to avoid rebalancing the map's search
On 2016/06/28 11:43:40, vabr (Chromium) wrote:
> On 2016/06/28 09:42:28, vasilii wrote:
> > Not sure what "rebalancing" here means. map::erase(it) is a const operation.
> 
> I do not think erase is const, and neither does the reference:
> http://www.cplusplus.com/reference/map/map/erase/. Assuming that std::map is
> implemented with a red-black tree, every removal potentially involves
> rebalancing it: https://en.wikipedia.org/wiki/Red%E2%80%93black_tree#Removal .
I
> believe this is also the reason why erase invalidates the iterator. 
> 
> > 
> > I'm puzzled about complexity here. Why don't you iterate through the map
once
> > and erase what you need from the store and from |best_matches|
> >
>
http://stackoverflow.com/questions/8234779/how-to-remove-from-a-map-while-ite...
> > ?
> 
> The old code used such for-loop with explicit in-body iterator updates and
> comments about invalidation. Such code seems prone to error.
> 
> It's hard to argue about efficiency, because I'm not sure how typical
> |best_matches| look like here. In most of the cases this takes the early
return
> on line 97. The cases where there is something to delete have hardly more than
a
> dozen of credentials, so the overall impact is minor. If most of them are
wiped
> (which seems plausible to me), the approach with the vector is faster (less
> rebalancing, and the new map is much smaller). If most of them stay, working
> inside the map would be faster.

C++11 standard 23.2.4.8 says that a.erase(iterator) has "amortized constant"
complexity. Not sure what you mean by "erase invalidates the iterator".
erase(it) invalidates |it| for obvious reason and doesn't invalidate anything
else (see 23.2.4.9).

My concern was about readability. I see a lot of code in the lines 84-124, 2
temporary vectors, 2 moves to/from the map while the following would do the
work:

for (auto it = best_matches->begin(); it != best_matches->end(); /* increment in
the loop*/) {
  if (long condition)
    it = best_matches->erase(it);
  else
    it++;
}

Why do you think it's error-prone?

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:54:
PasswordForm CreatePendingGAIA(const char* username, const char* password) {
On 2016/06/28 11:43:40, vabr (Chromium) wrote:
> On 2016/06/28 09:42:28, vasilii wrote:
> > Though I'm OK with these parameters, I'm still curious why you don't use
> > StringPiece here?
> > They are constructed anyway below.
> 
> No objections to StringPiece. Just to make sure the whole file is consistent,
> I'll do it in a separate CL https://codereview.chromium.org/2101943003/ and
> change the function above as well.

Acknowledged.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:581:
.WillOnce(SaveArg<0>(&saved[2]));
On 2016/06/28 11:43:40, vabr (Chromium) wrote:
> On 2016/06/28 09:42:28, vasilii wrote:
> > Three EXPECT_CALLs would work?
> 
> I suppose they could, but what would be the advantage? This is less redundant
> and a valid use of the GMock API.

The advantage would be the error message:
"Expected array is {300 lines here} while |saved| is {300 lines here}" vs.
"EXPECT_CALL(old) unsatisfied. There was a similar call with {one form - 100
lines}".

If the store is called just twice, the first one would be more difficult to
analyze.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:582:
form.origin = GURL("https://accounts.google.com");
On 2016/06/28 11:43:40, vabr (Chromium) wrote:
> On 2016/06/28 09:42:28, vasilii wrote:
> > Doesn't the previous origin work?
> No, it has to be http://accounts.google.com, not any other *.google.com
domain.
> 
> > If so, why not changing MakeSimpleForm()?
> MakeSimpleForm does not always need to be a GAIA form, and the name does not
> create that expectation. I created a MakeSimpleGAIAForm for the three tests
> which needed it. We could change the origin in MakeSimpleForm to, say,
> http://example.org, but that seems outside of the scope of this CL.

Acknowledged.

https://codereview.chromium.org/2098573002/diff/140001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/140001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:546:
EXPECT_CALL(*mock_store_, RemoveLogin(_)).WillOnce(SaveArg<0>(&saved));
EXPECT_CALL(*mock_store_, RemoveLogin(other));

[vabr (Chromium), 2016-06-28 14:57:38]

Thanks Vasilii,
Comments addressed, please take a look.

Cheers,
Vaclav

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl.cc (right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl.cc:108: // Move the
|best_matches| to a vector to avoid rebalancing the map's search
On 2016/06/28 13:07:55, vasilii wrote:
> On 2016/06/28 11:43:40, vabr (Chromium) wrote:
> > On 2016/06/28 09:42:28, vasilii wrote:
> > > Not sure what "rebalancing" here means. map::erase(it) is a const
operation.
> > 
> > I do not think erase is const, and neither does the reference:
> > http://www.cplusplus.com/reference/map/map/erase/. Assuming that std::map is
> > implemented with a red-black tree, every removal potentially involves
> > rebalancing it: https://en.wikipedia.org/wiki/Red%E2%80%93black_tree#Removal
.
> I
> > believe this is also the reason why erase invalidates the iterator. 
> > 
> > > 
> > > I'm puzzled about complexity here. Why don't you iterate through the map
> once
> > > and erase what you need from the store and from |best_matches|
> > >
> >
>
http://stackoverflow.com/questions/8234779/how-to-remove-from-a-map-while-ite...
> > > ?
> > 
> > The old code used such for-loop with explicit in-body iterator updates and
> > comments about invalidation. Such code seems prone to error.
> > 
> > It's hard to argue about efficiency, because I'm not sure how typical
> > |best_matches| look like here. In most of the cases this takes the early
> return
> > on line 97. The cases where there is something to delete have hardly more
than
> a
> > dozen of credentials, so the overall impact is minor. If most of them are
> wiped
> > (which seems plausible to me), the approach with the vector is faster (less
> > rebalancing, and the new map is much smaller). If most of them stay, working
> > inside the map would be faster.
> 
> C++11 standard 23.2.4.8 says that a.erase(iterator) has "amortized constant"
> complexity. Not sure what you mean by "erase invalidates the iterator".
> erase(it) invalidates |it| for obvious reason and doesn't invalidate anything
> else (see 23.2.4.9).
> 
> My concern was about readability. I see a lot of code in the lines 84-124, 2
> temporary vectors, 2 moves to/from the map while the following would do the
> work:
> 
> for (auto it = best_matches->begin(); it != best_matches->end(); /* increment
in
> the loop*/) {
>   if (long condition)
>     it = best_matches->erase(it);
>   else
>     it++;
> }
> 
> Why do you think it's error-prone?

I rewrote the code to use the simple for loop to compare readabilty and now I
agree with you. I feared that the need to increment in the loop body would be
error-prone, but the code is so much shorter and the intention clearer that I
withdraw my objection.

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/100001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:581:
.WillOnce(SaveArg<0>(&saved[2]));
On 2016/06/28 13:07:55, vasilii wrote:
> On 2016/06/28 11:43:40, vabr (Chromium) wrote:
> > On 2016/06/28 09:42:28, vasilii wrote:
> > > Three EXPECT_CALLs would work?
> > 
> > I suppose they could, but what would be the advantage? This is less
redundant
> > and a valid use of the GMock API.
> 
> The advantage would be the error message:
> "Expected array is {300 lines here} while |saved| is {300 lines here}" vs.
> "EXPECT_CALL(old) unsatisfied. There was a similar call with {one form - 100
> lines}".
> 
> If the store is called just twice, the first one would be more difficult to
> analyze.

Ah, I misunderstood you the first time, I though you suggested to keep the
SaveArgs, and missed that I should actually match the forms directly. Now done,
thanks for patience.

https://codereview.chromium.org/2098573002/diff/140001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl_unittest.cc
(right):

https://codereview.chromium.org/2098573002/diff/140001/components/password_ma...
components/password_manager/core/browser/form_saver_impl_unittest.cc:546:
EXPECT_CALL(*mock_store_, RemoveLogin(_)).WillOnce(SaveArg<0>(&saved));
On 2016/06/28 13:07:55, vasilii wrote:
> EXPECT_CALL(*mock_store_, RemoveLogin(other));

Done.

[vabr (Chromium), 2016-06-28 14:59:43]

Sorry, I messed up the upload, changes to the for loop will come in the next
patchset, I'll ping the CL.

[vabr (Chromium), 2016-06-28 15:04:09]

On 2016/06/28 14:59:43, vabr (Chromium) wrote:
> Sorry, I messed up the upload, changes to the for loop will come in the next
> patchset, I'll ping the CL.

Patch set 10 is now ready for review.

[vasilii, 2016-06-28 15:10:40]

lgtm

https://codereview.chromium.org/2098573002/diff/180001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl.cc (right):

https://codereview.chromium.org/2098573002/diff/180001/components/password_ma...
components/password_manager/core/browser/form_saver_impl.cc:11: #include
<vector>
Probably you don't need most of those now.

[vabr (Chromium), 2016-06-28 16:00:04]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-06-28 16:00:04]

The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2098573002/#ps210001
(title: "Just rebased")

[commit-bot: I haz the power, 2016-06-28 16:00:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-06-28 16:00:44]

Thanks!

https://codereview.chromium.org/2098573002/diff/180001/components/password_ma...
File components/password_manager/core/browser/form_saver_impl.cc (right):

https://codereview.chromium.org/2098573002/diff/180001/components/password_ma...
components/password_manager/core/browser/form_saver_impl.cc:11: #include
<vector>
On 2016/06/28 15:10:40, vasilii wrote:
> Probably you don't need most of those now.

Right, I forgot those. Done.

[commit-bot: I haz the power, 2016-06-28 19:09:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 19:09:59]

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_...)

[vabr (Chromium), 2016-06-29 06:33:33]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-06-29 06:33:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 07:21:41]

Description was changed from

==========
Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

PasswordFormManager ensures that outdated sync credentials are deleted, to help
eliminate the situation when the sync credential is stored in the password store
it protects.

This CL moves the logic determining which forms to drop and the Remove call to
PasswordStore from PasswordFormManager to FormSaver, the delegate supposed to be
handling store operations for PFM. This leaves only reading calls to
PasswordStore in PasswordFormManager. The CL also adds and removes tests and
modifies test support as appropriate.

This addresses a TODO from https://codereview.chromium.org/2090583003/. It
concludes the first transition step from the design
https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtP...
.

BUG=621355
==========

to

==========
Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

PasswordFormManager ensures that outdated sync credentials are deleted, to help
eliminate the situation when the sync credential is stored in the password store
it protects.

This CL moves the logic determining which forms to drop and the Remove call to
PasswordStore from PasswordFormManager to FormSaver, the delegate supposed to be
handling store operations for PFM. This leaves only reading calls to
PasswordStore in PasswordFormManager. The CL also adds and removes tests and
modifies test support as appropriate.

This addresses a TODO from https://codereview.chromium.org/2090583003/. It
concludes the first transition step from the design
https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtP...
.

BUG=621355
==========

[commit-bot: I haz the power, 2016-06-29 07:21:42]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-06-29 07:23:33]

Description was changed from

==========
Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

PasswordFormManager ensures that outdated sync credentials are deleted, to help
eliminate the situation when the sync credential is stored in the password store
it protects.

This CL moves the logic determining which forms to drop and the Remove call to
PasswordStore from PasswordFormManager to FormSaver, the delegate supposed to be
handling store operations for PFM. This leaves only reading calls to
PasswordStore in PasswordFormManager. The CL also adds and removes tests and
modifies test support as appropriate.

This addresses a TODO from https://codereview.chromium.org/2090583003/. It
concludes the first transition step from the design
https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtP...
.

BUG=621355
==========

to

==========
Implement PasswordFormManager::WipeStoreCopyIfOutdated in FormSaver

PasswordFormManager ensures that outdated sync credentials are deleted, to help
eliminate the situation when the sync credential is stored in the password store
it protects.

This CL moves the logic determining which forms to drop and the Remove call to
PasswordStore from PasswordFormManager to FormSaver, the delegate supposed to be
handling store operations for PFM. This leaves only reading calls to
PasswordStore in PasswordFormManager. The CL also adds and removes tests and
modifies test support as appropriate.

This addresses a TODO from https://codereview.chromium.org/2090583003/. It
concludes the first transition step from the design
https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtP...
.

BUG=621355

Committed: https://crrev.com/8afac70d8152dafd3a4e539a095c12324e34d63d
Cr-Commit-Position: refs/heads/master@{#402745}
==========

[commit-bot: I haz the power, 2016-06-29 07:23:34]

Patchset 12 (id:??) landed as
https://crrev.com/8afac70d8152dafd3a4e539a095c12324e34d63d
Cr-Commit-Position: refs/heads/master@{#402745}