[NTP:Downloads] Request offline pages even if model is not loaded.

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 474 matching lines @@
 
 void DownloadSuggestionsProvider::AsynchronouslyFetchOfflinePagesDownloads(
     bool notify) {
   if (!offline_page_model_) {
     // Offline pages are explicitly turned off, so we propagate "no pages"
     // further e.g. to clean its prefs.
     UpdateOfflinePagesCache(notify, std::vector<OfflinePageItem>());
     return;
   }
 
-  if (!offline_page_model_->is_loaded()) {
-    // Offline pages model is not ready yet and may return no offline pages.
-    if (notify) {
-      SubmitContentSuggestions();

[Marc Treib, 2017/02/10 09:08:11]

So the change is essentially to *not* notify here?
...okay, I guess that's reasonable. What happens if Offline Pages takes a looong
time to load for some reason?

[vitaliii, 2017/02/10 09:39:56]

The change is to query even if the model has not been loaded yet.
I did an experiment.
I inserted 10 seconds of sleep into OfflinePageModelImpl::FinalizeModelLoad().
With this CL the NTP is not loaded for the 10 seconds at all. Omnibox is
visible, but it is not functional.
However, the behavior is completely the same without this CL (still 10 seconds
delay before NTP is shown). So it seems that the model is loaded in the same
thread anyway. Note that our code checks is_loaded before this delay.

dewittj@ talked about 200ms median load time (see
https://codereview.chromium.org/2564163002/).

[Marc Treib, 2017/02/10 10:05:02]

Ah, I thought we'd query as soon as the model has loaded anyway, but looks like
that was removed at some point. Alright then.

Still, a second change is that before, we'd notify immediately here, while now,
we first wait for the OfflinePageModel to load.
That is interesting, and slightly terrifying.
Have you checked what happens when some other page is open - does the
OfflinePageModel load also block that?
If not, then apparently something on the NTP waits for the OfflinePageModel.
That's definitely something we should fix.

Huh, or on second thought: I'm not sure your experiment is valid. It looks like
FinalizeModelLoad runs on the UI thread, so inserting a delay there would freeze
the whole UI for that time. To simulate that the load takes long, you need to
insert a delay in the actual background task that does the loading. Maybe here:
https://cs.chromium.org/chromium/src/components/offline_pages/core/offline_pa...
Median IMO isn't very useful here; I expect there's a very long tail, especially
on lower-end devices. If that delay actually delays showing the NTP, then we
should investigate if we can do something about it.

[vitaliii, 2017/02/10 12:54:11]

But previously we would show incomplete list (without Offline Pages). Showing
nothing feels better.
Indeed, you are right. I inserted a 10 second sleep into  GetOfflinePagesSync
and now NTP works normally (even offline badge appers once the sleep has been
finished), Downloads are not shown on first NTP at all. They are shown on second
NTP. Not to show anything, if we have incomplete data, feels ok to me.
As my experiment above shows, it probably does not delay showing the NTP.

-    }
-
-    return;
-  }
-
+  // If Offline Page model is not loaded yet, it will process our query once it
+  // has finished loading.
   offline_page_model_->GetPagesMatchingQuery(
       BuildOfflinePageDownloadsQuery(offline_page_model_),
       base::Bind(&DownloadSuggestionsProvider::UpdateOfflinePagesCache,
                  weak_ptr_factory_.GetWeakPtr(), notify));
 }
 
 void DownloadSuggestionsProvider::FetchAssetsDownloads() {
   if (!download_manager_) {
     // The manager has gone down or was explicitly turned off.
     return;
@@ skipping 344 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);
   }
 }