Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2275613002:
Bookmark Suggestions: Make 'creation date fallback' time configurable (Closed)
|
Created:
4 years, 4 months ago by Marc Treib Modified:
4 years, 4 months ago Reviewers:
sfiera CC:
chromium-reviews, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@fix_proguard Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBookmark Suggestions: Make 'creation date fallback' time configurable
BUG=638538
Committed: https://crrev.com/ea3816100645db1ab7c5e85ea52b5e5230264502
Cr-Commit-Position: refs/heads/master@{#413766}
Patch Set 1 #
Total comments: 4
Patch Set 2 : comment #Messages
Total messages: 14 (6 generated)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
treib@chromium.org changed reviewers: + sfiera@chromium.org
PTAL!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2275613002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2275613002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:123: time_since_first_m54_start.InDays() < UseCreationDateFallbackForDays(); Seems like setting "0" (or negative) would have the effect of disabling fallback. Intentional? Do you want to document that?
https://codereview.chromium.org/2275613002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2275613002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:123: time_since_first_m54_start.InDays() < UseCreationDateFallbackForDays(); On 2016/08/23 15:31:14, sfiera wrote: > Seems like setting "0" (or negative) would have the effect of disabling > fallback. Intentional? Do you want to document that? That's exactly the behavior you'd expect, no? Use the fallback for 0 days?
https://codereview.chromium.org/2275613002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2275613002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:123: time_since_first_m54_start.InDays() < UseCreationDateFallbackForDays(); On 2016/08/23 15:35:19, Marc Treib wrote: > On 2016/08/23 15:31:14, sfiera wrote: > > Seems like setting "0" (or negative) would have the effect of disabling > > fallback. Intentional? Do you want to document that? > > That's exactly the behavior you'd expect, no? Use the fallback for 0 days? I might expect that "0" would mean "use the compiled default".
https://codereview.chromium.org/2275613002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2275613002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:123: time_since_first_m54_start.InDays() < UseCreationDateFallbackForDays(); On 2016/08/23 15:42:04, sfiera wrote: > On 2016/08/23 15:35:19, Marc Treib wrote: > > On 2016/08/23 15:31:14, sfiera wrote: > > > Seems like setting "0" (or negative) would have the effect of disabling > > > fallback. Intentional? Do you want to document that? > > > > That's exactly the behavior you'd expect, no? Use the fallback for 0 days? > > I might expect that "0" would mean "use the compiled default". I'd expect a comment if it meant that! Anyway, comment added.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2275613002/#ps20001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Bookmark Suggestions: Make 'creation date fallback' time configurable BUG=638538 ========== to ========== Bookmark Suggestions: Make 'creation date fallback' time configurable BUG=638538 Committed: https://crrev.com/ea3816100645db1ab7c5e85ea52b5e5230264502 Cr-Commit-Position: refs/heads/master@{#413766} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ea3816100645db1ab7c5e85ea52b5e5230264502 Cr-Commit-Position: refs/heads/master@{#413766} |
