Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555013003:
[Offline Pages] Convert Download Suggestions Provider to using OfflinePageAdded (Closed)
Description[Offline Pages] Convert Download Suggestions Provider to using OfflinePageAdded
Recently OfflinePageModelChanged was renamed to OfflinePageAdded to reflect
the reality of when that function was being called. As a result, the
OfflinePageAdded function now receives as a parameter the OfflinePageItem
that was added. This allows for a much simpler observer function that
directly operates on the new page in the download cache.
BUG=638276
Committed: https://crrev.com/804d3453c04c1caf0bf66cd73d5e2ae36dde7ae7
Cr-Commit-Position: refs/heads/master@{#437616}
Patch Set 1 #Patch Set 2 : Simplify #Patch Set 3 : Refactor out the creation time test. #Patch Set 4 : rebase #
Total comments: 13
Patch Set 5 : vitaliii comments. #Patch Set 6 : One more comment. #
Total comments: 4
Patch Set 7 : Prevent crash #Messages
Total messages: 33 (23 generated)
The CQ bit was checked by dewittj@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...
The CQ bit was checked by dewittj@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dewittj@chromium.org changed reviewers: + treib@chromium.org, vitaliii@chromium.org
PTAL, thanks! Note that the new routine does somewhat less than AsynchronouslyFetchOfflinePagesDownloads; I think the main behavior change is that the list of swiped away pages will not get pruned when OfflinePageAdded is called. Is that okay?
Hi Justin, It is fine not to prune the dismissed ids in OfflinePageAdded notification, assuming that we should get OfflinePage deleted notification anyway. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:51: bool CompareOfflinePagesForSorting(const OfflinePageItem& left, Could you have this consistent (naming, struct) with the struct |OrderDownloadsMostRecentlyDownloadedFirst| below? I would call it |OrderOfflinePagesMostRecentlyCreatedFirst| or call both |Compare.*| instead of |Order.*|. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:314: void DownloadSuggestionsProvider::OfflinePageAdded( Do I understand right that OfflinePage can be either deleted or added, but not updated? If I am right, this feels a bit strange, since you have last accessed time field, which may be of interest to someone. Previously my assumption was that model change included this. As far as I am aware we do not use it in Zine as of now, so this is rather checking out, but not a call to action. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:323: cached_offline_page_downloads_.push_back(added_page); You also need to check that this offline page is not dismissed (see |CacheAssetDownloadIfNeeded| for an example and |ReadOfflinePageDismissedIDsFromPrefs| for a source of dismissed ids). I was impressed that this passed our tests, so I will have a good look at them later. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:331: cached_offline_page_downloads_.erase(oldest_page_iterator); Could you instead do (*oldest_page_iterator) = added_page if the oldest page is older than the new one and not to add it in advance? This would omit erasing and do nothing if there is no need to add the page.
Some elaborations on my comments. Sorry for not noticing this immediately. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:51: bool CompareOfflinePagesForSorting(const OfflinePageItem& left, On 2016/12/08 17:40:32, vitaliii wrote: > Could you have this consistent (naming, struct) with the struct > |OrderDownloadsMostRecentlyDownloadedFirst| below? > I would call it |OrderOfflinePagesMostRecentlyCreatedFirst| or call both > |Compare.*| instead of |Order.*|. Actually, I understood that then the struct is ugly to use as a function, so please make both comparators a function and use |Compare| as a prefix. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:323: cached_offline_page_downloads_.push_back(added_page); On 2016/12/08 17:40:32, vitaliii wrote: > You also need to check that this offline page is not dismissed (see > |CacheAssetDownloadIfNeeded| for an example and > |ReadOfflinePageDismissedIDsFromPrefs| for a source of dismissed ids). > I was impressed that this passed our tests, so I will have a good look at them > later. Ok, technically you do not need to check this unless we get offline paged added twice without a removal in between. So please have a DCHECK(!ReadOfflinePageDismissedIDsFromPrefs().count(...)) instead to check that this page is not dismissed. Sorry for changing my mind.
The CQ bit was checked by dewittj@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...
The CQ bit was checked by dewittj@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...
Thanks! https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:51: bool CompareOfflinePagesForSorting(const OfflinePageItem& left, On 2016/12/08 17:40:32, vitaliii wrote: > Could you have this consistent (naming, struct) with the struct > |OrderDownloadsMostRecentlyDownloadedFirst| below? > I would call it |OrderOfflinePagesMostRecentlyCreatedFirst| or call both > |Compare.*| instead of |Order.*|. Done. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:51: bool CompareOfflinePagesForSorting(const OfflinePageItem& left, On 2016/12/08 18:17:20, vitaliii wrote: > On 2016/12/08 17:40:32, vitaliii wrote: > > Could you have this consistent (naming, struct) with the struct > > |OrderDownloadsMostRecentlyDownloadedFirst| below? > > I would call it |OrderOfflinePagesMostRecentlyCreatedFirst| or call both > > |Compare.*| instead of |Order.*|. > > Actually, I understood that then the struct is ugly to use as a function, so > please make both comparators a function and use |Compare| as a prefix. Done. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:314: void DownloadSuggestionsProvider::OfflinePageAdded( On 2016/12/08 17:40:32, vitaliii wrote: > Do I understand right that OfflinePage can be either deleted or added, but not > updated? > If I am right, this feels a bit strange, since you have last accessed time > field, which may be of interest to someone. Previously my assumption was that > model change included this. As far as I am aware we do not use it in Zine as of > now, so this is rather checking out, but not a call to action. Currently pages can be accessed (and thus altered) but there is no observer event fired when that happens. I am looking into adding an updated event. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:323: cached_offline_page_downloads_.push_back(added_page); On 2016/12/08 17:40:32, vitaliii wrote: > You also need to check that this offline page is not dismissed (see > |CacheAssetDownloadIfNeeded| for an example and > |ReadOfflinePageDismissedIDsFromPrefs| for a source of dismissed ids). > I was impressed that this passed our tests, so I will have a good look at them > later. I didn't add that check because I assumed that new pages could not possibly already be dismissed. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:323: cached_offline_page_downloads_.push_back(added_page); On 2016/12/08 18:17:20, vitaliii wrote: > On 2016/12/08 17:40:32, vitaliii wrote: > > You also need to check that this offline page is not dismissed (see > > |CacheAssetDownloadIfNeeded| for an example and > > |ReadOfflinePageDismissedIDsFromPrefs| for a source of dismissed ids). > > I was impressed that this passed our tests, so I will have a good look at them > > later. > > Ok, technically you do not need to check this unless we get offline paged added > twice without a removal in between. So please have a > DCHECK(!ReadOfflinePageDismissedIDsFromPrefs().count(...)) > instead to check that this page is not dismissed. > Sorry for changing my mind. Done. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:331: cached_offline_page_downloads_.erase(oldest_page_iterator); On 2016/12/08 17:40:32, vitaliii wrote: > Could you instead do > (*oldest_page_iterator) = added_page > if the oldest page is older than the new one and not to add it in advance? > This would omit erasing and do nothing if there is no need to add the page. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a nit. You still need an owner approval from treib@. https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2555013003/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:314: void DownloadSuggestionsProvider::OfflinePageAdded( On 2016/12/08 19:09:02, dewittj wrote: > On 2016/12/08 17:40:32, vitaliii wrote: > > Do I understand right that OfflinePage can be either deleted or added, but not > > updated? > > If I am right, this feels a bit strange, since you have last accessed time > > field, which may be of interest to someone. Previously my assumption was that > > model change included this. As far as I am aware we do not use it in Zine as > of > > now, so this is rather checking out, but not a call to action. > > Currently pages can be accessed (and thus altered) but there is no observer > event fired when that happens. I am looking into adding an updated event. Acknowledged. https://codereview.chromium.org/2555013003/diff/100001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2555013003/diff/100001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:324: DCHECK_EQ(ReadOfflinePageDismissedIDsFromPrefs().count( nit: Why not "DCHECK(!..."?
Sorry for the delay! LGTM with one more comment. https://codereview.chromium.org/2555013003/diff/100001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2555013003/diff/100001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:336: *oldest_page_iterator = added_page; This could crash if GetMaxSuggestionsCount ever returned 0.
The CQ bit was checked by dewittj@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...
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
The CQ bit was unchecked by dewittj@chromium.org
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, vitaliii@chromium.org Link to the patchset: https://codereview.chromium.org/2555013003/#ps140001 (title: "Prevent crash")
thanks! https://codereview.chromium.org/2555013003/diff/100001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2555013003/diff/100001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:324: DCHECK_EQ(ReadOfflinePageDismissedIDsFromPrefs().count( On 2016/12/09 10:02:37, vitaliii wrote: > nit: Why not "DCHECK(!..."? Feels more correct to compare to an integer rather than implicitly convert to bool. https://codereview.chromium.org/2555013003/diff/100001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:336: *oldest_page_iterator = added_page; On 2016/12/09 15:34:34, Marc Treib wrote: > This could crash if GetMaxSuggestionsCount ever returned 0. Done.
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": 140001, "attempt_start_ts": 1481309029837610,
"parent_rev": "c44e249ed3572fafb3fa9be32c630483aba27f51", "commit_rev":
"dff82afef7c099ce0ef008a3c79cba9d6540188d"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Convert Download Suggestions Provider to using OfflinePageAdded Recently OfflinePageModelChanged was renamed to OfflinePageAdded to reflect the reality of when that function was being called. As a result, the OfflinePageAdded function now receives as a parameter the OfflinePageItem that was added. This allows for a much simpler observer function that directly operates on the new page in the download cache. BUG=638276 ========== to ========== [Offline Pages] Convert Download Suggestions Provider to using OfflinePageAdded Recently OfflinePageModelChanged was renamed to OfflinePageAdded to reflect the reality of when that function was being called. As a result, the OfflinePageAdded function now receives as a parameter the OfflinePageItem that was added. This allows for a much simpler observer function that directly operates on the new page in the download cache. BUG=638276 Review-Url: https://codereview.chromium.org/2555013003 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Convert Download Suggestions Provider to using OfflinePageAdded Recently OfflinePageModelChanged was renamed to OfflinePageAdded to reflect the reality of when that function was being called. As a result, the OfflinePageAdded function now receives as a parameter the OfflinePageItem that was added. This allows for a much simpler observer function that directly operates on the new page in the download cache. BUG=638276 Review-Url: https://codereview.chromium.org/2555013003 ========== to ========== [Offline Pages] Convert Download Suggestions Provider to using OfflinePageAdded Recently OfflinePageModelChanged was renamed to OfflinePageAdded to reflect the reality of when that function was being called. As a result, the OfflinePageAdded function now receives as a parameter the OfflinePageItem that was added. This allows for a much simpler observer function that directly operates on the new page in the download cache. BUG=638276 Committed: https://crrev.com/804d3453c04c1caf0bf66cd73d5e2ae36dde7ae7 Cr-Commit-Position: refs/heads/master@{#437616} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/804d3453c04c1caf0bf66cd73d5e2ae36dde7ae7 Cr-Commit-Position: refs/heads/master@{#437616} |
