Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2256643002:
Add a fallback to creation date for Recent bookmarks on NTP (Closed)
DescriptionAdd a fallback to creation date for Recent bookmarks on NTP
This fallback is active when we have less bookmarks with last visit info than specified by Finch (defaults to 3). This is relevant for existing users after they update to m54.
In this fallback mode, we
- use creation date instead of (missing) last visit date and
- do not impose age limit on bookmarks to show
BUG=638538
Committed: https://crrev.com/a14bce74b74b8a043608b485dda1cb65191b24da
Cr-Commit-Position: refs/heads/master@{#412782}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Marc's comments #
Total comments: 14
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by jkrcal@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...
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code mostly looks good, but I have some concerns about the overall approach. See comments below. https://codereview.chromium.org/2256643002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right): https://codereview.chromium.org/2256643002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:33: // the transition phase M53 -> M54 when no last visit data is available). Hm, so even in the "fallback mode", wouldn't we still want to use the visit date if we have it? So that users gracefully graduate out of that mode? And I guess we don't want to use the creation date in the regular mode because it can be the date when the bookmark was synced? I.e. on a new device, all bookmarks will have a creation date of "now"? That seems super unfortunate... can we maybe detect that case? Does the bookmark have some "from sync" tag? https://codereview.chromium.org/2256643002/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2256643002/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:213: no_visit_data_ = !IsLastVisitDataAvailable(bookmark_model_); Hm, so this means, a new user will get 3 bookmarks. They visit one of them, and now they only get a single one (because the other two have creation dates more than 6 weeks ago). Is that the behavior we want? I think I'd be happier with some form of graceful fallback, rather than this hard switch between two modes.
The CQ bit was checked by jkrcal@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...
Marc, PTAL, again.
Description was changed from ========== Add a fallback to creation date for Recent bookmarks on NTP This fallback is active when we have no data for last visits of bookmarks (relevant for existing users after they update to m54). In this fallback mode, we - use creation date instead of (missing) last visit date - do not impose age limit on bookmarks to show - show at most 3 bookmarks (Finch configurable). BUG=638538 ========== to ========== Add a fallback to creation date for Recent bookmarks on NTP This fallback is active when we have less bookmarks with last visit info than specified by Finch (defaults to 3). This is relevant for existing users after they update to m54. In this fallback mode, we - use creation date instead of (missing) last visit date and - do not impose age limit on bookmarks to show BUG=638538 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a few nits which can be cleaned up afterwards, so I'll land this! https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right): https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:75: return node->date_added(); nit: Should have braces if the condition doesn't fit on one line https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:144: if (most_recent == bookmarks_for_url.end()) Can this ever happen? I think it can't, so I'd just remove the check. https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:162: // Remove the bookmarks that are not recently visited; we do no need them. nit: we do noT need them https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right): https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:34: bool creation_date_fallback = true); nit: Default parameters are kinda discouraged, I'd prefer to just specify it at each call site. https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:36: const char* kMaxBookmarkAgeInDaysParamName = "max_age_in_days"; Unrelated to this CL, but: AFAIK these param names must be unique within one Finch config (which will span multiple features), so they should probably be a bit more specific - "bookmarks_max_count" etc. https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:62: if (base::StringToInt(min_count_string, &min_count)) nit: If the string is non-empty but fails to parse, we might want to emit a warning.
The CQ bit was checked by treib@chromium.org
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.
Description was changed from ========== Add a fallback to creation date for Recent bookmarks on NTP This fallback is active when we have less bookmarks with last visit info than specified by Finch (defaults to 3). This is relevant for existing users after they update to m54. In this fallback mode, we - use creation date instead of (missing) last visit date and - do not impose age limit on bookmarks to show BUG=638538 ========== to ========== Add a fallback to creation date for Recent bookmarks on NTP This fallback is active when we have less bookmarks with last visit info than specified by Finch (defaults to 3). This is relevant for existing users after they update to m54. In this fallback mode, we - use creation date instead of (missing) last visit date and - do not impose age limit on bookmarks to show BUG=638538 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add a fallback to creation date for Recent bookmarks on NTP This fallback is active when we have less bookmarks with last visit info than specified by Finch (defaults to 3). This is relevant for existing users after they update to m54. In this fallback mode, we - use creation date instead of (missing) last visit date and - do not impose age limit on bookmarks to show BUG=638538 ========== to ========== Add a fallback to creation date for Recent bookmarks on NTP This fallback is active when we have less bookmarks with last visit info than specified by Finch (defaults to 3). This is relevant for existing users after they update to m54. In this fallback mode, we - use creation date instead of (missing) last visit date and - do not impose age limit on bookmarks to show BUG=638538 Committed: https://crrev.com/a14bce74b74b8a043608b485dda1cb65191b24da Cr-Commit-Position: refs/heads/master@{#412782} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a14bce74b74b8a043608b485dda1cb65191b24da Cr-Commit-Position: refs/heads/master@{#412782}
Message was sent while issue was closed.
pke@google.com changed reviewers: + pke@google.com
Message was sent while issue was closed.
The comments below are fixed here: https://codereview.chromium.org/2256183004/#ps1 https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right): https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:75: return node->date_added(); On 2016/08/18 09:31:44, Marc Treib wrote: > nit: Should have braces if the condition doesn't fit on one line Done. https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:144: if (most_recent == bookmarks_for_url.end()) On 2016/08/18 09:31:44, Marc Treib wrote: > Can this ever happen? I think it can't, so I'd just remove the check. Right, done. We could think about adding DCHECK(bookmarks_for_url.size()) above, but that shouldn't be necessary either (unless we consider concurrent updates to the bookmark model?). https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:162: // Remove the bookmarks that are not recently visited; we do no need them. On 2016/08/18 09:31:44, Marc Treib wrote: > nit: we do noT need them Done. https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right): https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:34: bool creation_date_fallback = true); On 2016/08/18 09:31:44, Marc Treib wrote: > nit: Default parameters are kinda discouraged, I'd prefer to just specify it at > each call site. Done. https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:36: const char* kMaxBookmarkAgeInDaysParamName = "max_age_in_days"; On 2016/08/18 09:31:44, Marc Treib wrote: > Unrelated to this CL, but: AFAIK these param names must be unique within one > Finch config (which will span multiple features), so they should probably be a > bit more specific - "bookmarks_max_count" etc. Done. https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:62: if (base::StringToInt(min_count_string, &min_count)) On 2016/08/18 09:31:44, Marc Treib wrote: > nit: If the string is non-empty but fails to parse, we might want to emit a > warning. Done.
Message was sent while issue was closed.
https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right): https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:144: if (most_recent == bookmarks_for_url.end()) On 2016/08/19 11:20:54, Philipp Keck wrote: > On 2016/08/18 09:31:44, Marc Treib wrote: > > Can this ever happen? I think it can't, so I'd just remove the check. > > Right, done. We could think about adding DCHECK(bookmarks_for_url.size()) above, > but that shouldn't be necessary either (unless we consider concurrent updates to > the bookmark model?). Well, a DCHECK is never stricly necessary, it just documents some pre-condition or assumption. So it might make sense to add one here.
Message was sent while issue was closed.
https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right): https://codereview.chromium.org/2256643002/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:144: if (most_recent == bookmarks_for_url.end()) On 2016/08/19 11:25:41, Marc Treib wrote: > On 2016/08/19 11:20:54, Philipp Keck wrote: > > On 2016/08/18 09:31:44, Marc Treib wrote: > > > Can this ever happen? I think it can't, so I'd just remove the check. > > > > Right, done. We could think about adding DCHECK(bookmarks_for_url.size()) > above, > > but that shouldn't be necessary either (unless we consider concurrent updates > to > > the bookmark model?). > > Well, a DCHECK is never stricly necessary, it just documents some pre-condition > or assumption. So it might make sense to add one here. Done. https://codereview.chromium.org/2256183004/diff2/20001:40001/components/ntp_s... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
