Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683383002:
[NTP:Downloads] Request offline pages even if model is not loaded. (Closed)
|
Created:
3 years, 10 months ago by vitaliii Modified:
3 years, 10 months ago Reviewers:
Marc Treib CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP:Downloads] Request offline pages even if model is not loaded.
Do not check whether Offline Pages model has been loaded when requesting
offline pages. Currently the model simply collects all requests and
answers them when it is loaded. In this CL Downloads provider waits for
the model to answer instead of not showing offline pages on startup.
BUG=690391
Review-Url: https://codereview.chromium.org/2683383002
Cr-Commit-Position: refs/heads/master@{#449603}
Committed: https://chromium.googlesource.com/chromium/src/+/600636adf773150653cf033c94649a75f7d3b2fb
Patch Set 1 #
Total comments: 7
Messages
Total messages: 16 (8 generated)
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi treib@, Could you have a look please?
lgtm https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (left): https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:498: SubmitContentSuggestions(); 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? https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (left): https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:859: offline_pages_model()->set_is_loaded(true); I guess this just isn't required anymore; it wouldn't hurt to keep it there?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi treib@, I addressed your comments, PTAL. https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (left): https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:498: SubmitContentSuggestions(); On 2017/02/10 09:08:11, Marc Treib wrote: > So the change is essentially to *not* notify here? > ...okay, I guess that's reasonable. The change is to query even if the model has not been loaded yet. > What happens if Offline Pages takes a looong > time to load for some reason? 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/). https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (left): https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:859: offline_pages_model()->set_is_loaded(true); On 2017/02/10 09:08:11, Marc Treib wrote: > I guess this just isn't required anymore; it wouldn't hurt to keep it there? No, it wouldn't. Anyway is_loaded = true by default, so that line had no effect except making this explicit to the reader. Since we do not use it in our code, I though that there is no point to notify the reader about it. Also dewittj@ was going to make is_loaded private in https://codereview.chromium.org/2564163002/.
https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (left): https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:498: SubmitContentSuggestions(); On 2017/02/10 09:39:56, vitaliii wrote: > On 2017/02/10 09:08:11, Marc Treib wrote: > > So the change is essentially to *not* notify here? > > ...okay, I guess that's reasonable. > > The change is to query even if the model has not been loaded yet. 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. > > What happens if Offline Pages takes a looong > > time to load for some reason? > > 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. 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... > dewittj@ talked about 200ms median load time (see > https://codereview.chromium.org/2564163002/). 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. https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (left): https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:859: offline_pages_model()->set_is_loaded(true); On 2017/02/10 09:39:56, vitaliii wrote: > On 2017/02/10 09:08:11, Marc Treib wrote: > > I guess this just isn't required anymore; it wouldn't hurt to keep it there? > > No, it wouldn't. > Anyway is_loaded = true by default, so that line had no effect except making > this explicit to the reader. Since we do not use it in our code, I though that > there is no point to notify the reader about it. > Also dewittj@ was going to make is_loaded private in > https://codereview.chromium.org/2564163002/. Acknowledged.
Hi treib@, PTAL. https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (left): https://codereview.chromium.org/2683383002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:498: SubmitContentSuggestions(); On 2017/02/10 10:05:02, Marc Treib wrote: > On 2017/02/10 09:39:56, vitaliii wrote: > > On 2017/02/10 09:08:11, Marc Treib wrote: > > > So the change is essentially to *not* notify here? > > > ...okay, I guess that's reasonable. > > > > The change is to query even if the model has not been loaded yet. > > 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. But previously we would show incomplete list (without Offline Pages). Showing nothing feels better. > > > > What happens if Offline Pages takes a looong > > > time to load for some reason? > > > > 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. > > 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... 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. > > dewittj@ talked about 200ms median load time (see > > https://codereview.chromium.org/2564163002/). > > 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. As my experiment above shows, it probably does not delay showing the NTP.
lgtm Ack
The CQ bit was checked by vitaliii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1486732727615810, "parent_rev":
"fc75ccee5fbb2fb80fe9981b38ea122180500c43", "commit_rev":
"600636adf773150653cf033c94649a75f7d3b2fb"}
Message was sent while issue was closed.
Description was changed from ========== [NTP:Downloads] Request offline pages even if model is not loaded. Do not check whether Offline Pages model has been loaded when requesting offline pages. Currently the model simply collects all requests and answers them when it is loaded. In this CL Downloads provider waits for the model to answer instead of not showing offline pages on startup. BUG=690391 ========== to ========== [NTP:Downloads] Request offline pages even if model is not loaded. Do not check whether Offline Pages model has been loaded when requesting offline pages. Currently the model simply collects all requests and answers them when it is loaded. In this CL Downloads provider waits for the model to answer instead of not showing offline pages on startup. BUG=690391 Review-Url: https://codereview.chromium.org/2683383002 Cr-Commit-Position: refs/heads/master@{#449603} Committed: https://chromium.googlesource.com/chromium/src/+/600636adf773150653cf033c9464... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/600636adf773150653cf033c9464... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
