[NTP] Cleanup: offline pages related tests.

chrome/browser/ntp_snippets/download_suggestions_provider.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ntp_snippets/download_suggestions_provider.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 215 matching lines @@
   // Ignored. The internal caches are not stored on disk and they are just
   // partial copies of the data stored at OfflinePage model and DownloadManager.
   // If it is cleared there, it will be cleared in these caches as well.
 }
 
 void DownloadSuggestionsProvider::GetDismissedSuggestionsForDebugging(
     Category category,
     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));
   } else {
     GetPagesMatchingQueryCallbackForGetDismissedSuggestions(
         callback, std::vector<OfflinePageItem>());
   }
 }
 
 void DownloadSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
@@ skipping 465 matching lines @@
 
 void DownloadSuggestionsProvider::UnregisterDownloadItemObservers() {
   DCHECK_NE(download_manager_, nullptr);
 
   std::vector<DownloadItem*> all_downloads;
   download_manager_->GetAllDownloads(&all_downloads);
 
   for (DownloadItem* item : all_downloads)
     item->RemoveObserver(this);
 }