Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1901563002:
Set site engagement timestamps to privacy-respectful values when history is cleared. (Closed)
DescriptionSet site engagement timestamps to privacy-respectful values when history is cleared.
The site engagement service stores last updated and last shortcut launch
timestamps per origin. When history is cleared, the former is set to the
time of history clearing and the latter is not changed.
This CL sets both timestamps to the time of the most recent remaining
visit in history for that origin once history clearing is completed. The
last shortcut launch time is only set if it was non-null prior to the
history being cleared.
A slight change to the history service query (adding the last visited date
to the data returned) is necessary to enable this change. More
comprehensive testing of the history expiry mechanism are added to
ensure correctness.
BUG=604284
Committed: https://crrev.com/3d900e9e2c2dc6cda11c94aea9848181a9b4ffd1
Cr-Commit-Position: refs/heads/master@{#388321}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing reviewer comments #Patch Set 3 : Modify history service to return last visited time #Patch Set 4 : Rebase #
Total comments: 8
Patch Set 5 : Addressing reviewer comments #Patch Set 6 : scoped_ptr is now unique_ptr #Messages
Total messages: 26 (8 generated)
dominickn@chromium.org changed reviewers: + benwells@chromium.org
PTAL, thanks!
https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:275: if (updated_time == nullptr) { Nit: Can you rearrange to be if (updated_time) { } else{ }? https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:453: void SiteEngagementService::ResetScoreForURL(const GURL& url, double score) { Is this still used? https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:751: updated_time = clock_->Now(); IIUC this will be fairly common: - usually people don't just visit the origin but visit some URL under the origin, as noted in the TODO - I think only a short amound of history is generally present. Is that right? https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:764: const GURL& url, double score, const base::Time* updated_time) { Should this score by undecayed? That is, what happens if the updated time was a long time ago - won't the score be decayed next time its read? https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.h:110: // shortcut launch time (if it is non-null) to |updated_time|. Othewise, last Typo in Othewise. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.h:113: // TODO(calamity): Ideally, all SiteEngagementScore methods should take a Could you file a bug and refer to it here?
This is going to get more complicated unfortunately. HistoryService:::GetLastVisitTimeForURL queries an in-memory database, not all of history. It only looks at items which have been searched for/typed on the history page. Hence the currently failing history expiry test; the last updated date keeps getting set to 4 weeks ago because the method isn't returning any dates. If I want to use the query-based approach, it looks like I have to use HistoryService::QueryHistory, which is asynchronous, and returns a vector of results. I'll basically need to query everything in history, and process the result to compute the last visit per origin. Our choices for M51 are: a) use QueryHistory. Not too efficient, and will need more processing code in site engagement service. However, it shouldn't need changes to the history service. b) modify the method calamity@ added to the history service to return the last visit time as well as the count of URLs. Needs a minor change to the history service, but should be fairly efficient. c) nuke everything when history is cleared Thoughts? https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:275: if (updated_time == nullptr) { On 2016/04/18 07:01:42, benwells wrote: > Nit: Can you rearrange to be > > if (updated_time) { > } else{ > }? Done. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:453: void SiteEngagementService::ResetScoreForURL(const GURL& url, double score) { On 2016/04/18 07:01:42, benwells wrote: > Is this still used? Yes, in the WebUI code for changing engagement scores. I disliked the idea of exposing an internal site engagement detail (last updated times) to the public interface, which is why I had the public method without the time detail. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:751: updated_time = clock_->Now(); On 2016/04/18 07:01:42, benwells wrote: > IIUC this will be fairly common: > - usually people don't just visit the origin but visit some URL under the > origin, as noted in the TODO > - I think only a short amound of history is generally present. > > Is that right? Yes, I agree with you here. As it turns out, HistoryService::GetLastVisitTimeForURL doesn't seem to work. The failing tests are because this method returns blank for everything, even origins which are in the history. After a lot of digging, I think it's because GetLastVisitTimeForURL queries an in-memory cache which holds only part of the history information - crucially, only URLs which have been searched for/typed will be returned here. This means I'll need to use a different method here, using a callback. Sigh. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:764: const GURL& url, double score, const base::Time* updated_time) { On 2016/04/18 07:01:42, benwells wrote: > Should this score by undecayed? That is, what happens if the updated time was a > long time ago - won't the score be decayed next time its read? Having thought about this more, it seems counter-intuitive that *removing* history would lead to an *increase* in engagement, which is what undecaying would do as I understand it. Plus, the way the procedure currently works, the next time the score is read, it would be decayed straight back down to what the score was before deletion started. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.h:110: // shortcut launch time (if it is non-null) to |updated_time|. Othewise, last On 2016/04/18 07:01:42, benwells wrote: > Typo in Othewise. Done. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.h:113: // TODO(calamity): Ideally, all SiteEngagementScore methods should take a On 2016/04/18 07:01:42, benwells wrote: > Could you file a bug and refer to it here? Done.
On 2016/04/18 23:27:52, dominickn wrote: > This is going to get more complicated unfortunately. > > HistoryService:::GetLastVisitTimeForURL queries an in-memory database, not all > of history. It only looks at items which have been searched for/typed on the > history page. Hence the currently failing history expiry test; the last updated > date keeps getting set to 4 weeks ago because the method isn't returning any > dates. > > If I want to use the query-based approach, it looks like I have to use > HistoryService::QueryHistory, which is asynchronous, and returns a vector of > results. I'll basically need to query everything in history, and process the > result to compute the last visit per origin. > > Our choices for M51 are: > > a) use QueryHistory. Not too efficient, and will need more processing code in > site engagement service. However, it shouldn't need changes to the history > service. > > b) modify the method calamity@ added to the history service to return the last > visit time as well as the count of URLs. Needs a minor change to the history > service, but should be fairly efficient. > > c) nuke everything when history is cleared > > > Thoughts? > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > File chrome/browser/engagement/site_engagement_service.cc (right): > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > chrome/browser/engagement/site_engagement_service.cc:275: if (updated_time == > nullptr) { > On 2016/04/18 07:01:42, benwells wrote: > > Nit: Can you rearrange to be > > > > if (updated_time) { > > } else{ > > }? > > Done. > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > chrome/browser/engagement/site_engagement_service.cc:453: void > SiteEngagementService::ResetScoreForURL(const GURL& url, double score) { > On 2016/04/18 07:01:42, benwells wrote: > > Is this still used? > > Yes, in the WebUI code for changing engagement scores. I disliked the idea of > exposing an internal site engagement detail (last updated times) to the public > interface, which is why I had the public method without the time detail. > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > chrome/browser/engagement/site_engagement_service.cc:751: updated_time = > clock_->Now(); > On 2016/04/18 07:01:42, benwells wrote: > > IIUC this will be fairly common: > > - usually people don't just visit the origin but visit some URL under the > > origin, as noted in the TODO > > - I think only a short amound of history is generally present. > > > > Is that right? > > Yes, I agree with you here. > > As it turns out, HistoryService::GetLastVisitTimeForURL doesn't seem to work. > The failing tests are because this method returns blank for everything, even > origins which are in the history. After a lot of digging, I think it's because > GetLastVisitTimeForURL queries an in-memory cache which holds only part of the > history information - crucially, only URLs which have been searched for/typed > will be returned here. > > This means I'll need to use a different method here, using a callback. Sigh. > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > chrome/browser/engagement/site_engagement_service.cc:764: const GURL& url, > double score, const base::Time* updated_time) { > On 2016/04/18 07:01:42, benwells wrote: > > Should this score by undecayed? That is, what happens if the updated time was > a > > long time ago - won't the score be decayed next time its read? > > Having thought about this more, it seems counter-intuitive that *removing* > history would lead to an *increase* in engagement, which is what undecaying > would do as I understand it. Plus, the way the procedure currently works, the > next time the score is read, it would be decayed straight back down to what the > score was before deletion started. > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > File chrome/browser/engagement/site_engagement_service.h (right): > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > chrome/browser/engagement/site_engagement_service.h:110: // shortcut launch time > (if it is non-null) to |updated_time|. Othewise, last > On 2016/04/18 07:01:42, benwells wrote: > > Typo in Othewise. > > Done. > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > chrome/browser/engagement/site_engagement_service.h:113: // TODO(calamity): > Ideally, all SiteEngagementScore methods should take a > On 2016/04/18 07:01:42, benwells wrote: > > Could you file a bug and refer to it here? > > Done. For the record, my preference is option b) (change the method in history service), because this will require the least amount of changes for M52 and beyond. The change will be very minor in history service, and I'd rather not introduce the poor solution (QueryHistory) or delete everything just for the sake of M51.
On 2016/04/18 23:37:29, dominickn wrote: > On 2016/04/18 23:27:52, dominickn wrote: > > This is going to get more complicated unfortunately. > > > > HistoryService:::GetLastVisitTimeForURL queries an in-memory database, not all > > of history. It only looks at items which have been searched for/typed on the > > history page. Hence the currently failing history expiry test; the last > updated > > date keeps getting set to 4 weeks ago because the method isn't returning any > > dates. Does that mean the existing code which updates the engagement to be a proportion based on remaining history is basically broken? > > > > If I want to use the query-based approach, it looks like I have to use > > HistoryService::QueryHistory, which is asynchronous, and returns a vector of > > results. I'll basically need to query everything in history, and process the > > result to compute the last visit per origin. > > > > Our choices for M51 are: > > > > a) use QueryHistory. Not too efficient, and will need more processing code in > > site engagement service. However, it shouldn't need changes to the history > > service. > > > > b) modify the method calamity@ added to the history service to return the last > > visit time as well as the count of URLs. Needs a minor change to the history > > service, but should be fairly efficient. > > > > c) nuke everything when history is cleared > > > > > > Thoughts? > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > File chrome/browser/engagement/site_engagement_service.cc (right): > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > chrome/browser/engagement/site_engagement_service.cc:275: if (updated_time == > > nullptr) { > > On 2016/04/18 07:01:42, benwells wrote: > > > Nit: Can you rearrange to be > > > > > > if (updated_time) { > > > } else{ > > > }? > > > > Done. > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > chrome/browser/engagement/site_engagement_service.cc:453: void > > SiteEngagementService::ResetScoreForURL(const GURL& url, double score) { > > On 2016/04/18 07:01:42, benwells wrote: > > > Is this still used? > > > > Yes, in the WebUI code for changing engagement scores. I disliked the idea of > > exposing an internal site engagement detail (last updated times) to the public > > interface, which is why I had the public method without the time detail. > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > chrome/browser/engagement/site_engagement_service.cc:751: updated_time = > > clock_->Now(); > > On 2016/04/18 07:01:42, benwells wrote: > > > IIUC this will be fairly common: > > > - usually people don't just visit the origin but visit some URL under the > > > origin, as noted in the TODO > > > - I think only a short amound of history is generally present. > > > > > > Is that right? > > > > Yes, I agree with you here. > > > > As it turns out, HistoryService::GetLastVisitTimeForURL doesn't seem to work. > > The failing tests are because this method returns blank for everything, even > > origins which are in the history. After a lot of digging, I think it's because > > GetLastVisitTimeForURL queries an in-memory cache which holds only part of the > > history information - crucially, only URLs which have been searched for/typed > > will be returned here. > > > > This means I'll need to use a different method here, using a callback. Sigh. > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > chrome/browser/engagement/site_engagement_service.cc:764: const GURL& url, > > double score, const base::Time* updated_time) { > > On 2016/04/18 07:01:42, benwells wrote: > > > Should this score by undecayed? That is, what happens if the updated time > was > > a > > > long time ago - won't the score be decayed next time its read? > > > > Having thought about this more, it seems counter-intuitive that *removing* > > history would lead to an *increase* in engagement, which is what undecaying > > would do as I understand it. Plus, the way the procedure currently works, the > > next time the score is read, it would be decayed straight back down to what > the > > score was before deletion started. > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > File chrome/browser/engagement/site_engagement_service.h (right): > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > chrome/browser/engagement/site_engagement_service.h:110: // shortcut launch > time > > (if it is non-null) to |updated_time|. Othewise, last > > On 2016/04/18 07:01:42, benwells wrote: > > > Typo in Othewise. > > > > Done. > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > chrome/browser/engagement/site_engagement_service.h:113: // TODO(calamity): > > Ideally, all SiteEngagementScore methods should take a > > On 2016/04/18 07:01:42, benwells wrote: > > > Could you file a bug and refer to it here? > > > > Done. > > For the record, my preference is option b) (change the method in history > service), because this will require the least amount of changes for M52 and > beyond. The change will be very minor in history service, and I'd rather not > introduce the poor solution (QueryHistory) or delete everything just for the > sake of M51.
On 2016/04/19 00:12:41, benwells wrote: > On 2016/04/18 23:37:29, dominickn wrote: > > On 2016/04/18 23:27:52, dominickn wrote: > > > This is going to get more complicated unfortunately. > > > > > > HistoryService:::GetLastVisitTimeForURL queries an in-memory database, not > all > > > of history. It only looks at items which have been searched for/typed on the > > > history page. Hence the currently failing history expiry test; the last > > updated > > > date keeps getting set to 4 weeks ago because the method isn't returning any > > > dates. > > Does that mean the existing code which updates the engagement to be a proportion > based on remaining history is basically broken? Answering my own question: no, I misunderstood. That code uses the asynch approach so shouldn't have any problems. > > > > > > > If I want to use the query-based approach, it looks like I have to use > > > HistoryService::QueryHistory, which is asynchronous, and returns a vector of > > > results. I'll basically need to query everything in history, and process the > > > result to compute the last visit per origin. > > > > > > Our choices for M51 are: > > > > > > a) use QueryHistory. Not too efficient, and will need more processing code > in > > > site engagement service. However, it shouldn't need changes to the history > > > service. > > > > > > b) modify the method calamity@ added to the history service to return the > last > > > visit time as well as the count of URLs. Needs a minor change to the history > > > service, but should be fairly efficient. > > > > > > c) nuke everything when history is cleared Or d) always set the date back to 4 weeks earlier regardless of history. I'd be happy with either b) or d). > > > > > > > > > Thoughts? > > > > > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > > File chrome/browser/engagement/site_engagement_service.cc (right): > > > > > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > > chrome/browser/engagement/site_engagement_service.cc:275: if (updated_time > == > > > nullptr) { > > > On 2016/04/18 07:01:42, benwells wrote: > > > > Nit: Can you rearrange to be > > > > > > > > if (updated_time) { > > > > } else{ > > > > }? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > > chrome/browser/engagement/site_engagement_service.cc:453: void > > > SiteEngagementService::ResetScoreForURL(const GURL& url, double score) { > > > On 2016/04/18 07:01:42, benwells wrote: > > > > Is this still used? > > > > > > Yes, in the WebUI code for changing engagement scores. I disliked the idea > of > > > exposing an internal site engagement detail (last updated times) to the > public > > > interface, which is why I had the public method without the time detail. > > > > > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > > chrome/browser/engagement/site_engagement_service.cc:751: updated_time = > > > clock_->Now(); > > > On 2016/04/18 07:01:42, benwells wrote: > > > > IIUC this will be fairly common: > > > > - usually people don't just visit the origin but visit some URL under the > > > > origin, as noted in the TODO > > > > - I think only a short amound of history is generally present. > > > > > > > > Is that right? > > > > > > Yes, I agree with you here. > > > > > > As it turns out, HistoryService::GetLastVisitTimeForURL doesn't seem to > work. > > > The failing tests are because this method returns blank for everything, even > > > origins which are in the history. After a lot of digging, I think it's > because > > > GetLastVisitTimeForURL queries an in-memory cache which holds only part of > the > > > history information - crucially, only URLs which have been searched > for/typed > > > will be returned here. > > > > > > This means I'll need to use a different method here, using a callback. Sigh. > > > > > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > > chrome/browser/engagement/site_engagement_service.cc:764: const GURL& url, > > > double score, const base::Time* updated_time) { > > > On 2016/04/18 07:01:42, benwells wrote: > > > > Should this score by undecayed? That is, what happens if the updated time > > was > > > a > > > > long time ago - won't the score be decayed next time its read? > > > > > > Having thought about this more, it seems counter-intuitive that *removing* > > > history would lead to an *increase* in engagement, which is what undecaying > > > would do as I understand it. Plus, the way the procedure currently works, > the > > > next time the score is read, it would be decayed straight back down to what > > the > > > score was before deletion started. > > > > > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > > File chrome/browser/engagement/site_engagement_service.h (right): > > > > > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > > chrome/browser/engagement/site_engagement_service.h:110: // shortcut launch > > time > > > (if it is non-null) to |updated_time|. Othewise, last > > > On 2016/04/18 07:01:42, benwells wrote: > > > > Typo in Othewise. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... > > > chrome/browser/engagement/site_engagement_service.h:113: // TODO(calamity): > > > Ideally, all SiteEngagementScore methods should take a > > > On 2016/04/18 07:01:42, benwells wrote: > > > > Could you file a bug and refer to it here? > > > > > > Done. > > > > For the record, my preference is option b) (change the method in history > > service), because this will require the least amount of changes for M52 and > > beyond. The change will be very minor in history service, and I'd rather not > > introduce the poor solution (QueryHistory) or delete everything just for the > > sake of M51.
On 2016/04/19 00:12:41, benwells wrote: > On 2016/04/18 23:37:29, dominickn wrote: > > On 2016/04/18 23:27:52, dominickn wrote: > > > This is going to get more complicated unfortunately. > > > > > > HistoryService:::GetLastVisitTimeForURL queries an in-memory database, not > all > > > of history. It only looks at items which have been searched for/typed on the > > > history page. Hence the currently failing history expiry test; the last > > updated > > > date keeps getting set to 4 weeks ago because the method isn't returning any > > > dates. > > Does that mean the existing code which updates the engagement to be a proportion > based on remaining history is basically broken? No, that works fine because it queries the backend history service, not the in-memory database. My proposed method b) would be to make the proportion code also calculate the last visit time. It's already having to iterate through everything in history, so doing a visit time lookup is negligible.
https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:751: updated_time = clock_->Now(); On 2016/04/18 23:27:52, dominickn wrote: > On 2016/04/18 07:01:42, benwells wrote: > > IIUC this will be fairly common: > > - usually people don't just visit the origin but visit some URL under the > > origin, as noted in the TODO > > - I think only a short amound of history is generally present. > > > > Is that right? > > Yes, I agree with you here. > > As it turns out, HistoryService::GetLastVisitTimeForURL doesn't seem to work. > The failing tests are because this method returns blank for everything, even > origins which are in the history. After a lot of digging, I think it's because > GetLastVisitTimeForURL queries an in-memory cache which holds only part of the > history information - crucially, only URLs which have been searched for/typed > will be returned here. > > This means I'll need to use a different method here, using a callback. Sigh. Ack. Can you also move this code to below where proportion_remaining is set? That keeps the code that finds the proportion all together. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:764: const GURL& url, double score, const base::Time* updated_time) { On 2016/04/18 23:27:52, dominickn wrote: > On 2016/04/18 07:01:42, benwells wrote: > > Should this score by undecayed? That is, what happens if the updated time was > a > > long time ago - won't the score be decayed next time its read? > > Having thought about this more, it seems counter-intuitive that *removing* > history would lead to an *increase* in engagement, which is what undecaying > would do as I understand it. Plus, the way the procedure currently works, the > next time the score is read, it would be decayed straight back down to what the > score was before deletion started. Undecaying should not lead to any increase in engagement in today's values. And yes, it would be decayed straight back down, that's the point. But it wouldn't be decayed back down to what the score was before the deletion started, it would be decayed back down to the new calculated proportional score.
Description was changed from ========== Set site engagement timestamps to privacy-respectful values when history is cleared. The site engagement service stores last updated and last shortcut launch timestamps per origin. When history is cleared, the former is set to the time of history clearing and the latter is not changed. This CL sets both timestamps to the time of the most recent remaining visit in history for that origin once history clearing is completed. The last shortcut launch time is only set if it was non-null prior to the history being cleared. Additional tests are added for the low-level reset. BUG=604284 ========== to ========== Set site engagement timestamps to privacy-respectful values when history is cleared. The site engagement service stores last updated and last shortcut launch timestamps per origin. When history is cleared, the former is set to the time of history clearing and the latter is not changed. This CL sets both timestamps to the time of the most recent remaining visit in history for that origin once history clearing is completed. The last shortcut launch time is only set if it was non-null prior to the history being cleared. A slight change to the history service query (adding the last visited date to the data returned) is necessary to enable this change. More comprehensive testing of the history expiry mechanism are added to ensure correctness. BUG=604284 ==========
dominickn@chromium.org changed reviewers: + sdefresne@chromium.org
@benwells: PTAL at site engagement. This should be much neater now. @sdefresne: PTAL at history, thanks! This CL is to be merged to M51 at the request of privacy. The bulk of changes to history are making tests more robust, with a minor rename and change to a method. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:751: updated_time = clock_->Now(); On 2016/04/19 00:18:53, benwells wrote: > On 2016/04/18 23:27:52, dominickn wrote: > > On 2016/04/18 07:01:42, benwells wrote: > > > IIUC this will be fairly common: > > > - usually people don't just visit the origin but visit some URL under the > > > origin, as noted in the TODO > > > - I think only a short amound of history is generally present. > > > > > > Is that right? > > > > Yes, I agree with you here. > > > > As it turns out, HistoryService::GetLastVisitTimeForURL doesn't seem to work. > > The failing tests are because this method returns blank for everything, even > > origins which are in the history. After a lot of digging, I think it's because > > GetLastVisitTimeForURL queries an in-memory cache which holds only part of the > > history information - crucially, only URLs which have been searched for/typed > > will be returned here. > > > > This means I'll need to use a different method here, using a callback. Sigh. > > Ack. > > Can you also move this code to below where proportion_remaining is set? That > keeps the code that finds the proportion all together. Done. https://codereview.chromium.org/1901563002/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_service.cc:764: const GURL& url, double score, const base::Time* updated_time) { On 2016/04/19 00:18:53, benwells wrote: > On 2016/04/18 23:27:52, dominickn wrote: > > On 2016/04/18 07:01:42, benwells wrote: > > > Should this score by undecayed? That is, what happens if the updated time > was > > a > > > long time ago - won't the score be decayed next time its read? > > > > Having thought about this more, it seems counter-intuitive that *removing* > > history would lead to an *increase* in engagement, which is what undecaying > > would do as I understand it. Plus, the way the procedure currently works, the > > next time the score is read, it would be decayed straight back down to what > the > > score was before deletion started. > > Undecaying should not lead to any increase in engagement in today's values. > > And yes, it would be decayed straight back down, that's the point. But it > wouldn't be decayed back down to what the score was before the deletion started, > it would be decayed back down to the new calculated proportional score. I finally understand what's going on here. Done, with tests modified to ensure the right thing is going on.
https://codereview.chromium.org/1901563002/diff/60001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/1901563002/diff/60001/components/history/core... components/history/core/browser/history_backend.cc:443: if (ContainsValue(origins, origin)) { nit: I would change to this instead to avoid doing two lookups in the map: auto iter = origins.find(origin); if (iter != origins.end()) { std::pair<int, base::Time>& value = *iter; ... } https://codereview.chromium.org/1901563002/diff/60001/components/history/core... File components/history/core/browser/history_types.h (right): https://codereview.chromium.org/1901563002/diff/60001/components/history/core... components/history/core/browser/history_types.h:464: typedef std::map<GURL, std::pair<int, base::Time>> OriginCountMap; OriginCountAndLastVisitedMap?
overall lg, but i'm concerned about the dcheck. https://codereview.chromium.org/1901563002/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1901563002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:783: static_cast<double>(remaining) / (remaining + deleted); Nit: move proportion_remaining calculation up to line 759, so that all the code to figure that out is together. https://codereview.chromium.org/1901563002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:794: DCHECK_LE(score, SiteEngagementScore::kMaxPoints); Should it be impossible for this DCHECK to fire? I'm not sure it is. You might undecay it back up to beyone max points, in which case you want to cap it at max points.
Thanks! https://codereview.chromium.org/1901563002/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service.cc (right): https://codereview.chromium.org/1901563002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:783: static_cast<double>(remaining) / (remaining + deleted); On 2016/04/19 07:28:19, benwells wrote: > Nit: move proportion_remaining calculation up to line 759, so that all the code > to figure that out is together. Done. https://codereview.chromium.org/1901563002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service.cc:794: DCHECK_LE(score, SiteEngagementScore::kMaxPoints); On 2016/04/19 07:28:19, benwells wrote: > Should it be impossible for this DCHECK to fire? I'm not sure it is. You might > undecay it back up to beyone max points, in which case you want to cap it at max > points. Good point. Done. https://codereview.chromium.org/1901563002/diff/60001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/1901563002/diff/60001/components/history/core... components/history/core/browser/history_backend.cc:443: if (ContainsValue(origins, origin)) { On 2016/04/19 07:18:01, sdefresne wrote: > nit: I would change to this instead to avoid doing two lookups in the map: > > auto iter = origins.find(origin); > if (iter != origins.end()) { > std::pair<int, base::Time>& value = *iter; > ... > } Done. https://codereview.chromium.org/1901563002/diff/60001/components/history/core... File components/history/core/browser/history_types.h (right): https://codereview.chromium.org/1901563002/diff/60001/components/history/core... components/history/core/browser/history_types.h:464: typedef std::map<GURL, std::pair<int, base::Time>> OriginCountMap; On 2016/04/19 07:18:01, sdefresne wrote: > OriginCountAndLastVisitedMap? Done.
lgtm
Patchset #6 (id:100001) has been deleted
components/history lgtm
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org Link to the patchset: https://codereview.chromium.org/1901563002/#ps120001 (title: "scoped_ptr is now unique_ptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901563002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901563002/120001
Message was sent while issue was closed.
Description was changed from ========== Set site engagement timestamps to privacy-respectful values when history is cleared. The site engagement service stores last updated and last shortcut launch timestamps per origin. When history is cleared, the former is set to the time of history clearing and the latter is not changed. This CL sets both timestamps to the time of the most recent remaining visit in history for that origin once history clearing is completed. The last shortcut launch time is only set if it was non-null prior to the history being cleared. A slight change to the history service query (adding the last visited date to the data returned) is necessary to enable this change. More comprehensive testing of the history expiry mechanism are added to ensure correctness. BUG=604284 ========== to ========== Set site engagement timestamps to privacy-respectful values when history is cleared. The site engagement service stores last updated and last shortcut launch timestamps per origin. When history is cleared, the former is set to the time of history clearing and the latter is not changed. This CL sets both timestamps to the time of the most recent remaining visit in history for that origin once history clearing is completed. The last shortcut launch time is only set if it was non-null prior to the history being cleared. A slight change to the history service query (adding the last visited date to the data returned) is necessary to enable this change. More comprehensive testing of the history expiry mechanism are added to ensure correctness. BUG=604284 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Set site engagement timestamps to privacy-respectful values when history is cleared. The site engagement service stores last updated and last shortcut launch timestamps per origin. When history is cleared, the former is set to the time of history clearing and the latter is not changed. This CL sets both timestamps to the time of the most recent remaining visit in history for that origin once history clearing is completed. The last shortcut launch time is only set if it was non-null prior to the history being cleared. A slight change to the history service query (adding the last visited date to the data returned) is necessary to enable this change. More comprehensive testing of the history expiry mechanism are added to ensure correctness. BUG=604284 ========== to ========== Set site engagement timestamps to privacy-respectful values when history is cleared. The site engagement service stores last updated and last shortcut launch timestamps per origin. When history is cleared, the former is set to the time of history clearing and the latter is not changed. This CL sets both timestamps to the time of the most recent remaining visit in history for that origin once history clearing is completed. The last shortcut launch time is only set if it was non-null prior to the history being cleared. A slight change to the history service query (adding the last visited date to the data returned) is necessary to enable this change. More comprehensive testing of the history expiry mechanism are added to ensure correctness. BUG=604284 Committed: https://crrev.com/3d900e9e2c2dc6cda11c94aea9848181a9b4ffd1 Cr-Commit-Position: refs/heads/master@{#388321} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3d900e9e2c2dc6cda11c94aea9848181a9b4ffd1 Cr-Commit-Position: refs/heads/master@{#388321} |
