Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2673633002:
[NTP::Downloads] Do not show old downloads. (Closed)
Description[NTP::Downloads] Do not show old downloads.
Add a variation parameter to choose what is considered "old" (by default
6 weeks) and not show downloads (both assets and offline pages) older
than that.
Reasoning: the section is meant for quick access, so if the user happens
to have only old downloads, there is no point to show them.
BUG=687888
Review-Url: https://codereview.chromium.org/2673633002
Cr-Commit-Position: refs/heads/master@{#448596}
Committed: https://chromium.googlesource.com/chromium/src/+/6dc672d93b3705ea017b2f538daa73408d970f7a
Patch Set 1 : #
Total comments: 11
Patch Set 2 : rebase. #Patch Set 3 : rebase. #Patch Set 4 : comments + clean rebase (sorry). #
Dependent Patchsets: Messages
Total messages: 24 (16 generated)
Patchset #1 (id:1) has been deleted
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: + tschumann@chromium.org
Hi tschumann@, PTAL. https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:736: if (!IsDownloadOld(GetOfflinePagePublishedTime(item))) { tschumann@, do we want to use last access time for offline pages already? I believe yes, but I feel like doing it in a separate CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:78: assets_enabled ? features::kAssetDownloadSuggestionsFeature +Jan, can you double-check this is sane? It's the first time I see a variation parameter being bound to different features? Does that mean we need to be very careful how to roll out experiments? Or do we need to set both? https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:735: if (!old_dismissed_ids.count(id_within_category)) { nit: It's now harder to get the context of the the } else { part. inverse? Maybe even split up into single, positive ifs: if (old_dismissed_ids.count()) { retained_dismissed_ids.insert(); continue; } if (IsDownloadOld(...))) { continue; } items.push_back(...); https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:736: if (!IsDownloadOld(GetOfflinePagePublishedTime(item))) { On 2017/02/02 14:06:55, vitaliii wrote: > tschumann@, do we want to use last access time for offline pages already? > I believe yes, but I feel like doing it in a separate CL. i'm fine doing this in one CL. https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:145: bool IsDownloadOld(const base::Time& published_time); 'old' is a bit short on semantics. Should we call it 'expired', 'stale', or 'outdated'? https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:1074: UnorderedElementsAre(HasUrl("http://dummy.com/1"), just as an FYI (no need to fix now). This is a good example about how moving too much logic into helper method makes it hard to reason about the test. When reading the test, I have no idea where the expected URLs are coming from and why I should expect those and not others. (especially that the strings are not even in this file...).
jkrcal@chromium.org changed reviewers: + jkrcal@chromium.org
https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:78: assets_enabled ? features::kAssetDownloadSuggestionsFeature On 2017/02/02 14:30:33, tschumann wrote: > +Jan, can you double-check this is sane? > > It's the first time I see a variation parameter being bound to different > features? Does that mean we need to be very careful how to roll out experiments? > Or do we need to set both? After offline discussion, this is sane, please add a comment to explain.
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
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...
Hi jkrcal@, I addressed the comments. Could you have a look as an OWNER of components/ntp_snippets? https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:78: assets_enabled ? features::kAssetDownloadSuggestionsFeature On 2017/02/02 14:30:33, tschumann wrote: > +Jan, can you double-check this is sane? > > It's the first time I see a variation parameter being bound to different > features? Does that mean we need to be very careful how to roll out experiments? > Or do we need to set both? Done. I added comments. |GetVariationParamByFeatureAsInt| returns std::string(), if the feature is turned off. Since one of the features may be disabled, we choose the enabled one. For the experiments, setting both sounds like the safest option. https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:735: if (!old_dismissed_ids.count(id_within_category)) { On 2017/02/02 14:30:33, tschumann wrote: > nit: It's now harder to get the context of the the } else { part. > inverse? > Maybe even split up into single, positive ifs: > if (old_dismissed_ids.count()) { > retained_dismissed_ids.insert(); > continue; > } > if (IsDownloadOld(...))) { > continue; > } > items.push_back(...); Done. I've inversed the outer |if|. https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:736: if (!IsDownloadOld(GetOfflinePagePublishedTime(item))) { On 2017/02/02 14:30:33, tschumann wrote: > On 2017/02/02 14:06:55, vitaliii wrote: > > tschumann@, do we want to use last access time for offline pages already? > > I believe yes, but I feel like doing it in a separate CL. > > i'm fine doing this in one CL. Acknowledged. https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:145: bool IsDownloadOld(const base::Time& published_time); On 2017/02/02 14:30:34, tschumann wrote: > 'old' is a bit short on semantics. > Should we call it 'expired', 'stale', or 'outdated'? Done. "Outdated"
Hi jkrcal@, I addressed the comments. Could you have a look as an OWNER of components/ntp_snippets? https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:78: assets_enabled ? features::kAssetDownloadSuggestionsFeature On 2017/02/02 14:30:33, tschumann wrote: > +Jan, can you double-check this is sane? > > It's the first time I see a variation parameter being bound to different > features? Does that mean we need to be very careful how to roll out experiments? > Or do we need to set both? Done. I added comments. |GetVariationParamByFeatureAsInt| returns std::string(), if the feature is turned off. Since one of the features may be disabled, we choose the enabled one. For the experiments, setting both sounds like the safest option. https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:735: if (!old_dismissed_ids.count(id_within_category)) { On 2017/02/02 14:30:33, tschumann wrote: > nit: It's now harder to get the context of the the } else { part. > inverse? > Maybe even split up into single, positive ifs: > if (old_dismissed_ids.count()) { > retained_dismissed_ids.insert(); > continue; > } > if (IsDownloadOld(...))) { > continue; > } > items.push_back(...); Done. I've inversed the outer |if|. https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:736: if (!IsDownloadOld(GetOfflinePagePublishedTime(item))) { On 2017/02/02 14:30:33, tschumann wrote: > On 2017/02/02 14:06:55, vitaliii wrote: > > tschumann@, do we want to use last access time for offline pages already? > > I believe yes, but I feel like doing it in a separate CL. > > i'm fine doing this in one CL. Acknowledged. https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2673633002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:145: bool IsDownloadOld(const base::Time& published_time); On 2017/02/02 14:30:34, tschumann wrote: > 'old' is a bit short on semantics. > Should we call it 'expired', 'stale', or 'outdated'? Done. "Outdated"
lgtm
The CQ bit was unchecked by vitaliii@chromium.org
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2673633002/#ps120001 (title: "comments + clean rebase (sorry).")
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": 120001, "attempt_start_ts": 1486463663223510,
"parent_rev": "58831855e1f0366cf7a9eae129c6d10b711d3195", "commit_rev":
"6dc672d93b3705ea017b2f538daa73408d970f7a"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Downloads] Do not show old downloads. Add a variation parameter to choose what is considered "old" (by default 6 weeks) and not show downloads (both assets and offline pages) older than that. Reasoning: the section is meant for quick access, so if the user happens to have only old downloads, there is no point to show them. BUG=687888 ========== to ========== [NTP::Downloads] Do not show old downloads. Add a variation parameter to choose what is considered "old" (by default 6 weeks) and not show downloads (both assets and offline pages) older than that. Reasoning: the section is meant for quick access, so if the user happens to have only old downloads, there is no point to show them. BUG=687888 Review-Url: https://codereview.chromium.org/2673633002 Cr-Commit-Position: refs/heads/master@{#448596} Committed: https://chromium.googlesource.com/chromium/src/+/6dc672d93b3705ea017b2f538daa... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/6dc672d93b3705ea017b2f538daa... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
