SuggestionsService undo blacklist functionality.

SuggestionsService undo blacklist functionality.

This CL introduces functionality to undo the blacklisting of a URL,
assuming a UI which allows for undo-ing for a few seconds after
blacklisting. The design consists in deffering blacklisted URL uploads
until they can no longer be undone, which is the simplest case.

Changes:
- BlacklistStore now has a concept of time before a URL is candidate for upload
- SuggestionsService now schedules blacklist uploads factoring in the delay

BUG=430949
Committed: https://crrev.com/3f7d74078e457ddfaf2bf07d1a78131249febc13
Cr-Commit-Position: refs/heads/master@{#307158}

[manzagop (departed), 2014-12-03 21:05:03]

manzagop@chromium.org changed reviewers:
+ mathp@chromium.org

[manzagop (departed), 2014-12-03 21:05:18]

Please have a look!

[Mathieu, 2014-12-04 18:53:33]

Comments!

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
File components/suggestions/blacklist_store.cc (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.cc:91: if (blacklist.urls_size() == 0)
!blacklist.urls_size() ?

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.cc:96: // A url is not in the timestamp
map: it's candidate for upload.
Indicate that this may happen if the application was restarted (BlacklistStore
should be a singleton so we can assume only a restart would bring us to this
case.)

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.cc:101: // Find the minimum blacklist
time. Note: blacklist_times_ is NOT empty since
CHECK(blacklist_times_ is not empty) ?

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
File components/suggestions/blacklist_store.h (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.h:41: virtual bool IsEmpty();
does this need to be public? I can't really see how it's used.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.h:49: virtual bool
GetTimeUntilReadyForUpload(const GURL& url,
I get easily confused by two methods with identical name.

Could we have GetTimeUntilURLReadyForUpload?

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
File components/suggestions/blacklist_store_unittest.cc (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store_unittest.cc:140:
EXPECT_TRUE(candidate_delta <= upload_delay);
reusing |candidate_delta| could mean that in case the function breaks and
doesn't modify |candidate_delta|, it wouldn't detect it.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store_unittest.cc:153:
EXPECT_LE(candidate_delta, no_delay);
are you not testing equal to zero here? If it's impractical to use another
function, add a comment.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service.cc:80: const TimeDelta
kDefaultSchedulingDelay = TimeDelta::FromSeconds(1);
consider having a const int and rather initialize TimeDelta in the constructor.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service.cc:168: if (!pending_request_.get())
{
put comment above here, mentioning why we only schedule when there isn't already
a request in progress.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service.cc:182: // The URL was in the
blacklist without being ready for upload and was
// The URL was not yet uploaded to the server and could be removed from the
blacklist.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_service_unittest.cc (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service_unittest.cc:543:
scoped_ptr<SuggestionsService> suggestions_service(
indent 2 less for the whole function

[manzagop (departed), 2014-12-05 15:13:23]

Addressed comments. PTAL!

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
File components/suggestions/blacklist_store.cc (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.cc:91: if (blacklist.urls_size() == 0)
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> !blacklist.urls_size() ?

Done.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.cc:96: // A url is not in the timestamp
map: it's candidate for upload.
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> Indicate that this may happen if the application was restarted (BlacklistStore
> should be a singleton so we can assume only a restart would bring us to this
> case.)

Done. Good point.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.cc:101: // Find the minimum blacklist
time. Note: blacklist_times_ is NOT empty since
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> CHECK(blacklist_times_ is not empty) ?

Done.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
File components/suggestions/blacklist_store.h (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.h:41: virtual bool IsEmpty();
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> does this need to be public? I can't really see how it's used.

Unused. Got rid of the function.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store.h:49: virtual bool
GetTimeUntilReadyForUpload(const GURL& url,
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> I get easily confused by two methods with identical name.
> 
> Could we have GetTimeUntilURLReadyForUpload?

Done.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
File components/suggestions/blacklist_store_unittest.cc (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store_unittest.cc:140:
EXPECT_TRUE(candidate_delta <= upload_delay);
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> reusing |candidate_delta| could mean that in case the function breaks and
> doesn't modify |candidate_delta|, it wouldn't detect it.

Done. Good catch.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/b...
components/suggestions/blacklist_store_unittest.cc:153:
EXPECT_LE(candidate_delta, no_delay);
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> are you not testing equal to zero here? If it's impractical to use another
> function, add a comment.

I should be. Forgot to update the test after finding negative times cause issues
when scheduling tasks.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service.cc:80: const TimeDelta
kDefaultSchedulingDelay = TimeDelta::FromSeconds(1);
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> consider having a const int and rather initialize TimeDelta in the
constructor.

Done. Went with local variables.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service.cc:168: if (!pending_request_.get())
{
On 2014/12/04 18:53:33, Mathieu Perreault wrote:
> put comment above here, mentioning why we only schedule when there isn't
already
> a request in progress.

Done.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service.cc:182: // The URL was in the
blacklist without being ready for upload and was
On 2014/12/04 18:53:32, Mathieu Perreault wrote:
> // The URL was not yet uploaded to the server and could be removed from the
> blacklist.
> 

Changed it, though not quite to what you propose because we can't remove a URL
that hasn't been uploaded if we're past the deadline.

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_service_unittest.cc (right):

https://codereview.chromium.org/766053010/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service_unittest.cc:543:
scoped_ptr<SuggestionsService> suggestions_service(
On 2014/12/04 18:53:33, Mathieu Perreault wrote:
> indent 2 less for the whole function

Done.

[Mathieu, 2014-12-05 16:32:05]

lgtm from my side.

https://codereview.chromium.org/766053010/diff/40001/components/suggestions/b...
File components/suggestions/blacklist_store.cc (right):

https://codereview.chromium.org/766053010/diff/40001/components/suggestions/b...
components/suggestions/blacklist_store.cc:99: CHECK(blacklist_times_.size());
Let's remove this and DCHECK below instead.

https://codereview.chromium.org/766053010/diff/40001/components/suggestions/b...
components/suggestions/blacklist_store.cc:100: base::TimeTicks min_time =
blacklist_times_.begin()->second;
initial value should be a big number

https://codereview.chromium.org/766053010/diff/40001/components/suggestions/b...
components/suggestions/blacklist_store.cc:107: 
DCHECK that it's not the big number

[manzagop (departed), 2014-12-05 20:09:20]

https://codereview.chromium.org/766053010/diff/40001/components/suggestions/b...
File components/suggestions/blacklist_store.cc (right):

https://codereview.chromium.org/766053010/diff/40001/components/suggestions/b...
components/suggestions/blacklist_store.cc:99: CHECK(blacklist_times_.size());
On 2014/12/05 16:32:05, Mathieu Perreault wrote:
> Let's remove this and DCHECK below instead.

Done.

https://codereview.chromium.org/766053010/diff/40001/components/suggestions/b...
components/suggestions/blacklist_store.cc:100: base::TimeTicks min_time =
blacklist_times_.begin()->second;
On 2014/12/05 16:32:05, Mathieu Perreault wrote:
> initial value should be a big number

Done. Operating on TimeDelta instead of TimeTicks since it has a Max() function.

https://codereview.chromium.org/766053010/diff/40001/components/suggestions/b...
components/suggestions/blacklist_store.cc:107: 
On 2014/12/05 16:32:05, Mathieu Perreault wrote:
> DCHECK that it's not the big number

Done.

[manzagop (departed), 2014-12-05 20:14:27]

manzagop@chromium.org changed reviewers:
+ newt@chromium.org

[manzagop (departed), 2014-12-05 20:14:27]

Hi Newton,
I'm missing OWNERS for most_visited_sites.cc. The change is small: a callback
has been added to the SuggestionsService's BlacklistURL. Could you have a look?
Thanks!
Pierre

[newt (away), 2014-12-05 22:07:07]

On 2014/12/05 20:14:27, manzagop wrote:
> Hi Newton,
> I'm missing OWNERS for most_visited_sites.cc. The change is small: a callback
> has been added to the SuggestionsService's BlacklistURL. Could you have a
look?
> Thanks!
> Pierre

chrome/browser/android/most_visited_sites.cc lgtm

[newt (away), 2014-12-05 22:12:54]

On 2014/12/05 22:07:07, newt wrote:
> On 2014/12/05 20:14:27, manzagop wrote:
> > Hi Newton,
> > I'm missing OWNERS for most_visited_sites.cc. The change is small: a
callback
> > has been added to the SuggestionsService's BlacklistURL. Could you have a
> look?
> > Thanks!
> > Pierre
> 
> chrome/browser/android/most_visited_sites.cc lgtm

mathp: Do we have support for delayed blacklisting in the Chrome for Android UI?

[Mathieu, 2014-12-05 22:47:10]

Not in the UI, no. On iOS they do, which is the motivation.
On Dec 5, 2014 5:12 PM, <newt@chromium.org> wrote:

> On 2014/12/05 22:07:07, newt wrote:
>
>> On 2014/12/05 20:14:27, manzagop wrote:
>> > Hi Newton,
>> > I'm missing OWNERS for most_visited_sites.cc. The change is small: a
>>
> callback
>
>> > has been added to the SuggestionsService's BlacklistURL. Could you have
>> a
>> look?
>> > Thanks!
>> > Pierre
>>
>
>  chrome/browser/android/most_visited_sites.cc lgtm
>>
>
> mathp: Do we have support for delayed blacklisting in the Chrome for
> Android UI?
>
> https://codereview.chromium.org/766053010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mathieu, 2014-12-05 22:49:17]

On 2014/12/05 22:47:10, Mathieu Perreault wrote:
> Not in the UI, no. On iOS they do, which is the motivation.
> On Dec 5, 2014 5:12 PM, <mailto:newt@chromium.org> wrote:
> 
> > On 2014/12/05 22:07:07, newt wrote:
> >
> >> On 2014/12/05 20:14:27, manzagop wrote:
> >> > Hi Newton,
> >> > I'm missing OWNERS for most_visited_sites.cc. The change is small: a
> >>
> > callback
> >
> >> > has been added to the SuggestionsService's BlacklistURL. Could you have
> >> a
> >> look?
> >> > Thanks!
> >> > Pierre
> >>
> >
> >  chrome/browser/android/most_visited_sites.cc lgtm
> >>
> >
> > mathp: Do we have support for delayed blacklisting in the Chrome for
> > Android UI?
> >
> > https://codereview.chromium.org/766053010/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

By "delayed blacklisting" I assumed you meant being given some amount of time to
undo a blacklist. They show a toast on iOS to undo removing a tile. It's similar
to how it's done on the desktop NTP. Following this change we can implement this
on Android for the Suggestionsservice suggestions.

[newt (away), 2014-12-05 22:58:53]

What I meant was: will this change cause weird behavior in Chrome for Android's
UI? i.e. when you click "Remove" on a site, will there be a 3 second delay
before it disappears? That would look odd.

[Mathieu, 2014-12-06 00:55:29]

On 2014/12/05 22:58:53, newt wrote:
> What I meant was: will this change cause weird behavior in Chrome for
Android's
> UI? i.e. when you click "Remove" on a site, will there be a 3 second delay
> before it disappears? That would look odd.

No, we return immediately with the item removed. No change there.

[newt (away), 2014-12-06 01:32:22]

Great :)

On Fri, Dec 5, 2014 at 6:55 PM, <mathp@chromium.org> wrote:

> On 2014/12/05 22:58:53, newt wrote:
>
>> What I meant was: will this change cause weird behavior in Chrome for
>>
> Android's
>
>> UI? i.e. when you click "Remove" on a site, will there be a 3 second delay
>> before it disappears? That would look odd.
>>
>
> No, we return immediately with the item removed. No change there.
>
> https://codereview.chromium.org/766053010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[manzagop (departed), 2014-12-06 01:39:53]

On 2014/12/06 01:32:22, newt wrote:
> Great :)
> 
> On Fri, Dec 5, 2014 at 6:55 PM, <mailto:mathp@chromium.org> wrote:
> 
> > On 2014/12/05 22:58:53, newt wrote:
> >
> >> What I meant was: will this change cause weird behavior in Chrome for
> >>
> > Android's
> >
> >> UI? i.e. when you click "Remove" on a site, will there be a 3 second delay
> >> before it disappears? That would look odd.
> >>
> >
> > No, we return immediately with the item removed. No change there.
> >
> > https://codereview.chromium.org/766053010/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks for the review!

[manzagop (departed), 2014-12-06 01:40:01]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2014-12-06 01:40:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/766053010/60001

[commit-bot: I haz the power, 2014-12-06 01:47:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-06 01:47:17]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[manzagop (departed), 2014-12-06 02:10:20]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2014-12-06 02:10:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/766053010/80001

[commit-bot: I haz the power, 2014-12-06 03:32:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-06 03:32:22]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[manzagop (departed), 2014-12-06 03:45:52]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2014-12-06 03:47:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/766053010/100001

[commit-bot: I haz the power, 2014-12-06 05:04:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-06 05:05:33]

Patchset 6 (id:??) landed as
https://crrev.com/3f7d74078e457ddfaf2bf07d1a78131249febc13
Cr-Commit-Position: refs/heads/master@{#307158}