Inform server of dismissed articles.

Inform server of dismissed articles.

It seems that the server currently has logic to not send back excluded
IDs, but lacks the logic needed to request more. Still, that's something
that can be improved in the server later.

There should be tests for NTPSnippetsService to verify the new behavior,
but that's not possible until it uses the new server.

BUG=633613
Committed: https://crrev.com/52f7671273011c930c4f1ba8a252eac426f88948
Cr-Commit-Position: refs/heads/master@{#414428}

[sfiera, 2016-08-24 16:40:51]

sfiera@chromium.org changed reviewers:
+ treib@chromium.org

[sfiera, 2016-08-24 16:40:51]

Oops. This is more important than server-side categories in M54. Fortunately,
it's much shorter. PTAL

[tschumann, 2016-08-24 16:56:19]

tschumann@chromium.org changed reviewers:
+ tschumann@chromium.org

[tschumann, 2016-08-24 16:56:19]

lgtm


Completely unrelated: the fetcher could use some refactoring some day -- it's
quite messy. Nothing to fix this week though ;-)

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:182:
test_excluded_.insert("1234567890");
nit: ids are now in hex format -- might be nice to document here, say
0x0123456789abcdef

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:270:
excluded_ids.insert(snippet->id());
i'm wondering if we should limit those, to say no more than 200 or so. but we
don't have them in chronological order, right?

[chromium-reviews, 2016-08-24 17:02:30]

On Wed, Aug 24, 2016 at 9:40 AM, <sfiera@chromium.org> wrote:

> Reviewers: Marc Treib
> CL: https://codereview.chromium.org/2274953002/
>
> Message:
> Oops. This is more important than server-side categories in M54.
> Fortunately,
> it's much shorter. PTAL
>
> Description:
> Inform server of dismissed articles.
>
> It seems that the server currently has logic to not send back excluded
> IDs, but lacks the logic needed to request more.
>
Just clarifying: You mean to request more data from its backends to send a
full set of suggestions back to the client.
(having the client request additional elements as in the More button is out
of scope for M54).
Do we have a bug for the server-side part? (like requesting more to make
sure it has enough results)


> Still, that's something
> that can be improved in the server later.
>
> There should be tests for NTPSnippetsService to verify the new behavior,
> but that's not possible until it uses the new server.
>
> BUG=633613
>
> Base URL: https://chromium.googlesource.com/chromium/src@master
>
> Affected files (+49, -4 lines):
> M components/ntp_snippets/ntp_snippets_fetcher.h
> M components/ntp_snippets/ntp_snippets_fetcher.cc
> M components/ntp_snippets/ntp_snippets_fetcher_unittest.cc
> M components/ntp_snippets/ntp_snippets_service.cc
>
>
> Index: components/ntp_snippets/ntp_snippets_fetcher.cc
> diff --git a/components/ntp_snippets/ntp_snippets_fetcher.cc
> b/components/ntp_snippets/ntp_snippets_fetcher.cc
> index c0018e7d816ad650ff06f25bdf664b5b64034e2e..
> f7177584effad68c34d9b5c81b38a7d04b81dbee 100644
> --- a/components/ntp_snippets/ntp_snippets_fetcher.cc
> +++ b/components/ntp_snippets/ntp_snippets_fetcher.cc
> @@ -216,6 +216,7 @@ void NTPSnippetsFetcher::SetCallback(
> void NTPSnippetsFetcher::FetchSnippetsFromHosts(
> const std::set<std::string>& hosts,
> const std::string& language_code,
> + const std::set<std::string>& excluded_ids,
> int count,
> bool interactive_request) {
> if (!request_throttler_.DemandQuotaForRequest(interactive_request))
> @@ -223,6 +224,7 @@ void NTPSnippetsFetcher::FetchSnippetsFromHosts(
>
> hosts_ = hosts;
> fetch_start_time_ = tick_clock_->NowTicks();
> + excluded_ids_ = excluded_ids;
>
> if (UsesHostRestrictions() && hosts_.empty()) {
> FetchFinished(OptionalSnippets(), FetchResult::EMPTY_HOSTS,
> @@ -317,12 +319,19 @@ std::string
NTPSnippetsFetcher::RequestParams::BuildRequest()
> {
> if (!user_locale.empty()) {
> request->SetString("uiLanguage", user_locale);
> }
> +
> auto regular_hosts = base::MakeUnique<base::ListValue>();
> for (const auto& host : host_restricts) {
> regular_hosts->AppendString(host);
> }
> request->Set("regularlyVisitedHostNames", std::move(regular_hosts));
>
> + auto excluded = base::MakeUnique<base::ListValue>();
> + for (const auto& id : excluded_ids) {
> + excluded->AppendString(id);
> + }
> + request->Set("excludedSuggestionIds", std::move(excluded));
> +
> // TODO(sfiera): support authentication and personalization
> // TODO(sfiera): support count_to_fetch
> break;
> @@ -392,6 +401,7 @@ void NTPSnippetsFetcher::
> FetchSnippetsNonAuthenticated() {
> params.fetch_api = fetch_api_;
> params.host_restricts =
> UsesHostRestrictions() ? hosts_ : std::set<std::string>();
> + params.excluded_ids = excluded_ids_;
> params.count_to_fetch = count_to_fetch_;
> FetchSnippetsImpl(url, std::string(), params.BuildRequest());
> }
> @@ -407,6 +417,7 @@ void NTPSnippetsFetcher::FetchSnippetsAuthenticated(
> params.user_locale = locale_;
> params.host_restricts =
> UsesHostRestrictions() ? hosts_ : std::set<std::string>();
> + params.excluded_ids = excluded_ids_;
> params.count_to_fetch = count_to_fetch_;
> // TODO(jkrcal, treib): Add unit-tests for authenticated fetches.
> FetchSnippetsImpl(fetch_url_,
> Index: components/ntp_snippets/ntp_snippets_fetcher.h
> diff --git a/components/ntp_snippets/ntp_snippets_fetcher.h
> b/components/ntp_snippets/ntp_snippets_fetcher.h
> index 33f6c180ff2cc205d0bee64c7cfe6b7c2af5d015..
> daa8bae58d0cf2b3aee947b3e58e7f6161925fcd 100644
> --- a/components/ntp_snippets/ntp_snippets_fetcher.h
> +++ b/components/ntp_snippets/ntp_snippets_fetcher.h
> @@ -91,6 +91,7 @@ class NTPSnippetsFetcher : public
> OAuth2TokenService::Consumer,
>
> // Fetches snippets from the server. |hosts| restricts the results to a
> set of
> // hosts, e.g. "www.google.com". An empty host set produces an error.
> + // Suggestions in |excluded_ids| will not be returned.
> //
> // If an ongoing fetch exists, it will be cancelled and a new one started,
> // without triggering an additional callback (i.e. not noticeable by
> @@ -100,6 +101,7 @@ class NTPSnippetsFetcher : public
> OAuth2TokenService::Consumer,
> // |interactive_request| is set to true (use only for user-initiated
> fetches).
> void FetchSnippetsFromHosts(const std::set<std::string>& hosts,
> const std::string& language_code,
> + const std::set<std::string>& excluded_ids,
> int count,
> bool interactive_request);
>
> @@ -146,6 +148,7 @@ class NTPSnippetsFetcher : public
> OAuth2TokenService::Consumer,
> bool only_return_personalized_results;
> std::string user_locale;
> std::set<std::string> host_restricts;
> + std::set<std::string> excluded_ids;
> int count_to_fetch;
>
> RequestParams();
> @@ -201,6 +204,9 @@ class NTPSnippetsFetcher : public
> OAuth2TokenService::Consumer,
> // Hosts to restrict the snippets to.
> std::set<std::string> hosts_;
>
> + // Snippets to exclude from the results.
> + std::set<std::string> excluded_ids_;
> +
> // Count of snippets to fetch.
> int count_to_fetch_;
>
> Index: components/ntp_snippets/ntp_snippets_fetcher_unittest.cc
> diff --git a/components/ntp_snippets/ntp_snippets_fetcher_unittest.cc
> b/components/ntp_snippets/ntp_snippets_fetcher_unittest.cc
> index de2e1ccd3bf94f7e2f2fecab6c5213ee85228574..
> 2467e2316689ab26298a9dc63816eb0824da645a 100644
> --- a/components/ntp_snippets/ntp_snippets_fetcher_unittest.cc
> +++ b/components/ntp_snippets/ntp_snippets_fetcher_unittest.cc
> @@ -179,6 +179,7 @@ class NTPSnippetsFetcherTest : public testing::Test {
> snippets_fetcher_->SetTickClockForTesting(
> mock_task_runner_->GetMockTickClock());
> test_hosts_.insert("www.somehost.com");
> + test_excluded_.insert("1234567890");
> // Increase initial time such that ticks are non-zero.
> mock_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(1234));
> }
> @@ -191,6 +192,7 @@ class NTPSnippetsFetcherTest : public testing::Test {
> const std::string& test_lang() const { return test_lang_; }
> const GURL& test_url() { return test_url_; }
> const std::set<std::string>& test_hosts() const { return test_hosts_; }
> + const std::set<std::string>& test_excluded() const { return
> test_excluded_; }
> base::HistogramTester& histogram_tester() { return histogram_tester_; }
>
> void InitFakeURLFetcherFactory() {
> @@ -228,6 +230,7 @@ class NTPSnippetsFetcherTest : public testing::Test {
> const std::string test_lang_;
> const GURL test_url_;
> std::set<std::string> test_hosts_;
> + std::set<std::string> test_excluded_;
> base::HistogramTester histogram_tester_;
>
> DISALLOW_COPY_AND_ASSIGN(NTPSnippetsFetcherTest);
> @@ -255,6 +258,7 @@ TEST_F(NTPSnippetsFetcherTest,
> BuildRequestAuthenticated) {
> params.only_return_personalized_results = true;
> params.user_locale = "en";
> params.host_restricts = {"chromium.org"};
> + params.excluded_ids = {"1234567890"};
> params.count_to_fetch = 25;
>
> params.fetch_api = NTPSnippetsFetcher::CHROME_READER_API;
> @@ -302,6 +306,9 @@ TEST_F(NTPSnippetsFetcherTest,
> BuildRequestAuthenticated) {
> " \"uiLanguage\": \"en\","
> " \"regularlyVisitedHostNames\": ["
> " \"chromium.org\""
> + " ],"
> + " \"excludedSuggestionIds\": ["
> + " \"1234567890\""
> " ]"
> "}"));
> }
> @@ -311,6 +318,7 @@ TEST_F(NTPSnippetsFetcherTest,
> BuildRequestUnauthenticated) {
> params.only_return_personalized_results = false;
> params.host_restricts = {};
> params.count_to_fetch = 10;
> + params.excluded_ids = {};
>
> params.fetch_api = NTPSnippetsFetcher::CHROME_READER_API;
> EXPECT_THAT(params.BuildRequest(),
> @@ -347,7 +355,8 @@ TEST_F(NTPSnippetsFetcherTest,
> BuildRequestUnauthenticated) {
> params.fetch_api = NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API;
> EXPECT_THAT(params.BuildRequest(),
> EqualsJSON("{"
> - " \"regularlyVisitedHostNames\": []"
> + " \"regularlyVisitedHostNames\": [],"
> + " \"excludedSuggestionIds\": []"
> "}"));
> }
>
> @@ -378,6 +387,7 @@ TEST_F(NTPSnippetsFetcherTest,
> ShouldFetchSuccessfully) {
> net::URLRequestStatus::SUCCESS);
> EXPECT_CALL(mock_callback(), Run(IsSingleArticle("http://localhost/foobar
> ")));
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -413,6 +423,7 @@ TEST_F(NTPSnippetsContentSuggestionsFetcherTest,
> ShouldFetchSuccessfully) {
> net::URLRequestStatus::SUCCESS);
> EXPECT_CALL(mock_callback(), Run(IsSingleArticle("http://localhost/foobar
> ")));
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -465,6 +476,7 @@ TEST_F(NTPSnippetsContentSuggestionsFetcherTest,
> ServerCategories) {
> EXPECT_CALL(mock_callback(), Run(_))
> .WillOnce(WithArg<0>(MovePointeeTo(&snippets)));
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*force_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -505,6 +517,7 @@ TEST_F(NTPSnippetsFetcherTest,
> ShouldFetchSuccessfullyEmptyList) {
> net::URLRequestStatus::SUCCESS);
> EXPECT_CALL(mock_callback(), Run(IsEmptyArticleList()));
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -522,6 +535,7 @@ TEST_F(NTPSnippetsFetcherHostRestrictedTest,
> ShouldReportEmptyHostsError) {
> EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
> snippets_fetcher().FetchSnippetsFromHosts(/*hosts=*/std::set<std::string>(
> ),
> /*language_code=*/"en-US",
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -543,7 +557,8 @@ TEST_F(NTPSnippetsFetcherHostRestrictedTest,
> ShouldReportEmptyHostsError) {
> TEST_F(NTPSnippetsFetcherHostRestrictedTest, ShouldRestrictToHosts) {
> net::TestURLFetcherFactory test_url_fetcher_factory;
> snippets_fetcher().FetchSnippetsFromHosts(
> - {"www.somehost1.com", "www.somehost2.com"}, test_lang(), /*count=*/17,
> + {"www.somehost1.com", "www.somehost2.com"}, test_lang(),
> test_excluded(),
> + /*count=*/17,
> /*interactive_request=*/true);
> net::TestURLFetcher* fetcher = test_url_fetcher_factory.GetFetcherByID(0);
> ASSERT_THAT(fetcher, NotNull());
> @@ -575,6 +590,7 @@ TEST_F(NTPSnippetsFetcherTest,
> ShouldReportUrlStatusError) {
> net::URLRequestStatus::FAILED);
> EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -596,6 +612,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldReportHttpError) {
> net::URLRequestStatus::SUCCESS);
> EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -616,6 +633,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldReportJsonError) {
> net::URLRequestStatus::SUCCESS);
> EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -638,6 +656,7 @@ TEST_F(NTPSnippetsFetcherTest,
> ShouldReportJsonErrorForEmptyResponse) {
> net::URLRequestStatus::SUCCESS);
> EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -657,6 +676,7 @@ TEST_F(NTPSnippetsFetcherTest,
> ShouldReportInvalidListError) {
> net::URLRequestStatus::SUCCESS);
> EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -677,6 +697,7 @@ TEST_F(NTPSnippetsFetcherTest,
> ShouldReportHttpErrorForMissingBakedResponse) {
> InitFakeURLFetcherFactory();
> EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> @@ -688,11 +709,13 @@ TEST_F(NTPSnippetsFetcherTest,
> ShouldCancelOngoingFetch) {
> net::URLRequestStatus::SUCCESS);
> EXPECT_CALL(mock_callback(), Run(IsEmptyArticleList()));
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> // Second call to FetchSnippetsFromHosts() overrides/cancels the previous.
> // Callback is expected to be called once.
> snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
> + test_excluded(),
> /*count=*/1,
> /*interactive_request=*/true);
> FastForwardUntilNoTasksRemain();
> Index: components/ntp_snippets/ntp_snippets_service.cc
> diff --git a/components/ntp_snippets/ntp_snippets_service.cc
> b/components/ntp_snippets/ntp_snippets_service.cc
> index 1c8a12acfa6e90752082ce69e8841384db8ef89b..
> 2d96c0c2d3329b998c2a64ef36328933121db02b 100644
> --- a/components/ntp_snippets/ntp_snippets_service.cc
> +++ b/components/ntp_snippets/ntp_snippets_service.cc
> @@ -265,8 +265,13 @@ void NTPSnippetsService::FetchSnippetsFromHosts(
> if (snippets_.empty())
> UpdateCategoryStatus(CategoryStatus::AVAILABLE_LOADING);
>
> - snippets_fetcher_->FetchSnippetsFromHosts(
> - hosts, application_language_code_, kMaxSnippetCount,
> interactive_request);
> + std::set<std::string> excluded_ids;
> + for (const auto& snippet : dismissed_snippets_) {
> + excluded_ids.insert(snippet->id());
> + }
> + snippets_fetcher_->FetchSnippetsFromHosts(hosts,
> application_language_code_,
> + excluded_ids, kMaxSnippetCount,
> + interactive_request);
> }
>
> void NTPSnippetsService::RescheduleFetching() {
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "New Tab Page development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ntp-dev+unsubscribe@chromium.org.
> To post to this group, send email to ntp-dev@chromium.org.
> To view this discussion on the web visit https://groups.google.com/a/
> chromium.org/d/msgid/ntp-dev/001a1141ebb2b9acc7053ad3f15b%40google.com
>
<https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a1141ebb2b9acc705...>
> .
>


Tim Schumann

Software Engineer

tschumann@google.com


Google Germany GmbH

Erika-Mann-Str. 33 Munich, 80636

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und
löschen Sie die E-Mail und alle Anhänge. Vielen Dank.


This e-mail is confidential. If you are not the right addressee please do
not forward it, please inform the sender, and please erase this e-mail
including any attachments. Thanks.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sfiera, 2016-08-24 17:08:29]

On 2016/08/24 17:02:30, chromium-reviews wrote:
> Just clarifying: You mean to request more data from its backends to send a
> full set of suggestions back to the client.

Yes.

> (having the client request additional elements as in the More button is out
> of scope for M54).
> Do we have a bug for the server-side part? (like requesting more to make
> sure it has enough results)

Not that I'm aware of.

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:270:
excluded_ids.insert(snippet->id());
On 2016/08/24 16:56:19, tschumann wrote:
> i'm wondering if we should limit those, to say no more than 200 or so. but we
> don't have them in chronological order, right?

We have timestamps of creation and expiration, but not the time when we received
them from the server.

In any case, I assume that the server will only honor exclusions up to, say, 50
or so IDs. If, at that point, we stop returning new snippets, it will serve as a
throttle on dismissing more snippets.

[sfiera, 2016-08-24 17:14:11]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 17:14:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tschumann, 2016-08-24 18:24:07]

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:270:
excluded_ids.insert(snippet->id());
On 2016/08/24 17:08:28, sfiera wrote:
> On 2016/08/24 16:56:19, tschumann wrote:
> > i'm wondering if we should limit those, to say no more than 200 or so. but
we
> > don't have them in chronological order, right?
> 
> We have timestamps of creation and expiration, but not the time when we
received
> them from the server.
> 
> In any case, I assume that the server will only honor exclusions up to, say,
50
> or so IDs. If, at that point, we stop returning new snippets, it will serve as
a
> throttle on dismissing more snippets.

Good point! This of course assumes that we don't screw up expiration dates some
day. Would be bad to send large requests just because the articles only expire
in 2033 for some reason ;-)
I might be a bit paranoid though...

[commit-bot: I haz the power, 2016-08-24 19:23:13]

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

[commit-bot: I haz the power, 2016-08-24 19:23:14]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Marc Treib, 2016-08-25 09:01:42]

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.h:93: // hosts, e.g.
"www.google.com". An empty host set produces an error.
Not your doing, but: Do you mind updating this comment? It only applies if host
restrictions are on, and they're actually off by default now.

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:270:
excluded_ids.insert(snippet->id());
On 2016/08/24 18:24:07, tschumann wrote:
> On 2016/08/24 17:08:28, sfiera wrote:
> > On 2016/08/24 16:56:19, tschumann wrote:
> > > i'm wondering if we should limit those, to say no more than 200 or so. but
> we
> > > don't have them in chronological order, right?
> > 
> > We have timestamps of creation and expiration, but not the time when we
> received
> > them from the server.
> > 
> > In any case, I assume that the server will only honor exclusions up to, say,
> 50
> > or so IDs. If, at that point, we stop returning new snippets, it will serve
as
> a
> > throttle on dismissing more snippets.
> 
> Good point! This of course assumes that we don't screw up expiration dates
some
> day. Would be bad to send large requests just because the articles only expire
> in 2033 for some reason ;-)
> I might be a bit paranoid though...

+1 for some hard limit, just to be safe. I'd put that limit into the fetcher
though, not here.

[sfiera, 2016-08-25 10:39:56]

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.h:93: // hosts, e.g.
"www.google.com". An empty host set produces an error.
On 2016/08/25 09:01:41, Marc Treib wrote:
> Not your doing, but: Do you mind updating this comment? It only applies if
host
> restrictions are on, and they're actually off by default now.

Done.

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2274953002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:270:
excluded_ids.insert(snippet->id());
On 2016/08/25 09:01:41, Marc Treib wrote:
> On 2016/08/24 18:24:07, tschumann wrote:
> > On 2016/08/24 17:08:28, sfiera wrote:
> > > On 2016/08/24 16:56:19, tschumann wrote:
> > > > i'm wondering if we should limit those, to say no more than 200 or so.
but
> > we
> > > > don't have them in chronological order, right?
> > > 
> > > We have timestamps of creation and expiration, but not the time when we
> > received
> > > them from the server.
> > > 
> > > In any case, I assume that the server will only honor exclusions up to,
say,
> > 50
> > > or so IDs. If, at that point, we stop returning new snippets, it will
serve
> as
> > a
> > > throttle on dismissing more snippets.
> > 
> > Good point! This of course assumes that we don't screw up expiration dates
> some
> > day. Would be bad to send large requests just because the articles only
expire
> > in 2033 for some reason ;-)
> > I might be a bit paranoid though...
> 
> +1 for some hard limit, just to be safe. I'd put that limit into the fetcher
> though, not here.

Done (there, 100).

[sfiera, 2016-08-25 10:40:00]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 10:40:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 10:55:15]

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

[commit-bot: I haz the power, 2016-08-25 10:55:15]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Marc Treib, 2016-08-25 11:41:44]

LGTM with some more nits

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:334: if (exclusion_count++ >=
kMaxExcludedIds)
ListValue has a GetSize, no need for a separate counter

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:93: // hosts, e.g.
"www.google.com". If host restrictions are enabled, en empty
s/en/an/

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:397: // Truncated to
100 entries, those lexically first.
"lexically first" is really just how it happens to be, not something we
explicitly want, right? Might be worth saying that in the comment - as it is, it
sounds like we're explicitly testing for that.

[sfiera, 2016-08-25 11:48:16]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 11:48:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-08-25 11:58:08]

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:334: if (exclusion_count++ >=
kMaxExcludedIds)
On 2016/08/25 11:41:44, Marc Treib wrote:
> ListValue has a GetSize, no need for a separate counter

Done.

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.h:93: // hosts, e.g.
"www.google.com". If host restrictions are enabled, en empty
On 2016/08/25 11:41:44, Marc Treib wrote:
> s/en/an/

Done.

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/2274953002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:397: // Truncated to
100 entries, those lexically first.
On 2016/08/25 11:41:44, Marc Treib wrote:
> "lexically first" is really just how it happens to be, not something we
> explicitly want, right? Might be worth saying that in the comment - as it is,
it
> sounds like we're explicitly testing for that.

Done.

[sfiera, 2016-08-25 12:18:22]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-08-25 12:18:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
tschumann@chromium.org, treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2274953002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-25 12:18:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 12:27:33]

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

[commit-bot: I haz the power, 2016-08-25 12:27:34]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sfiera, 2016-08-25 12:40:48]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-08-25 12:40:48]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
tschumann@chromium.org
Link to the patchset: https://codereview.chromium.org/2274953002/#ps100001
(title: "Rebase, fix.")

[commit-bot: I haz the power, 2016-08-25 12:41:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 14:05:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-25 14:09:05]

Description was changed from

==========
Inform server of dismissed articles.

It seems that the server currently has logic to not send back excluded
IDs, but lacks the logic needed to request more. Still, that's something
that can be improved in the server later.

There should be tests for NTPSnippetsService to verify the new behavior,
but that's not possible until it uses the new server.

BUG=633613
==========

to

==========
Inform server of dismissed articles.

It seems that the server currently has logic to not send back excluded
IDs, but lacks the logic needed to request more. Still, that's something
that can be improved in the server later.

There should be tests for NTPSnippetsService to verify the new behavior,
but that's not possible until it uses the new server.

BUG=633613

Committed: https://crrev.com/52f7671273011c930c4f1ba8a252eac426f88948
Cr-Commit-Position: refs/heads/master@{#414428}
==========

[commit-bot: I haz the power, 2016-08-25 14:09:06]

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