[NTP] Cleanup: offline pages related tests.

chrome/browser/ntp_snippets/download_suggestions_provider.cc

Index: chrome/browser/ntp_snippets/download_suggestions_provider.cc
diff --git a/chrome/browser/ntp_snippets/download_suggestions_provider.cc b/chrome/browser/ntp_snippets/download_suggestions_provider.cc
index 2c1393158dd627bd903dda2eaa4afd0a8180b4ca..2b9a92eac885c43b728641be4de5f4b194c5db2c 100644
--- a/chrome/browser/ntp_snippets/download_suggestions_provider.cc
+++ b/chrome/browser/ntp_snippets/download_suggestions_provider.cc
@@ -233,10 +233,12 @@ void DownloadSuggestionsProvider::GetDismissedSuggestionsForDebugging(
     const ntp_snippets::DismissedSuggestionsCallback& callback) {
   DCHECK_EQ(provided_category_, category);
 
-  // TODO(vitaliii): Query all pages instead by using an empty query.
   if (offline_page_model_) {
+    // Even not related to downloads offline pages are queried here, so that

[Marc Treib, 2016/11/21 12:27:53]

Offline pages which are not related to downloads are also queried here, so
that...
?

[vitaliii, 2016/11/21 14:56:46]

Done.

+    // they can be returned in case there are problems with dismissing.

[Marc Treib, 2016/11/21 12:27:53]

I don't quite understand this. You're saying, this also returns Recent Tabs,
because that might be helpful when debugging?

[vitaliii, 2016/11/21 14:56:46]

Yes, while testing, I would say.
My arguments: 
1) the function contract does not say that the returned suggestions must be
related to downloads, so I do not see why we should add this implicit
assumption;
2) this function is used only for testing, so no performance drawbacks.

Overall, I do not like the idea of returning |ContentSuggestion|'s (because as
you saw in the tests, this forces us to keep all suggestions in the data
source), but I do not have a better approach (IDs seem to be an implementation
detail). 

[Marc Treib, 2016/11/21 15:38:10]

Well, it's a method on the Provider, so I'd say it is not-so-implicitly assumed
that it would only return this provider's suggestions.
I can see that it might be useful anyway, given that two providers share a data
source. But it could also be confusing to see dismissed downloads, even though
you've never dismissed a download...
Yeah, I'm not worried about that.
Yes, the GetDismissedSuggestionsForDebugging implementation has proven to be
quite a burden for several providers. Maybe we should rethink if it's worth all
the effort.

[vitaliii, 2016/11/21 15:49:55]

Hm, it seems like we misunderstood each other. 

I clearly agree that |GetDismissedSuggestionsForDebugging| should not return
unrelated to this provider suggestions. My point is that the main reason for
this must be that the dismissed suggestions code works correctly, but not that
we explicitly decided not to return "wrong" suggestions even if they are
actually dismissed. 

In other words, my argument is that it must have an ability to return not
related to downloads offline pages, if they are actually dismissed. However, by
querying only subset of offline pages we take away this ability.

[Marc Treib, 2016/11/21 15:56:57]

Ah, I think I see your point now. No objections then, though the comment could
maybe be a bit clearer :)

[vitaliii, 2016/11/22 06:32:46]

Done.

+    OfflinePageModelQueryBuilder query_builder;
     offline_page_model_->GetPagesMatchingQuery(
-        BuildOfflinePageDownloadsQuery(offline_page_model_),
+        query_builder.Build(offline_page_model_->GetPolicyController()),
         base::Bind(&DownloadSuggestionsProvider::
                        GetPagesMatchingQueryCallbackForGetDismissedSuggestions,
                    weak_ptr_factory_.GetWeakPtr(), callback));