Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562073002:
[NTP::Downloads] Limit number of dismissed IDs instead of pruning. (Closed)
|
Created:
4 years ago by vitaliii Modified:
4 years ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP::Downloads] Limit number of dismissed IDs instead of pruning.
Recently we discovered, that our dismissed IDs pruning may happen at a
wrong time, when DownloadManager does not have a complete list.
Unfortunately, there is no way to check whether DownloadManager
finished starting up.
Therefore, this CL removes pruning of asset downloads and instead
limits maximal number of stored IDs to 100 (the oldest are removed
once the threshold is reached).
BUG=672758
Committed: https://crrev.com/13f871dfc1ee4f80880c5f779f32b1c1da79a5c4
Cr-Commit-Position: refs/heads/master@{#437882}
Patch Set 1 : . #
Total comments: 10
Patch Set 2 : treib@ comments. #Patch Set 3 : rebase + test. #
Total comments: 10
Patch Set 4 : rebase & treib@ comments. #
Messages
Total messages: 35 (27 generated)
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi treib@, PTAL.
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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:54: const int kMaxDismissedIdNum = 100; nit: s/Num/Count/ (IMO that's much more common) https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:296: if (std::find(dismissed_ids.begin(), dismissed_ids.end(), id) != optional: base::ContainsValue (from stl_util.h) is IMO a bit more readable. https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:297: dismissed_ids.end()) { Hm, so this is now O(n*m) instead of O((n+m)*log(n)) (n = number of dismissed, m = number of downloads). I *guess* that's okay? https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:175: std::list<std::string> ReadAssetDismissedIDsFromPrefs() const; vector? No need to be super inefficient and use linked lists :) https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:978: EXPECT_THAT(GetDismissedSuggestions(), SizeIs(Lt(ids.size()))); You could expose the actual max count (e.g. via a static GetMaxDismissedCountForTesting).
Patchset #2 (id:40001) 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 treib@, I addressed your comments. PTAL. https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:54: const int kMaxDismissedIdNum = 100; On 2016/12/09 16:37:26, Marc Treib wrote: > nit: s/Num/Count/ (IMO that's much more common) Done. https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:296: if (std::find(dismissed_ids.begin(), dismissed_ids.end(), id) != On 2016/12/09 16:37:26, Marc Treib wrote: > optional: base::ContainsValue (from stl_util.h) is IMO a bit more readable. Done. Thanks, it is definitely more readable. I was trying to find it, but did not manage, which is funny, because it was already used in the same file. https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:297: dismissed_ids.end()) { On 2016/12/09 16:37:26, Marc Treib wrote: > Hm, so this is now O(n*m) instead of O((n+m)*log(n)) (n = number of dismissed, m > = number of downloads). I *guess* that's okay? This is fine, because this method is used only in tests. Everywhere else we never check multiple items (since we do not prune anymore). https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:175: std::list<std::string> ReadAssetDismissedIDsFromPrefs() const; On 2016/12/09 16:37:26, Marc Treib wrote: > vector? > No need to be super inefficient and use linked lists :) Done. https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2562073002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:978: EXPECT_THAT(GetDismissedSuggestions(), SizeIs(Lt(ids.size()))); On 2016/12/09 16:37:26, Marc Treib wrote: > You could expose the actual max count (e.g. via a static > GetMaxDismissedCountForTesting). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a few more nits https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:14: #include "base/stl_util.h" Included twice now https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:302: std::vector<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs(); nit: The other scope also as a |dismissed_ids| variable - please give this one a different name. https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:722: // TODO(vitaliii): Do not use std::list once ensure pruning at the right time. vector now :) Also, this sentence doesn't grammar (or I just don't get it) https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:989: i < DownloadSuggestionsProvider::GetMaxDismissedCountForTesting() + 1; optional: Could store this in a variable first, to make the loop a bit more readable. https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:1007: // The oldest dismissed suggestion must become undismissed now. Maybe mention that this is kind of an "anti-test"? I.e. it documents the current behavior, but it's really not the behavior we'd want.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I addressed your nits, no need to look. https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:14: #include "base/stl_util.h" On 2016/12/12 09:29:15, Marc Treib wrote: > Included twice now Done. https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:302: std::vector<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs(); On 2016/12/12 09:29:15, Marc Treib wrote: > nit: The other scope also as a |dismissed_ids| variable - please give this one a > different name. Done. I renamed both. https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:722: // TODO(vitaliii): Do not use std::list once ensure pruning at the right time. On 2016/12/12 09:29:15, Marc Treib wrote: > vector now :) > Also, this sentence doesn't grammar (or I just don't get it) Done. https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:989: i < DownloadSuggestionsProvider::GetMaxDismissedCountForTesting() + 1; On 2016/12/12 09:29:15, Marc Treib wrote: > optional: Could store this in a variable first, to make the loop a bit more > readable. Done. https://codereview.chromium.org/2562073002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:1007: // The oldest dismissed suggestion must become undismissed now. On 2016/12/12 09:29:15, Marc Treib wrote: > Maybe mention that this is kind of an "anti-test"? I.e. it documents the current > behavior, but it's really not the behavior we'd want. Done.
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2562073002/#ps100001 (title: "rebase & treib@ comments.")
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": 100001, "attempt_start_ts": 1481557326585220,
"parent_rev": "597fb9e53681251262cbc7e9556b0fd2ac2e878a", "commit_rev":
"7dba895035d05651922147b8b75442e0253d9bbb"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481557326585220,
"parent_rev": "20ce4f5e64cb1046d1f045e3e3059bdff4bdd197", "commit_rev":
"e90fada4ed4acd7eb50a012614bd89254c430649"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Downloads] Limit number of dismissed IDs instead of pruning. Recently we discovered, that our dismissed IDs pruning may happen at a wrong time, when DownloadManager does not have a complete list. Unfortunately, there is no way to check whether DownloadManager finished starting up. Therefore, this CL removes pruning of asset downloads and instead limits maximal number of stored IDs to 100 (the oldest are removed once the threshold is reached). BUG=672758 ========== to ========== [NTP::Downloads] Limit number of dismissed IDs instead of pruning. Recently we discovered, that our dismissed IDs pruning may happen at a wrong time, when DownloadManager does not have a complete list. Unfortunately, there is no way to check whether DownloadManager finished starting up. Therefore, this CL removes pruning of asset downloads and instead limits maximal number of stored IDs to 100 (the oldest are removed once the threshold is reached). BUG=672758 Review-Url: https://codereview.chromium.org/2562073002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [NTP::Downloads] Limit number of dismissed IDs instead of pruning. Recently we discovered, that our dismissed IDs pruning may happen at a wrong time, when DownloadManager does not have a complete list. Unfortunately, there is no way to check whether DownloadManager finished starting up. Therefore, this CL removes pruning of asset downloads and instead limits maximal number of stored IDs to 100 (the oldest are removed once the threshold is reached). BUG=672758 Review-Url: https://codereview.chromium.org/2562073002 ========== to ========== [NTP::Downloads] Limit number of dismissed IDs instead of pruning. Recently we discovered, that our dismissed IDs pruning may happen at a wrong time, when DownloadManager does not have a complete list. Unfortunately, there is no way to check whether DownloadManager finished starting up. Therefore, this CL removes pruning of asset downloads and instead limits maximal number of stored IDs to 100 (the oldest are removed once the threshold is reached). BUG=672758 Committed: https://crrev.com/13f871dfc1ee4f80880c5f779f32b1c1da79a5c4 Cr-Commit-Position: refs/heads/master@{#437882} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/13f871dfc1ee4f80880c5f779f32b1c1da79a5c4 Cr-Commit-Position: refs/heads/master@{#437882} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
