Single URL Expires Were Not Being Deleted.

Single URL Expires Were Not Being Deleted.

Some notification clients expect URLs while others expect URLRows but in some cases the URLRows were not being provided. Eliminated the duplication of data structures by removing the set of URLs and relying only on the vector of URLRows.

tim@ for OWNERS review of sync/...

BUG=122740
TEST=Added unit test.
TBR=tim@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=132268

[mrossetti, 2012-04-10 23:21:51]

Please, whichever OWNER gets a few moments please take a look.

Note: This is the simple fix — carrying around both a vector or URLRows and a
set of GURLs. Some clients of the NOTIFICATION_HISTORY_URLS_DELETED notification
want the URLRow and some prefer the set of GURLs. This is redundant. I _could_
update all of the clients that current use a set of GURLs to use the vector of
URLRows instead. Doing this would require changing 14 source files (and a few
headers).

The unfortunate thing is that only one NOTIFICATION_HISTORY_URLS_DELETED
notification client needs the rows, or rather the history ID: the
InMemoryHistoryBackend. Not having written this code, I am assuming that the
reason for this is to eliminate the need for the InMemoryHistoryBackend from
having to perform a number of complicated and potentially risky database queries
based on URL.

[mrossetti, 2012-04-12 20:49:22]

A ping on this as it represents a privacy issue, minor, but still...

[sky, 2012-04-12 20:54:10]

http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
File chrome/browser/history/expire_history_backend.cc (right):

http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
chrome/browser/history/expire_history_backend.cc:362: deleted_details->rows =
dependencies->deleted_urls;
Why do we need both? If we're providing rows can't we nuked dleeted_urls since
rows includes the url?

[brettw, 2012-04-12 20:54:10]

Send reviews to one person. If it's not clear I must do something, I archive the
mail. Spamming multiple people will generally make your review take longer and
wastes everybody's time with more email.

[mrossetti, 2012-04-12 20:56:00]

On 2012/04/12 20:54:10, brettw wrote:
> Send reviews to one person. If it's not clear I must do something, I archive
the
> mail. Spamming multiple people will generally make your review take longer and
> wastes everybody's time with more email.

Sorry for the 'spam' brettw. Thanks for the counsel.

[mrossetti, 2012-04-12 21:19:35]

On 2012/04/12 20:54:10, sky wrote:
>
http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
> File chrome/browser/history/expire_history_backend.cc (right):
> 
>
http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
> chrome/browser/history/expire_history_backend.cc:362: deleted_details->rows =
> dependencies->deleted_urls;
> Why do we need both? If we're providing rows can't we nuked dleeted_urls since
> rows includes the url?

On 2012/04/12 20:54:10, sky wrote:
>
http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
> File chrome/browser/history/expire_history_backend.cc (right):
> 
>
http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
> chrome/browser/history/expire_history_backend.cc:362: deleted_details->rows =
> dependencies->deleted_urls;
> Why do we need both? If we're providing rows can't we nuked dleeted_urls since
> rows includes the url?

I'm glad you asked as I'm somewhat, but not totally, ambivalent. There are 14
source files and 2 or 3 headers that would have to be changed. Specific
functions requiring changes listed below (mostly for my benefit). The breadth of
the changes (16 files vs. 2) plus a very minor performance hit (very minor) was
the catalyst for my original comment where I described my conundrum. 

If you prefer that I ripple the |rows| usage throughout and remove |urls| I'd be
perfectly happy to do that. Just give the word.

Mike

- AndroidProviderBackend::DeleteHistoryInternal would need very minor changes.
- AndroidProviderBackend::SimulateUpdateURL would need very minor changes
- AndroidProviderBackendTest.DeleteHistoryAndBookmarks would need very minor
changes.
- BrowsingHistoryHandler::Observe uses |urls| and <set> equality but changing
that would be simple.
- HistoryExtensionEventRouter::HistoryUrlsRemoved uses |urls| but could easily
be converted.
- InMemoryHistoryBackend::OnURLsDeleted already uses |rows|.
- InMemoryURLIndex::OnURLsDeleted already uses |rows|.
- NetworkActionPredictor::DeleteRowsWithURLs uses |urls| and <set>::find but
changing that would be moderate, requiring a header change.
- ProfileSyncServiceTypedUrlTest.ProcessUserChangeRemove would need minor
changes.
- ShortcutsBackend::Observe uses |urls| and <set>::find but changing that would
be straightforward.
- TopSites::Observe uses |urls| but could easily be converted.
- TypedUrlChangeProcessor::HandleURLsDeleted uses |urls| but could easily be
converted.
- VisitedLinkMaster::DeleteURLs uses the |urls| set but could easily be
converted.

[sky, 2012-04-12 21:32:12]

On Thu, Apr 12, 2012 at 2:19 PM,  <mrossetti@chromium.org> wrote:
> On 2012/04/12 20:54:10, sky wrote:
>
>
http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
>>
>> File chrome/browser/history/expire_history_backend.cc (right):
>
>
>
>
http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
>>
>> chrome/browser/history/expire_history_backend.cc:362:
>> deleted_details->rows =
>> dependencies->deleted_urls;
>> Why do we need both? If we're providing rows can't we nuked dleeted_urls
>> since
>> rows includes the url?
>
>
> On 2012/04/12 20:54:10, sky wrote:
>
>
http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
>>
>> File chrome/browser/history/expire_history_backend.cc (right):
>
>
>
>
http://codereview.chromium.org/10031028/diff/2002/chrome/browser/history/expi...
>>
>> chrome/browser/history/expire_history_backend.cc:362:
>> deleted_details->rows =
>> dependencies->deleted_urls;
>> Why do we need both? If we're providing rows can't we nuked dleeted_urls
>> since
>> rows includes the url?
>
>
> I'm glad you asked as I'm somewhat, but not totally, ambivalent. There are
> 14
> source files and 2 or 3 headers that would have to be changed. Specific
> functions requiring changes listed below (mostly for my benefit). The
> breadth of
> the changes (16 files vs. 2) plus a very minor performance hit (very minor)
> was
> the catalyst for my original comment where I described my conundrum.
>
> If you prefer that I ripple the |rows| usage throughout and remove |urls|
> I'd be
> perfectly happy to do that. Just give the word.

Yes please. If there's no reason to provide both, remove urls.

  -Scott

> Mike
>
> - AndroidProviderBackend::DeleteHistoryInternal would need very minor
> changes.
> - AndroidProviderBackend::SimulateUpdateURL would need very minor changes
> - AndroidProviderBackendTest.DeleteHistoryAndBookmarks would need very minor
> changes.
> - BrowsingHistoryHandler::Observe uses |urls| and <set> equality but
> changing
> that would be simple.
> - HistoryExtensionEventRouter::HistoryUrlsRemoved uses |urls| but could
> easily
> be converted.
> - InMemoryHistoryBackend::OnURLsDeleted already uses |rows|.
> - InMemoryURLIndex::OnURLsDeleted already uses |rows|.
> - NetworkActionPredictor::DeleteRowsWithURLs uses |urls| and <set>::find but
> changing that would be moderate, requiring a header change.
> - ProfileSyncServiceTypedUrlTest.ProcessUserChangeRemove would need minor
> changes.
> - ShortcutsBackend::Observe uses |urls| and <set>::find but changing that
> would
> be straightforward.
> - TopSites::Observe uses |urls| but could easily be converted.
> - TypedUrlChangeProcessor::HandleURLsDeleted uses |urls| but could easily be
> converted.
> - VisitedLinkMaster::DeleteURLs uses the |urls| set but could easily be
> converted.
>
>
> http://codereview.chromium.org/10031028/

[mrossetti, 2012-04-13 16:33:57]

I've eliminated the duplicate change data by removing URLsDeletedDetails.urls
and relying solely on URLsDeletedDetails.rows for the list of deleted URLs.

[sky, 2012-04-13 16:54:01]

LGTM

https://chromiumcodereview.appspot.com/10031028/diff/12001/chrome/browser/his...
File chrome/browser/history/history_types.h (right):

https://chromiumcodereview.appspot.com/10031028/diff/12001/chrome/browser/his...
chrome/browser/history/history_types.h:138: explicit URLRowHasURL(const GURL&
url): url_(url) {}
nit: '):' -> ') :'

https://chromiumcodereview.appspot.com/10031028/diff/12001/chrome/browser/his...
chrome/browser/history/history_types.h:146: };
DISALLOW_COPY_AND_ASSIGN

[mrossetti, 2012-04-13 18:19:19]

Updated. Will commit once bots smile.

http://codereview.chromium.org/10031028/diff/12001/chrome/browser/history/his...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/10031028/diff/12001/chrome/browser/history/his...
chrome/browser/history/history_types.h:138: explicit URLRowHasURL(const GURL&
url): url_(url) {}
On 2012/04/13 16:54:02, sky wrote:
> nit: '):' -> ') :'

Done.

http://codereview.chromium.org/10031028/diff/12001/chrome/browser/history/his...
chrome/browser/history/history_types.h:146: };
On 2012/04/13 16:54:02, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Functors need to be copyable.