Return a list of changed from WebDatabase::RemoveFormElementsAddedBetween()

Return a list of changed from WebDatabase::RemoveFormElementsAddedBetween()
This changes add a list of AutofillChange out parameter to the RemoveFormElementsAddedBetween() method in order to communicate exactly what happened to the caller.  This method may only update some autofill entries (by removing use timestamps that fall in the specified time range) or completely remove the autofill entry (if all the use timestamps fall between the specified time range).  For sync, we need to know the difference.

This change also required some new testing method on WebDatabase to make it possible to add new autofill entries with specific timestamps.  This is needed to create a reliable test that requires entries to be added at different times.  If there is a better way to do this, please let me know.

The next change will add the notifications to the WebDataService.

BUG=30169
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34889

[Paweł Hajdan Jr., 2009-12-17 06:50:57]

Drive-by with test-related comments.

http://codereview.chromium.org/506047/diff/1/5
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/506047/diff/1/5#newcode508
chrome/browser/webdata/web_database_unittest.cc:508: for (uint i = 0; i <
arraysize(expected_changes); i++) {
nit: shouldn't it be size_t instead of uint?

http://codereview.chromium.org/506047/diff/1/5#newcode509
chrome/browser/webdata/web_database_unittest.cc:509:
EXPECT_TRUE(expected_changes[i] == changes[i]);
Please add some message (via SCOPED_TRACE or <<) with more info when this test
assertion fails.

http://codereview.chromium.org/506047/diff/1/5#newcode563
chrome/browser/webdata/web_database_unittest.cc:563:
EXPECT_TRUE(db.Init(file_));
Technically this should also be ASSERT_TRUE, but I see it as EXPECT_TRUE in
other places in the file. Changing it in one place would be confusing. Feel free
to change in all places or not at all.

http://codereview.chromium.org/506047/diff/1/5#newcode584
chrome/browser/webdata/web_database_unittest.cc:584: EXPECT_EQ(1U,
changes.size());
ASSERT_TRUE to avoid a crash if changes are empty?

http://codereview.chromium.org/506047/diff/1/5#newcode592
chrome/browser/webdata/web_database_unittest.cc:592: EXPECT_EQ(1U,
changes.size());
ASSERT_TRUE?

[skrul, 2009-12-17 18:34:33]

Thanks for the drive by :)

http://codereview.chromium.org/506047/diff/1/5
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/506047/diff/1/5#newcode508
chrome/browser/webdata/web_database_unittest.cc:508: for (uint i = 0; i <
arraysize(expected_changes); i++) {
On 2009/12/17 06:50:57, Paweł Hajdan Jr. wrote:
> nit: shouldn't it be size_t instead of uint?

Done.

http://codereview.chromium.org/506047/diff/1/5#newcode509
chrome/browser/webdata/web_database_unittest.cc:509:
EXPECT_TRUE(expected_changes[i] == changes[i]);
On 2009/12/17 06:50:57, Paweł Hajdan Jr. wrote:
> Please add some message (via SCOPED_TRACE or <<) with more info when this test
> assertion fails.

I went ahead and added operator<< to these classes so I can use EXPECT_EQ.

http://codereview.chromium.org/506047/diff/1/5#newcode563
chrome/browser/webdata/web_database_unittest.cc:563:
EXPECT_TRUE(db.Init(file_));
On 2009/12/17 06:50:57, Paweł Hajdan Jr. wrote:
> Technically this should also be ASSERT_TRUE, but I see it as EXPECT_TRUE in
> other places in the file. Changing it in one place would be confusing. Feel
free
> to change in all places or not at all.

Done.

http://codereview.chromium.org/506047/diff/1/5#newcode584
chrome/browser/webdata/web_database_unittest.cc:584: EXPECT_EQ(1U,
changes.size());
On 2009/12/17 06:50:57, Paweł Hajdan Jr. wrote:
> ASSERT_TRUE to avoid a crash if changes are empty?

Done.

http://codereview.chromium.org/506047/diff/1/5#newcode592
chrome/browser/webdata/web_database_unittest.cc:592: EXPECT_EQ(1U,
changes.size());
On 2009/12/17 06:50:57, Paweł Hajdan Jr. wrote:
> ASSERT_TRUE?

Done.

[Paweł Hajdan Jr., 2009-12-17 18:39:19]

Tests LGTM, thanks.

[James Hawkins, 2009-12-17 20:09:25]

http://codereview.chromium.org/506047/diff/3001/3002
File chrome/browser/webdata/autofill_change.cc (right):

http://codereview.chromium.org/506047/diff/3001/3002#newcode6
chrome/browser/webdata/autofill_change.cc:6: #include
"chrome/browser/webdata/autofill_change.h"
autofill_change.h should be the first include in the file.

http://codereview.chromium.org/506047/diff/3001/3002#newcode27
chrome/browser/webdata/autofill_change.cc:27: return os << " "; // <<
change.key();
Weird spacing on this last line?

http://codereview.chromium.org/506047/diff/3001/3003
File chrome/browser/webdata/autofill_change.h (right):

http://codereview.chromium.org/506047/diff/3001/3003#newcode33
chrome/browser/webdata/autofill_change.h:33: std::ostream&
operator<<(std::ostream& os, const AutofillChange& change);
streams are highly discouraged.  Is this for the tests?

http://codereview.chromium.org/506047/diff/3001/3004
File chrome/browser/webdata/autofill_entry.cc (right):

http://codereview.chromium.org/506047/diff/3001/3004#newcode6
chrome/browser/webdata/autofill_entry.cc:6: #include
"chrome/browser/webdata/autofill_entry.h"
autofill_entry.h should be the first include in the file.

http://codereview.chromium.org/506047/diff/3001/3005
File chrome/browser/webdata/autofill_entry.h (right):

http://codereview.chromium.org/506047/diff/3001/3005#newcode31
chrome/browser/webdata/autofill_entry.h:31: std::ostream&
operator<<(std::ostream& os, const AutofillKey& key);
Streams are highly discouraged.

http://codereview.chromium.org/506047/diff/3001/3006
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/506047/diff/3001/3006#newcode658
chrome/browser/webdata/web_data_service.cc:658: &changes))
It seems the purpose of |changes| is for debugging purposes.  Can you change
RemoveFormElementsAddedBetween to accept a NULL |changes| when we don't care
about the value?

http://codereview.chromium.org/506047/diff/3001/3007
File chrome/browser/webdata/web_database.cc (right):

http://codereview.chromium.org/506047/diff/3001/3007#newcode1060
chrome/browser/webdata/web_database.cc:1060: AutofillChange::REMOVE :
nit: Can you refactor this "was_removed ? ..." into its own line for
readability?

[skrul, 2009-12-17 20:52:32]

http://codereview.chromium.org/506047/diff/3001/3002
File chrome/browser/webdata/autofill_change.cc (right):

http://codereview.chromium.org/506047/diff/3001/3002#newcode6
chrome/browser/webdata/autofill_change.cc:6: #include
"chrome/browser/webdata/autofill_change.h"
On 2009/12/17 20:09:25, James Hawkins wrote:
> autofill_change.h should be the first include in the file.

Done.

http://codereview.chromium.org/506047/diff/3001/3002#newcode27
chrome/browser/webdata/autofill_change.cc:27: return os << " "; // <<
change.key();
On 2009/12/17 20:09:25, James Hawkins wrote:
> Weird spacing on this last line?

Done.

http://codereview.chromium.org/506047/diff/3001/3003
File chrome/browser/webdata/autofill_change.h (right):

http://codereview.chromium.org/506047/diff/3001/3003#newcode33
chrome/browser/webdata/autofill_change.h:33: std::ostream&
operator<<(std::ostream& os, const AutofillChange& change);
On 2009/12/17 20:09:25, James Hawkins wrote:
> streams are highly discouraged.  Is this for the tests?

This was only needed for testing -- I've moved it into the test.

http://codereview.chromium.org/506047/diff/3001/3004
File chrome/browser/webdata/autofill_entry.cc (right):

http://codereview.chromium.org/506047/diff/3001/3004#newcode6
chrome/browser/webdata/autofill_entry.cc:6: #include
"chrome/browser/webdata/autofill_entry.h"
On 2009/12/17 20:09:25, James Hawkins wrote:
> autofill_entry.h should be the first include in the file.

Done.

http://codereview.chromium.org/506047/diff/3001/3005
File chrome/browser/webdata/autofill_entry.h (right):

http://codereview.chromium.org/506047/diff/3001/3005#newcode31
chrome/browser/webdata/autofill_entry.h:31: std::ostream&
operator<<(std::ostream& os, const AutofillKey& key);
On 2009/12/17 20:09:25, James Hawkins wrote:
> Streams are highly discouraged.

Moved into the test.

http://codereview.chromium.org/506047/diff/3001/3006
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/506047/diff/3001/3006#newcode658
chrome/browser/webdata/web_data_service.cc:658: &changes))
On 2009/12/17 20:09:25, James Hawkins wrote:
> It seems the purpose of |changes| is for debugging purposes.  Can you change
> RemoveFormElementsAddedBetween to accept a NULL |changes| when we don't care
> about the value?

This exists temporarily to allow me to put only the API change in this CL and
finish the implementation in the next CL.  I can make this CL larger and include
the additional changes here if you'd like.

http://codereview.chromium.org/506047/diff/3001/3007
File chrome/browser/webdata/web_database.cc (right):

http://codereview.chromium.org/506047/diff/3001/3007#newcode1060
chrome/browser/webdata/web_database.cc:1060: AutofillChange::REMOVE :
On 2009/12/17 20:09:25, James Hawkins wrote:
> nit: Can you refactor this "was_removed ? ..." into its own line for
> readability?

Done.

[James Hawkins, 2009-12-17 21:02:57]

> This exists temporarily to allow me to put only the API change in this CL and
> finish the implementation in the next CL.  I can make this CL larger and
include
> the additional changes here if you'd like.
> 

No, that's fine.  LGTM