Support to remove passwords by date_synced timestamp.

Support to remove passwords by date_synced timestamp.

The new method RemoveLoginsSyncedBetween() works exactly like the existing RemoveLoginsCreatedBetween(). 

BUG=362679
TBR=zea@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277863

[vasilii, 2014-06-13 18:27:34]

Hi Garrett,

please review. It's the second (and hopefully last) patch for the "Remote
Unsync" project.

[vasilii, 2014-06-16 13:28:44]

Nicolas, please review passwords_helper.cc.
Garrett, please review everything else.

[vasilii, 2014-06-17 08:14:54]

Vaclav, please review the CL.

[vabr (Chromium), 2014-06-17 15:28:41]

Hi Vasilii,

I'm publishing the first part of the comments, and will finish the rest of the
CL later today.

Thanks!
Vaclav

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:743:
forms->reserve(forms->size() + all_forms.size());
What are the reasons for this speed optimisation?
Have we done any benchmarks to prove the need for that?
If not, I'm not sure it's worth to trade the space and one more line of code for
the speed.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.h (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:91: base::Time
delete_end) OVERRIDE;
Please also comment on the special meaning of the default base::Time value for
|delete_end| (in the public methods at least, but ideally also in the private
ones as well).

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:98: virtual bool
GetLoginsCreatedBetween(base::Time get_begin, base::Time get_end,
optional nit: Is this formatting according to clang-format?
It's probably fine with the style guide, I just guess clang-format would put
every argument on one line. I see a value in following clang-format in that
potential refactoring later can rely on running clang-format and inducing
changes only on places which were refactored.

As this repeats on couple of places, my suggestion would be to run git cl format
on the whole CL, and accept those changes unless there is a strong reason
against.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:113: // Retrieves
password created/synced in the time interval.
Please comment on the return value (also below).

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:115: bool
date_is_creation,
|date_is_creation| would deserve an explanation. I did not understand what that
means until I read the definition of RemoveLogins*Between in the .cc file.

Even better, I'd suggest turning it from a bool to an enum like
enum TimestampToCompare { CREATION_TIMESTAMP, SYNC_TIMESTAMP };

That would avoid the need to add the comment, and as a bonus, make the meaning
of the argument clear on the callsites.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:494: bool
NativeBackendKWallet::GetLoginsList(PasswordFormList* forms,
This method looks sufficiently similar to NativeBackendGnome::GetLoginsBetween.
Could they benefit from a shared method in PasswordStoreX::NativeBackend?

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.h (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Many of the comments I made for Gnome apply here as well. Please take them into
account as appropriate.

[vabr (Chromium), 2014-06-17 16:53:31]

LGTM on the condition of both my sets of comments being addressed.

Cheers,
Vaclav

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.h (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:91: base::Time
delete_end) OVERRIDE;
On 2014/06/17 15:28:40, vabr (Chromium) wrote:
> Please also comment on the special meaning of the default base::Time value for
> |delete_end| (in the public methods at least, but ideally also in the private
> ones as well).

The appropriate place for the comment is actually in PasswordStoreX.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac.h (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac.h:64:
RemoveLoginsSyncedBetweenImpl(const base::Time& delete_begin,
nit: Given that base::Time contains just an int, you can pass by value here, as
you do for Gnome & KDE.

https://codereview.chromium.org/335893002/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/335893002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.h:60: bool
RemoveLoginsSyncedBetween(const base::Time delete_begin,
nit: remove const
(I appreciate the attempt to be consistent with the rest of the code, but I
believe the consistency across the similar APIs for different platforms is more
important. In this respect, the code is actually ripe for a clean-up CL to
always pass base::Time by value, not by const ref or const value.)

[vasilii, 2014-06-17 17:24:15]

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:743:
forms->reserve(forms->size() + all_forms.size());
On 2014/06/17 15:28:40, vabr (Chromium) wrote:
> What are the reasons for this speed optimisation?
> Have we done any benchmarks to prove the need for that?
> If not, I'm not sure it's worth to trade the space and one more line of code
for
> the speed.

Done. I copied it from GetLoginsCreatedBetween(). I don't believe there were
benchmarks here.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.h (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:91: base::Time
delete_end) OVERRIDE;
On 2014/06/17 16:53:30, vabr (Chromium) wrote:
> On 2014/06/17 15:28:40, vabr (Chromium) wrote:
> > Please also comment on the special meaning of the default base::Time value
for
> > |delete_end| (in the public methods at least, but ideally also in the
private
> > ones as well).
> 
> The appropriate place for the comment is actually in PasswordStoreX.

Done.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:98: virtual bool
GetLoginsCreatedBetween(base::Time get_begin, base::Time get_end,
On 2014/06/17 15:28:40, vabr (Chromium) wrote:
> optional nit: Is this formatting according to clang-format?
> It's probably fine with the style guide, I just guess clang-format would put
> every argument on one line. I see a value in following clang-format in that
> potential refactoring later can rely on running clang-format and inducing
> changes only on places which were refactored.
> 
> As this repeats on couple of places, my suggestion would be to run git cl
format
> on the whole CL, and accept those changes unless there is a strong reason
> against.

Done.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:113: // Retrieves
password created/synced in the time interval.
On 2014/06/17 15:28:40, vabr (Chromium) wrote:
> Please comment on the return value (also below).

Done.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:115: bool
date_is_creation,
On 2014/06/17 15:28:40, vabr (Chromium) wrote:
> |date_is_creation| would deserve an explanation. I did not understand what
that
> means until I read the definition of RemoveLogins*Between in the .cc file.
> 
> Even better, I'd suggest turning it from a bool to an enum like
> enum TimestampToCompare { CREATION_TIMESTAMP, SYNC_TIMESTAMP };
> 
> That would avoid the need to add the comment, and as a bonus, make the meaning
> of the argument clear on the callsites.

Done.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:494: bool
NativeBackendKWallet::GetLoginsList(PasswordFormList* forms,
On 2014/06/17 15:28:40, vabr (Chromium) wrote:
> This method looks sufficiently similar to
NativeBackendGnome::GetLoginsBetween.
> Could they benefit from a shared method in PasswordStoreX::NativeBackend?

PasswordStoreX::NativeBackend is an interface. It doesn't have any
implementation.
Actually, this method should gone. There is no GetLoginsSyncedBetween() in
PasswordStoreX::NativeBackend because it's excessive. We call it together with
RemoveLoginsCreatedBetween, therefore they should be merged. After this I can
just remove GetLoginsCreatedBetween() because RemoveLoginsCreatedBetween()
doesn't depend on it.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.h (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/06/17 15:28:41, vabr (Chromium) wrote:
> Many of the comments I made for Gnome apply here as well. Please take them
into
> account as appropriate.

Done.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac.h (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac.h:64:
RemoveLoginsSyncedBetweenImpl(const base::Time& delete_begin,
On 2014/06/17 16:53:30, vabr (Chromium) wrote:
> nit: Given that base::Time contains just an int, you can pass by value here,
as
> you do for Gnome & KDE.

Done.

https://codereview.chromium.org/335893002/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/335893002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.h:60: bool
RemoveLoginsSyncedBetween(const base::Time delete_begin,
On 2014/06/17 16:53:31, vabr (Chromium) wrote:
> nit: remove const
> (I appreciate the attempt to be consistent with the rest of the code, but I
> believe the consistency across the similar APIs for different platforms is
more
> important. In this respect, the code is actually ripe for a clean-up CL to
> always pass base::Time by value, not by const ref or const value.)

Done.

[vasilii, 2014-06-17 17:27:06]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:29:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/335893002/40001

[Garrett Casto, 2014-06-17 17:57:36]

lgtm

Terribly sorry for the late review, just a few things that can wait for a later
CL. It does seem like this area is ripe for a cleanup pass to consolidate some
existing code duplication if anyone ever has time for it.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:119:
backend_->RemoveLoginsSyncedBetween(delete_begin, delete_end, &changes)) {
Seems like we should standardize how we report changes between
RemoveLoginsSyncedBetween() and RemoveLoginsCreatedBetween(). The way we are
doing it here seems reasonable as it removes the GetLoginsCreatedBetween(),
though doing it the other way it closer to how LoginDatabase is structured. I I
think that I prefer this way but it doesn't particular matter which we choose,
it just seems like there is no reason for them to be different.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:120: allow_fallback_ =
false;
It also seems like it would be nice if we had UMA stats for this, possibly
update LogStatsForBulkDeletion to handle this? Doesn't need to be in this CL,
but could you file a bug for this so it doesn't get forgotten?

[commit-bot: I haz the power, 2014-06-17 21:12:48]

Change committed as 277863

[Nicolas Zea, 2014-06-17 21:41:12]

https://codereview.chromium.org/335893002/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/passwords_helper.cc (right):

https://codereview.chromium.org/335893002/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/passwords_helper.cc:152:
password_manager::ContainsSamePasswordForms(verifier_forms, forms);
Does it make sense to modify this method instead to not compare date_synced?

[vasilii, 2014-06-18 10:00:57]

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:119:
backend_->RemoveLoginsSyncedBetween(delete_begin, delete_end, &changes)) {
On 2014/06/17 17:57:36, Garrett Casto wrote:
> Seems like we should standardize how we report changes between
> RemoveLoginsSyncedBetween() and RemoveLoginsCreatedBetween(). The way we are
> doing it here seems reasonable as it removes the GetLoginsCreatedBetween(),
> though doing it the other way it closer to how LoginDatabase is structured. I
I
> think that I prefer this way but it doesn't particular matter which we choose,
> it just seems like there is no reason for them to be different.

You are right. The plan is to remove GetLoginsCreatedBetween() in the separate
CL.

https://codereview.chromium.org/335893002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:120: allow_fallback_ =
false;
On 2014/06/17 17:57:36, Garrett Casto wrote:
> It also seems like it would be nice if we had UMA stats for this, possibly
> update LogStatsForBulkDeletion to handle this? Doesn't need to be in this CL,
> but could you file a bug for this so it doesn't get forgotten?

http://crbug.com/386079. The new method will be called when user unsyncs his
profile locally or remotely. I think there will be histograms for that.

https://codereview.chromium.org/335893002/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/passwords_helper.cc (right):

https://codereview.chromium.org/335893002/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/passwords_helper.cc:152:
password_manager::ContainsSamePasswordForms(verifier_forms, forms);
On 2014/06/17 21:41:11, Nicolas Zea wrote:
> Does it make sense to modify this method instead to not compare date_synced?

ContainsSamePasswordForms() resides in components/. If it was just for sync
integration, I'd agree. But now it can be reused in non-sync scenarios where
date_synced may be important. PasswordForm::operator== definitely should compare
all fields.

[vabr (Chromium), 2014-06-18 12:01:03]

Thanks for addressing the comments, LGTM.

Cheers,
Vaclav