Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9585020:
Cull autofill entries older than 60 days. (Closed)
|
Created:
8 years, 9 months ago by GeorgeY Modified:
8 years, 9 months ago CC:
chromium-reviews, dhollowa+watch_chromium.org, akalin, Nicolas Zea, tim (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCull autofill entries older than 60 days.
Tested with 20K Autocomplete entries selected 100000 times. 500 of them are in range.
---Start MergeDataAndStartSyncing for autofill
Read from db - 7s
Sift through them selecting only those needed for update - ~0.5s
Process 476 changes - <1s
---End MergeDataAndStartSyncing for autofill - total ~8.5 s
Callback back after deleting extra entries - 17s
BUG=28990
TEST=unit-test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129097
Patch Set 1 #Patch Set 2 : Fixed the unit test #Patch Set 3 : Fixed couple of issues #Patch Set 4 : fix spelling #
Total comments: 21
Patch Set 5 : Addressed comments and added culling when the sync is off. #
Total comments: 2
Patch Set 6 : Changed Autofill to keep only two timestamps #Patch Set 7 : Fixed clang #
Total comments: 26
Patch Set 8 : Addressed the comments #
Total comments: 34
Patch Set 9 : adressed comments #
Total comments: 17
Patch Set 10 : Addressed comments #
Total comments: 8
Patch Set 11 : addressed comments #
Total comments: 6
Patch Set 12 : #Patch Set 13 : if (!RemoveFormElementForID(pair_id)) #
Total comments: 12
Patch Set 14 : addressed comments #Patch Set 15 : addressed comments - re-uploading as the first one failed #
Total comments: 4
Patch Set 16 : fixed error #Patch Set 17 : Addressed comment #Messages
Total messages: 26 (0 generated)
I've always wondered: Why do we need to save lots of timestamps alongside each autocomplete item? Could we simplify a bunch of this logic by only saving one or two timestamps? https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:114: base::TimeDelta::FromDays(AutofillEntry::kExpirationPeriodInDays); nit: might as well write these three lines as two lines by doing the calculation immediately. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:115: for (size_t i = 0; i < new_synced_entries.size() && !need_to_cull_data; ++i) nit: The loop body is multiple lines, so please add curly braces. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:136: // delete it locally. I don't follow how this tells us that the key is not on the server. Don't we also reach this case if the key is on the server and has different timestamps than the local data? https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:142: if (need_to_cull_data) { Why not just always make this call? It should be a cheap no-op if nothing needs to be culled, right? https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:147: culling_time); nit: WDS::RemoveFormElementsAddedBefore(culling_time) seems like a better public interface for this. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:76: for (size_t i = 0; i < timestamps_.size(); ++i) nit: The loop body is more than one line, so please add curly braces. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:89: } nit: One-line if-stmt, so no need for curlys. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:96: timestamps_culled_ = true; It seems a little odd to me that we're using timestamps_culled_ to mean two different things... Why does one type of culling happen in the constructor, whereas the other type is initiated externally? https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.h (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:58: static const int64 kExpirationPeriodInDays = 60; nit: This should be defined wherever it is used, rather than here. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry_unittest.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry_unittest.cc:92: } Might be good to also test a timestamp exactly at the cutoff boundary.
https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:114: base::TimeDelta::FromDays(AutofillEntry::kExpirationPeriodInDays); On 2012/03/06 08:48:35, Ilya Sherman wrote: > nit: might as well write these three lines as two lines by doing the calculation > immediately. Done. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:115: for (size_t i = 0; i < new_synced_entries.size() && !need_to_cull_data; ++i) On 2012/03/06 08:48:35, Ilya Sherman wrote: > nit: The loop body is multiple lines, so please add curly braces. Done. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:136: // delete it locally. On 2012/03/06 08:48:35, Ilya Sherman wrote: > I don't follow how this tells us that the key is not on the server. Don't we > also reach this case if the key is on the server and has different timestamps > than the local data? Then it would be SyncChange::ACTION_UPDATE https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:142: if (need_to_cull_data) { On 2012/03/06 08:48:35, Ilya Sherman wrote: > Why not just always make this call? It should be a cheap no-op if nothing needs > to be culled, right? Yes, but the call *always* schedules extra task on the DB thread, this check avoids it. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:147: culling_time); On 2012/03/06 08:48:35, Ilya Sherman wrote: > nit: WDS::RemoveFormElementsAddedBefore(culling_time) seems like a better public > interface for this. Why create a trivial interface (it would call RemoveFormElementsAddedBetween) internally, when it is so easy to use this one? (RemoveFormElementsAddedBetween is used in 11 other places). https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:76: for (size_t i = 0; i < timestamps_.size(); ++i) On 2012/03/06 08:48:35, Ilya Sherman wrote: > nit: The loop body is more than one line, so please add curly braces. Done. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:89: } On 2012/03/06 08:48:35, Ilya Sherman wrote: > nit: One-line if-stmt, so no need for curlys. Done. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:96: timestamps_culled_ = true; On 2012/03/06 08:48:35, Ilya Sherman wrote: > It seems a little odd to me that we're using timestamps_culled_ to mean two > different things... Why does one type of culling happen in the constructor, > whereas the other type is initiated externally? Because the original culling just limits the number of timestamps taken in the consideration, they are *not* deleted from anywhere, they just not used (or synced). Time culling on the other hand is a more involved operation, as it is done both on the object and with the db, so it is done occasionally externally. In both cases |timestamps_culled_| means that the object was changed and needs to be re-synced. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.h (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:58: static const int64 kExpirationPeriodInDays = 60; On 2012/03/06 08:48:35, Ilya Sherman wrote: > nit: This should be defined wherever it is used, rather than here. It is used in two completely different places, this looks like the correct place to put it in. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry_unittest.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autofill_entry_unittest.cc:92: } On 2012/03/06 08:48:35, Ilya Sherman wrote: > Might be good to also test a timestamp exactly at the cutoff boundary. Done.
As discussed offline, it's probably not safe to delete old timestamps from autocomplete items that also have new timestamps, since this will break "Clear Browsing Data" functionality. https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/10001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:136: // delete it locally. On 2012/03/09 20:14:24, GeorgeY wrote: > On 2012/03/06 08:48:35, Ilya Sherman wrote: > > I don't follow how this tells us that the key is not on the server. Don't we > > also reach this case if the key is on the server and has different timestamps > > than the local data? > > Then it would be SyncChange::ACTION_UPDATE Ah, makes sense -- thanks. I think it would help to move the comment into the inner if-stmt -- that was part of what threw me off, since at a glance it looks like that applies to the whole else-stmt. https://chromiumcodereview.appspot.com/9585020/diff/17001/chrome/browser/auto... File chrome/browser/autofill/personal_data_manager.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/17001/chrome/browser/auto... chrome/browser/autofill/personal_data_manager.cc:166: // As all autofill data is ready, the Autocomplete data is ready as well. nit: "autofill" -> "Autofill"
Changed to store only first and last time query. It is a shame that SQL does not support cursors as the same subquery executed 3 times, leading to 1 minute 23 seconds of db thread time for 1000000 entries (Chrome is responsive all that time). TODO in the future: 1. Collapse autofill_dates into autofill - did not do it now as table upgrades run on database initialization, so we need to reduce db size first. 2. Add |count| to the sync https://chromiumcodereview.appspot.com/9585020/diff/17001/chrome/browser/auto... File chrome/browser/autofill/personal_data_manager.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/17001/chrome/browser/auto... chrome/browser/autofill/personal_data_manager.cc:166: // As all autofill data is ready, the Autocomplete data is ready as well. On 2012/03/09 23:15:22, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" Done.
On 2012/03/14 21:55:30, GeorgeY wrote: > Changed to store only first and last time query. > It is a shame that SQL does not support cursors as the same subquery executed 3 > times, leading to 1 minute 23 seconds of db thread time for 1000000 entries > (Chrome is responsive all that time). > > TODO in the future: > 1. Collapse autofill_dates into autofill - did not do it now as table upgrades > run on database initialization, so we need to reduce db size first. > 2. Add |count| to the sync We should Sync (or at least locally persist) the count prior to deleting the data. Once the data is gone, we will not be able to recover the counts.
https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/auto... File chrome/browser/autofill/personal_data_manager.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/auto... chrome/browser/autofill/personal_data_manager.cc:176: wds->RemoveFormElementsAccessedBefore( nit: Can we make this interface simply be "wds->RemoveExpiredFormElements()" and avoid exposing the "AutofillEntry::kExpirationPeriodInDays" constant? https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:116: if (new_synced_entries[i].LastAccessOlder(culling_time)) nit: Can we make this interface be "IsExpired()" and avoid exposing the constant "AutofillEntry::kExpirationPeriodInDays"? https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:76: if (timestamps_.empty() || timestamps_.back() < time) nit: Can we DCHECK(!timestamps_.empty()), or is there a valid reason why we might have an empty vector of timestamps? https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:78: return false; nit: This implementation can be collapsed to "return timestamps_.empty() || timestamps_.back() < time;" https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:107: result->push_back(source.back()); Just to make sure, are the timestamps guaranteed to be sorted? It would be good to at least document that expectation in the method comments (in the header file, where the method is declared). https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:109: VLOG(1) << "Culling timestamps. Current count is : " << source.size(); nit: DVLOG? https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.h (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:60: // Culls the list of timestamps to 2 - the first and last used. This is a nit: Perhaps "first and last used" -> "oldest and most recent"? https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:61: // precursor to getting rid of the timestamps db altogether. If you plan to remove the timestamps db in a separate CL, please file a bug to track that work, so that it doesn't get lost. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry_unittest.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry_unittest.cc:12: const unsigned int kMaxAutofillTimeStamps = 2; nit: Please add a linebreak between this constant declaration and the first test. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:633: "ORDER BY date_created DESC LIMIT 1)")); The nested lookup is a little hard to follow. Did you write it this way for efficiency reasons? If not, I'd prefer to first look up which data we should delete, and then to delete it. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:822: // one. nit: Perhaps we should overwrite it rather than deleting it? (Not too important if we're removing this table soon anyway.) https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/web_data_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/web_data_service.cc:525: } I like exposing this API to clients of the WDS -- or even simplifying it further to RemoveExpiredFormElements(). But, within the WDS implementation, it seems much easier to make this be a simple wrapper around WDS::RemoveFormElementsAddedBetween(). Am I missing a reason why that is a bad idea?
https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/auto... File chrome/browser/autofill/personal_data_manager.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/auto... chrome/browser/autofill/personal_data_manager.cc:176: wds->RemoveFormElementsAccessedBefore( On 2012/03/15 21:00:41, Ilya Sherman wrote: > nit: Can we make this interface simply be "wds->RemoveExpiredFormElements()" and > avoid exposing the "AutofillEntry::kExpirationPeriodInDays" constant? Yes, done https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:116: if (new_synced_entries[i].LastAccessOlder(culling_time)) On 2012/03/15 21:00:41, Ilya Sherman wrote: > nit: Can we make this interface be "IsExpired()" and avoid exposing the constant > "AutofillEntry::kExpirationPeriodInDays"? sure, done https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:76: if (timestamps_.empty() || timestamps_.back() < time) On 2012/03/15 21:00:41, Ilya Sherman wrote: > nit: Can we DCHECK(!timestamps_.empty()), or is there a valid reason why we > might have an empty vector of timestamps? Done. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:78: return false; On 2012/03/15 21:00:41, Ilya Sherman wrote: > nit: This implementation can be collapsed to "return timestamps_.empty() || > timestamps_.back() < time;" Done. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:107: result->push_back(source.back()); On 2012/03/15 21:00:41, Ilya Sherman wrote: > Just to make sure, are the timestamps guaranteed to be sorted? It would be good > to at least document that expectation in the method comments (in the header > file, where the method is declared). Done. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:109: VLOG(1) << "Culling timestamps. Current count is : " << source.size(); On 2012/03/15 21:00:41, Ilya Sherman wrote: > nit: DVLOG? Done. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.h (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:60: // Culls the list of timestamps to 2 - the first and last used. This is a On 2012/03/15 21:00:41, Ilya Sherman wrote: > nit: Perhaps "first and last used" -> "oldest and most recent"? Done. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:61: // precursor to getting rid of the timestamps db altogether. On 2012/03/15 21:00:41, Ilya Sherman wrote: > If you plan to remove the timestamps db in a separate CL, please file a bug to > track that work, so that it doesn't get lost. Done. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry_unittest.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry_unittest.cc:12: const unsigned int kMaxAutofillTimeStamps = 2; On 2012/03/15 21:00:41, Ilya Sherman wrote: > nit: Please add a linebreak between this constant declaration and the first > test. Done. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_entry_unittest.cc:80: // Timestamp is on the edge. This is removed, as it is going to be flaky with the code change (time can advance and item can expire). https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:633: "ORDER BY date_created DESC LIMIT 1)")); On 2012/03/15 21:00:41, Ilya Sherman wrote: > The nested lookup is a little hard to follow. Did you write it this way for > efficiency reasons? If not, I'd prefer to first look up which data we should > delete, and then to delete it. Yes it is for efficiency reasons: doing two SQL statements slower than doing one combined. Added the comment. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:822: // one. On 2012/03/15 21:00:41, Ilya Sherman wrote: > nit: Perhaps we should overwrite it rather than deleting it? (Not too important > if we're removing this table soon anyway.) I am not 100% certain that it will be more efficient - right now items are ordered on the timestamps, so time queries require sequential access to the file. (though I do not know how the indexing is implemented in SQLite - may be sequential does not help much :)) https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/web_data_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/web_data_service.cc:525: } On 2012/03/15 21:00:41, Ilya Sherman wrote: > I like exposing this API to clients of the WDS -- or even simplifying it further > to RemoveExpiredFormElements(). But, within the WDS implementation, it seems > much easier to make this be a simple wrapper around > WDS::RemoveFormElementsAddedBetween(). Am I missing a reason why that is a bad > idea? There are different functions: one removes all time stamps on the interval (t1, t2) and is used from the cache clearing, The other removes 1. all items that were last accessed before some time. Culls the entries to have only two timestamps. After we merge the tables will rid of this function.
Before we can commit this CL, we'll need to store the number of timestamps, at least locally -- otherwise, we won't be able to recover the counts later. https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... File chrome/browser/webdata/web_data_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/26001/chrome/browser/webd... chrome/browser/webdata/web_data_service.cc:525: } On 2012/03/17 00:36:16, GeorgeY wrote: > On 2012/03/15 21:00:41, Ilya Sherman wrote: > > I like exposing this API to clients of the WDS -- or even simplifying it > further > > to RemoveExpiredFormElements(). But, within the WDS implementation, it seems > > much easier to make this be a simple wrapper around > > WDS::RemoveFormElementsAddedBetween(). Am I missing a reason why that is a > bad > > idea? > There are different functions: one removes all time stamps on the interval (t1, > t2) and is used from the cache clearing, > The other removes 1. all items that were last accessed before some time. Culls > the entries to have only two timestamps. After we merge the tables will rid of > this function. Ok, that makes sense. Please add a TODO to refactor away this function after the tables are merged. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:135: if (SyncChange::ACTION_ADD == i->second.first) nit: Please reverse the order of the operands to "==" https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:4: #include "chrome/browser/webdata/autofill_entry.h" nit: Please leave a blank line between the copyright license and the first #include. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:78: return (timestamps_.empty() || timestamps_.back() < time); nit: No need to check timestamps_.empty() given that you DCHECK() non-emptiness immediately above. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:108: } nit: Please leave a blank line after this if-stmt, since it contains a return stmt within it. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.h (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:61: // precursor to getting rid of the timestamps db altogether. (Issue #118696) nit: Please include a link to the issue, i.e. http://crbug.com/118696 https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:71: static const int64 kExpirationPeriodInDays = 60; nit: Let's tuck this into the implementation file rather than the header. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:432: return false; It doesn't look to me like this will always save precisely the oldest and the most recent timestamps. Am I misunderstanding how this works? https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:449: DCHECK(!delete_end.is_null()); nit: Please leave a blank line after this DCHECK https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:451: // were used between the given times. nit: Please update this comment. (At the least, there aren't "given times" that I can easily identify.) https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:463: } nit: Please leave a blank line after this loop https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:465: return false; nit: Please leave a blank line after this if-stmt, and add a brief comment describing the block below. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:471: return false; nit: Please leave a blank line after this if-stmt and add a quick comment describing the block below. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:479: // Cull remaining entries. nit: "entries" -> "entries' timestamps" https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:630: // Inner SELECT selects the oldest |date_created| for a given |pair_id|. oldest or newest? I would have expected us to be removing the newer one... https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.h (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:161: bool RemoveExpiredFormElements(const base::Time& delete_end, nit: I find the fact that we're passing |delete_end| into here surprising. It's always set to AutofillEntry::ExpirationTime(), right? If so, let's remove this parameter and compute it in the implementation.
https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:135: if (SyncChange::ACTION_ADD == i->second.first) On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please reverse the order of the operands to "==" sure https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:4: #include "chrome/browser/webdata/autofill_entry.h" On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please leave a blank line between the copyright license and the first > #include. Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:78: return (timestamps_.empty() || timestamps_.back() < time); On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: No need to check timestamps_.empty() given that you DCHECK() non-emptiness > immediately above. Old implementation *allows* for all of the timestamps deleted (theoretically), but not the new one. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:108: } On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please leave a blank line after this if-stmt, since it contains a return > stmt within it. Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.h (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:61: // precursor to getting rid of the timestamps db altogether. (Issue #118696) On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please include a link to the issue, i.e. http://crbug.com/118696 Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.h:71: static const int64 kExpirationPeriodInDays = 60; On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Let's tuck this into the implementation file rather than the header. Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:432: return false; On 2012/03/19 21:12:51, Ilya Sherman wrote: > It doesn't look to me like this will always save precisely the oldest and the > most recent timestamps. Am I misunderstanding how this works? The initial cleanup will leave only the oldest and the most recent timestamps. I user clears data for last week, for example, it will clean the timestamps for the last week only. if we cleared both timestamps, we need to delete the entry. There is actually one corner case, that I fixed in the new CL. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:449: DCHECK(!delete_end.is_null()); On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please leave a blank line after this DCHECK Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:451: // were used between the given times. On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please update this comment. (At the least, there aren't "given times" that > I can easily identify.) Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:463: } On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please leave a blank line after this loop Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:465: return false; On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please leave a blank line after this if-stmt, and add a brief comment > describing the block below. Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:471: return false; On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: Please leave a blank line after this if-stmt and add a quick comment > describing the block below. Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:479: // Cull remaining entries. On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: "entries" -> "entries' timestamps" Done. https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:630: // Inner SELECT selects the oldest |date_created| for a given |pair_id|. On 2012/03/19 21:12:51, Ilya Sherman wrote: > oldest or newest? I would have expected us to be removing the newer one... Fixed the comment https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.h (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:161: bool RemoveExpiredFormElements(const base::Time& delete_end, On 2012/03/19 21:12:51, Ilya Sherman wrote: > nit: I find the fact that we're passing |delete_end| into here surprising. It's > always set to AutofillEntry::ExpirationTime(), right? If so, let's remove this > parameter and compute it in the implementation. sure
https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:78: return (timestamps_.empty() || timestamps_.back() < time); On 2012/03/20 20:47:58, GeorgeY wrote: > On 2012/03/19 21:12:51, Ilya Sherman wrote: > > nit: No need to check timestamps_.empty() given that you DCHECK() > non-emptiness > > immediately above. > > Old implementation *allows* for all of the timestamps deleted (theoretically), > but not the new one. If so, we shouldn't have a DCHECK() ;) https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:127: // Sync back only the data that appeared after |culling_time|. nit: |culling_time| is no longer a variable defined in this scope https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:422: itr != elements.end(); itr++) { nit: As long as you're editing nearby code... this should probably be "++itr" https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:432: if (how_many == 2 || (how_many == 1 && CountTimestampsData(itr->a) == 0)) { How about just always checking CountTimestampsData() and not worrying about the value of |how_many|? In fact, that seems to be what AddToCountOfFormElement() already does... If we need the special-casing, could you please explain why? https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:455: // were last used before the given time. nit: "the given time" -> "|delete_end|" https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:543: if (!s.Step()) nit: Should this if-stmt include a NOTREACHED()? https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.h (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:158: // |AutofillEntry::ExpirationTime()|. Removes corresponding row from autofill nit: "Removes corresponding row" -> "Removes the corresponding row"; "from autofill table" -> "from the autofill table" https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:177: int CountTimestampsData(int64 pair_id); This seems to be redundant with GetCountOfFormElement()
https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:78: return (timestamps_.empty() || timestamps_.back() < time); On 2012/03/20 21:25:13, Ilya Sherman wrote: > On 2012/03/20 20:47:58, GeorgeY wrote: > > On 2012/03/19 21:12:51, Ilya Sherman wrote: > > > nit: No need to check timestamps_.empty() given that you DCHECK() > > non-emptiness > > > immediately above. > > > > Old implementation *allows* for all of the timestamps deleted (theoretically), > > but not the new one. > > If so, we shouldn't have a DCHECK() ;) OK, removed. Again in the current code it is a valid concurrence. In the new code it is an error. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:127: // Sync back only the data that appeared after |culling_time|. On 2012/03/20 21:25:13, Ilya Sherman wrote: > nit: |culling_time| is no longer a variable defined in this scope Done. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:422: itr != elements.end(); itr++) { On 2012/03/20 21:25:13, Ilya Sherman wrote: > nit: As long as you're editing nearby code... this should probably be "++itr" Done. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:432: if (how_many == 2 || (how_many == 1 && CountTimestampsData(itr->a) == 0)) { On 2012/03/20 21:25:13, Ilya Sherman wrote: > How about just always checking CountTimestampsData() and not worrying about the > value of |how_many|? In fact, that seems to be what AddToCountOfFormElement() > already does... If we need the special-casing, could you please explain why? Different counts. The other count functions are returning/updating |count| property in the autofill table, that can be arbitrary big. CountTimestampsData() on the other hand does a select count(*) on the autofill_dates table to verify if any of the timestamps do remain. As after initial cleanup we store at most two timestamps and we know how many we deleted, it does not make sense to do additional select if we do not need it. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:455: // were last used before the given time. On 2012/03/20 21:25:13, Ilya Sherman wrote: > nit: "the given time" -> "|delete_end|" Done. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:543: if (!s.Step()) On 2012/03/20 21:25:13, Ilya Sherman wrote: > nit: Should this if-stmt include a NOTREACHED()? Done. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.h (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:158: // |AutofillEntry::ExpirationTime()|. Removes corresponding row from autofill On 2012/03/20 21:25:13, Ilya Sherman wrote: > nit: "Removes corresponding row" -> "Removes the corresponding row"; "from > autofill table" -> "from the autofill table" Done. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:177: int CountTimestampsData(int64 pair_id); On 2012/03/20 21:25:13, Ilya Sherman wrote: > This seems to be redundant with GetCountOfFormElement() Nope. Updated comment
https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:78: return (timestamps_.empty() || timestamps_.back() < time); On 2012/03/21 20:56:40, GeorgeY wrote: > On 2012/03/20 21:25:13, Ilya Sherman wrote: > > On 2012/03/20 20:47:58, GeorgeY wrote: > > > On 2012/03/19 21:12:51, Ilya Sherman wrote: > > > > nit: No need to check timestamps_.empty() given that you DCHECK() > > > non-emptiness > > > > immediately above. > > > > > > Old implementation *allows* for all of the timestamps deleted > (theoretically), > > > but not the new one. > > > > If so, we shouldn't have a DCHECK() ;) > > OK, removed. Again in the current code it is a valid concurrence. In the new > code it is an error. nit: If the DCHECK() will be correct in the future, you might want to add a TODO() to add it once the migration happens. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:432: if (how_many == 2 || (how_many == 1 && CountTimestampsData(itr->a) == 0)) { On 2012/03/21 20:56:40, GeorgeY wrote: > On 2012/03/20 21:25:13, Ilya Sherman wrote: > > How about just always checking CountTimestampsData() and not worrying about > the > > value of |how_many|? In fact, that seems to be what AddToCountOfFormElement() > > already does... If we need the special-casing, could you please explain why? > > Different counts. The other count functions are returning/updating |count| > property in the autofill table, that can be arbitrary big. > CountTimestampsData() on the other hand does a select count(*) on the > autofill_dates table to verify if any of the timestamps do remain. As after > initial cleanup we store at most two timestamps and we know how many we deleted, > it does not make sense to do additional select if we do not need it. Ah, ok, I understand what you mean about this being different counts. I do prefer that we always do the additional select, though, rather than relying on |how_many|, because there's a small chance that this method will be called before we've culled the timestamps down to just two. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.h (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:177: int CountTimestampsData(int64 pair_id); On 2012/03/21 20:56:40, GeorgeY wrote: > On 2012/03/20 21:25:13, Ilya Sherman wrote: > > This seems to be redundant with GetCountOfFormElement() > Nope. Updated comment Ah, makes sense -- thanks :) https://chromiumcodereview.appspot.com/9585020/diff/53001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/53001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:437: if (!AddToCountOfFormElement(itr->a, -how_many, &was_removed)) As we discussed offline, I don't think we want to pass in a pointer to |was_removed| to this method anymore, since you're now handling the removal logic inline in this method.
https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... File chrome/browser/webdata/autofill_entry.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/29001/chrome/browser/webd... chrome/browser/webdata/autofill_entry.cc:78: return (timestamps_.empty() || timestamps_.back() < time); On 2012/03/21 21:20:53, Ilya Sherman wrote: > On 2012/03/21 20:56:40, GeorgeY wrote: > > On 2012/03/20 21:25:13, Ilya Sherman wrote: > > > On 2012/03/20 20:47:58, GeorgeY wrote: > > > > On 2012/03/19 21:12:51, Ilya Sherman wrote: > > > > > nit: No need to check timestamps_.empty() given that you DCHECK() > > > > non-emptiness > > > > > immediately above. > > > > > > > > Old implementation *allows* for all of the timestamps deleted > > (theoretically), > > > > but not the new one. > > > > > > If so, we shouldn't have a DCHECK() ;) > > > > OK, removed. Again in the current code it is a valid concurrence. In the new > > code it is an error. > > nit: If the DCHECK() will be correct in the future, you might want to add a > TODO() to add it once the migration happens. Done. https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/41001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:432: if (how_many == 2 || (how_many == 1 && CountTimestampsData(itr->a) == 0)) { On 2012/03/21 21:20:53, Ilya Sherman wrote: > On 2012/03/21 20:56:40, GeorgeY wrote: > > On 2012/03/20 21:25:13, Ilya Sherman wrote: > > > How about just always checking CountTimestampsData() and not worrying about > > the > > > value of |how_many|? In fact, that seems to be what > AddToCountOfFormElement() > > > already does... If we need the special-casing, could you please explain > why? > > > > Different counts. The other count functions are returning/updating |count| > > property in the autofill table, that can be arbitrary big. > > CountTimestampsData() on the other hand does a select count(*) on the > > autofill_dates table to verify if any of the timestamps do remain. As after > > initial cleanup we store at most two timestamps and we know how many we > deleted, > > it does not make sense to do additional select if we do not need it. > > Ah, ok, I understand what you mean about this being different counts. I do > prefer that we always do the additional select, though, rather than relying on > |how_many|, because there's a small chance that this method will be called > before we've culled the timestamps down to just two. Done. https://chromiumcodereview.appspot.com/9585020/diff/53001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/53001/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:437: if (!AddToCountOfFormElement(itr->a, -how_many, &was_removed)) On 2012/03/21 21:20:53, Ilya Sherman wrote: > As we discussed offline, I don't think we want to pass in a pointer to > |was_removed| to this method anymore, since you're now handling the removal > logic inline in this method. Done.
Some comments. http://codereview.chromium.org/9585020/diff/53001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/9585020/diff/53001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:168: // the entries will be culled when sync is connected. I don't think this logic is necessary (see my comment regarding deleting entries in MergeDataAndStartSyncing). http://codereview.chromium.org/9585020/diff/53001/chrome/browser/webdata/auto... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): http://codereview.chromium.org/9585020/diff/53001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:43: for (size_t i = 0; i < timestamps_count; ++i) { I'm worried that (if my understanding is correct) this will create updates to all entries since you're now only storing the first and last timestamps. Some logic here to simply ignore all intervening timestamps would be good. That way, sync entries created previously will retain their current timestamps until the next time they're updated locally. http://codereview.chromium.org/9585020/diff/53001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:118: if (!SaveChangesToWebData(new_synced_entries)) It seems odd to add data to the autofill db if you know you'll delete some of it. Perhaps modify CreateOrUpdateEntry to also check for sync entries that don't exist locally but are expired, and create DELETE events directly within MergeDataAndStartSyncing for those. This will also handle the case where the expiring of local data happens before sync starts up, and I think should simplify some of this logic (I think you should be able to get rid of keys_to_ignore)
https://chromiumcodereview.appspot.com/9585020/diff/56003/chrome/browser/webd... File chrome/browser/webdata/autofill_table.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/56003/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:428: bool was_removed = false; nit: You could save a line by writing this as "bool should_remove = (CountTimestampsData(itr->a) == 0);" ? https://chromiumcodereview.appspot.com/9585020/diff/56003/chrome/browser/webd... chrome/browser/webdata/autofill_table.cc:559: NOTREACHED() << "Should remove the element earlier in the code"; nit: Please move the string "Should remove the element earlier in the code" to be a comment rather than a logged string (saves on binary size). https://chromiumcodereview.appspot.com/9585020/diff/56003/chrome/browser/webd... File chrome/browser/webdata/autofill_table.h (right): https://chromiumcodereview.appspot.com/9585020/diff/56003/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:171: // |delta|. Removes the row from the table and if the count becomes 0. It seems like this method should not be responsible for removing the row from the table, ever -- it should just update the count. Am I missing some reason why that's not so?
Addressed all comments. Sorry for the patchset name - an accident :) http://codereview.chromium.org/9585020/diff/53001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/9585020/diff/53001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:168: // the entries will be culled when sync is connected. On 2012/03/21 22:33:03, nzea wrote: > I don't think this logic is necessary (see my comment regarding deleting entries > in MergeDataAndStartSyncing). No. This is the primary place where we do it: there a lot of people that do not have sync set up, whose autofill db grew out of control http://codereview.chromium.org/9585020/diff/53001/chrome/browser/webdata/auto... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): http://codereview.chromium.org/9585020/diff/53001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:43: for (size_t i = 0; i < timestamps_count; ++i) { On 2012/03/21 22:33:03, nzea wrote: > I'm worried that (if my understanding is correct) this will create updates to > all entries since you're now only storing the first and last timestamps. Some > logic here to simply ignore all intervening timestamps would be good. > > That way, sync entries created previously will retain their current timestamps > until the next time they're updated locally. Updates will be created only for the entries that remain after culling, which should be minority. Most of the extra entries would get "DELETE", which need to happen in any case as otherwise it would continue to be synced. http://codereview.chromium.org/9585020/diff/53001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:118: if (!SaveChangesToWebData(new_synced_entries)) On 2012/03/21 22:33:03, nzea wrote: > It seems odd to add data to the autofill db if you know you'll delete some of > it. Perhaps modify CreateOrUpdateEntry to also check for sync entries that don't > exist locally but are expired, and create DELETE events directly within > MergeDataAndStartSyncing for those. > > This will also handle the case where the expiring of local data happens before > sync starts up, and I think should simplify some of this logic (I think you > should be able to get rid of keys_to_ignore) Change was not as simple :), but I added it - expire autofill data from sync are not written locally anymore. http://codereview.chromium.org/9585020/diff/56003/chrome/browser/webdata/auto... File chrome/browser/webdata/autofill_table.cc (right): http://codereview.chromium.org/9585020/diff/56003/chrome/browser/webdata/auto... chrome/browser/webdata/autofill_table.cc:428: bool was_removed = false; On 2012/03/21 23:43:30, Ilya Sherman wrote: > nit: You could save a line by writing this as "bool should_remove = > (CountTimestampsData(itr->a) == 0);" ? sure http://codereview.chromium.org/9585020/diff/56003/chrome/browser/webdata/auto... chrome/browser/webdata/autofill_table.cc:559: NOTREACHED() << "Should remove the element earlier in the code"; On 2012/03/21 23:43:30, Ilya Sherman wrote: > nit: Please move the string "Should remove the element earlier in the code" to > be a comment rather than a logged string (saves on binary size). Done. http://codereview.chromium.org/9585020/diff/56003/chrome/browser/webdata/auto... File chrome/browser/webdata/autofill_table.h (right): http://codereview.chromium.org/9585020/diff/56003/chrome/browser/webdata/auto... chrome/browser/webdata/autofill_table.h:171: // |delta|. Removes the row from the table and if the count becomes 0. On 2012/03/21 23:43:30, Ilya Sherman wrote: > It seems like this method should not be responsible for removing the row from > the table, ever -- it should just update the count. Am I missing some reason > why that's not so? No. It is in NOTREACHED() land anyway - removed.
LGTM with nits. Thanks :) https://chromiumcodereview.appspot.com/9585020/diff/70001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): https://chromiumcodereview.appspot.com/9585020/diff/70001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:139: // Delete only the changes never synced to the db as they are too old. nit: "synced to" -> "synced to" (extra space) https://chromiumcodereview.appspot.com/9585020/diff/70001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.cc:263: } nit: Up to you, but I think this would be a little clearer if it was written as bool success = web_data_service_->GetDatabase()... DCHECK(success); https://chromiumcodereview.appspot.com/9585020/diff/70001/chrome/browser/webd... File chrome/browser/webdata/autocomplete_syncable_service.h (right): https://chromiumcodereview.appspot.com/9585020/diff/70001/chrome/browser/webd... chrome/browser/webdata/autocomplete_syncable_service.h:100: // |ignored_entries| - entries that came from the sync, but to old to be nit: "to old" -> "too old" https://chromiumcodereview.appspot.com/9585020/diff/70001/chrome/browser/webd... File chrome/browser/webdata/autofill_table.h (right): https://chromiumcodereview.appspot.com/9585020/diff/70001/chrome/browser/webd... chrome/browser/webdata/autofill_table.h:171: // |delta|. nit: Looks like this comment could fit on a single line.
http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:43: for (size_t i = 0; i < timestamps_count; ++i) { This shouldn't union, but instead just check first/last timestamps, and if they're different update them. http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:336: autofill_specifics.usage_timestamp(ts)); this should probably only look at the first and last timestamps as well, since older clients are still going to be creating specifics with all the timestamps.
http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:43: for (size_t i = 0; i < timestamps_count; ++i) { On 2012/03/26 18:23:55, nzea wrote: > This shouldn't union, but instead just check first/last timestamps, and if > they're different update them. Done. http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:139: // Delete only the changes never synced to the db as they are too old. On 2012/03/23 22:44:45, Ilya Sherman wrote: > nit: "synced to" -> "synced to" (extra space) Done. http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:263: } On 2012/03/23 22:44:45, Ilya Sherman wrote: > nit: Up to you, but I think this would be a little clearer if it was written as > > bool success = web_data_service_->GetDatabase()... > DCHECK(success); Done. http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:336: autofill_specifics.usage_timestamp(ts)); On 2012/03/26 18:23:55, nzea wrote: > this should probably only look at the first and last timestamps as well, since > older clients are still going to be creating specifics with all the timestamps. Done. http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... File chrome/browser/webdata/autocomplete_syncable_service.h (right): http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.h:100: // |ignored_entries| - entries that came from the sync, but to old to be On 2012/03/23 22:44:45, Ilya Sherman wrote: > nit: "to old" -> "too old" Done. http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... File chrome/browser/webdata/autofill_table.h (right): http://codereview.chromium.org/9585020/diff/70001/chrome/browser/webdata/auto... chrome/browser/webdata/autofill_table.h:171: // |delta|. On 2012/03/23 22:44:45, Ilya Sherman wrote: > nit: Looks like this comment could fit on a single line. Done.
http://codereview.chromium.org/9585020/diff/79001/chrome/browser/webdata/auto... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): http://codereview.chromium.org/9585020/diff/79001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:62: if (timestamps.front() > time_begin) { I don't think this is quite right. You're not setting different = true if timestamps.front() < time_begin. Also, operator> returns true if lhs is more recent than rhs (it compares microseconds since epoch). So you're updating timestamps[0] only if it's already more recent, when I believe the more recent one is the one you want to keep, right? Also you're modifying timestamps instead of new_timestamps here. In addition, I'd prefer that the count of timestamps not be a consideration when comparing with sync data, given that other clients will continue to track all the timestamps (and the fact that previously this code unioned means they'll continue to add the timestamps back in when they restart sync). how about something like: if (timestamps.front() != sync_time_begin || timestamps.back() != sync_time_end) { new_timestamps.push_back(timestamps.front() > sync_time_begin ? timestamps.front() : sync_time_begin); if (timestamps.size() > 1 || sync_timestamps_count > 1) new_timestamps.push_back(timestamps.back() < sync_time_end ? timestamps.back() : sync_time_end); return true; } return false; This way you only care about the first/last sync timestamps and you return different if they're not already set properly. http://codereview.chromium.org/9585020/diff/79001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:381: *(it->second.second) = new_entry; This code has no effect if the entry is expired right? Perhaps move into the else case of the if statement above?
http://codereview.chromium.org/9585020/diff/79001/chrome/browser/webdata/auto... File chrome/browser/webdata/autocomplete_syncable_service.cc (right): http://codereview.chromium.org/9585020/diff/79001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:62: if (timestamps.front() > time_begin) { On 2012/03/26 20:58:22, nzea wrote: > I don't think this is quite right. You're not setting different = true if > timestamps.front() < time_begin. Also, operator> returns true if lhs is more > recent than rhs (it compares microseconds since epoch). So you're updating > timestamps[0] only if it's already more recent, when I believe the more recent > one is the one you want to keep, right? Also you're modifying timestamps instead > of new_timestamps here. > > In addition, I'd prefer that the count of timestamps not be a consideration when > comparing with sync data, given that other clients will continue to track all > the timestamps (and the fact that previously this code unioned means they'll > continue to add the timestamps back in when they restart sync). > > how about something like: > if (timestamps.front() != sync_time_begin || timestamps.back() != sync_time_end) > { > new_timestamps.push_back(timestamps.front() > sync_time_begin ? > timestamps.front() : sync_time_begin); > if (timestamps.size() > 1 || sync_timestamps_count > 1) > new_timestamps.push_back(timestamps.back() < sync_time_end ? > timestamps.back() : sync_time_end); > return true; > } > return false; > > This way you only care about the first/last sync timestamps and you return > different if they're not already set properly. sure. works as well, though slightly modified (your code does not work if both have only one different timestamp, for example) http://codereview.chromium.org/9585020/diff/79001/chrome/browser/webdata/auto... chrome/browser/webdata/autocomplete_syncable_service.cc:381: *(it->second.second) = new_entry; On 2012/03/26 20:58:22, nzea wrote: > This code has no effect if the entry is expired right? Perhaps move into the > else case of the if statement above? Sure. It will get deleted in any case.
LGTM, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/georgey@chromium.org/9585020/87002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/georgey@chromium.org/9585020/87002
Change committed as 129097 |
