Use bookmark creation date fallback for 6 weeks after installing M54

Use bookmark creation date fallback for 6 weeks after installing M54

Add pref that stores the time the BookmarkSuggestionsProvider was first
started on M54. Enable the creation_date_fallback only if that time is
at most 6 weeks in the past.

Fix minor nits from previous CL 2256643002/#ps20001.

BUG=638538
Committed: https://crrev.com/dc5e1eb66477e2efba986ce9dff6be91258709bd
Cr-Commit-Position: refs/heads/master@{#413193}

[Philipp Keck, 2016-08-19 11:20:22]

pke@google.com changed reviewers:
+ treib@chromium.org

[Philipp Keck, 2016-08-19 11:20:23]

PTAL

[Marc Treib, 2016-08-19 11:41:12]

https://codereview.chromium.org/2256183004/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:36: const
char* kMaxBookmarkAgeInDaysParamName = "ntp_bookmarks_max_age_in_days";
nit: I'd remove the "ntp_", that much is clear from context.

https://codereview.chromium.org/2256183004/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:65:
LOG(WARNING) << "Failed to parse min bookmarks count" << min_count_string;
Add the same above for max_count please

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:91: int64_t
first_M54_start =
nit: Move all this into a helper function? Then you could initialize
creation_date_fallback_ in the initializer list.

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:93: if
(first_M54_start == 0) {
HasPrefPath() might be easier to read than comparing to the default value.

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:100:
time_since_first_M54_start.InDays() < kUseCreationDateFallbackForDays;
InDays() rounds down, so this is not strictly correct. Maybe compute the
difference instead and check if it's positive?

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right):

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:108: //
TODO(jkrcal): Remove this and the pref when it's not needed anymore.
s/when it's not needed anymore/after M55/
Be as specific as possible with this kind of TODO. Chances are the details will
get forgotten, and at some point in the far future someone will wonder if this
thing is still needed.

[Philipp Keck, 2016-08-19 12:49:52]

https://codereview.chromium.org/2256183004/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:36: const
char* kMaxBookmarkAgeInDaysParamName = "ntp_bookmarks_max_age_in_days";
On 2016/08/19 11:41:11, Marc Treib wrote:
> nit: I'd remove the "ntp_", that much is clear from context.

Done.

https://codereview.chromium.org/2256183004/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:65:
LOG(WARNING) << "Failed to parse min bookmarks count" << min_count_string;
On 2016/08/19 11:41:11, Marc Treib wrote:
> Add the same above for max_count please

Done. Also for age_in_days_string (GetThresholdTime()) above.

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:91: int64_t
first_M54_start =
On 2016/08/19 11:41:12, Marc Treib wrote:
> nit: Move all this into a helper function? Then you could initialize
> creation_date_fallback_ in the initializer list.

True. Not sure if the code is best placed in a helper function because that
could be called anytime from anywhere. This code only makes sense to be executed
in the constructor, when the provider is created.

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:93: if
(first_M54_start == 0) {
On 2016/08/19 11:41:12, Marc Treib wrote:
> HasPrefPath() might be easier to read than comparing to the default value.

Done.

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:100:
time_since_first_M54_start.InDays() < kUseCreationDateFallbackForDays;
On 2016/08/19 11:41:11, Marc Treib wrote:
> InDays() rounds down, so this is not strictly correct. Maybe compute the
> difference instead and check if it's positive?

Should be correct as is.

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right):

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:108: //
TODO(jkrcal): Remove this and the pref when it's not needed anymore.
On 2016/08/19 11:41:12, Marc Treib wrote:
> s/when it's not needed anymore/after M55/
> Be as specific as possible with this kind of TODO. Chances are the details
will
> get forgotten, and at some point in the far future someone will wonder if this
> thing is still needed.

Done.

[Marc Treib, 2016-08-19 13:37:17]

lgtm

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:91: int64_t
first_M54_start =
On 2016/08/19 12:49:52, Philipp Keck wrote:
> On 2016/08/19 11:41:12, Marc Treib wrote:
> > nit: Move all this into a helper function? Then you could initialize
> > creation_date_fallback_ in the initializer list.
> 
> True. Not sure if the code is best placed in a helper function because that
> could be called anytime from anywhere. This code only makes sense to be
executed
> in the constructor, when the provider is created.

Well, you could argue the same for any method/function that has only a single
call site, but sometimes it still makes sense to have them. Anyway, your call,
it's also okay as is.

https://codereview.chromium.org/2256183004/diff/40002/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/40002/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:95: int64_t
first_M54_start;
nit: Make this a base::Time (since that's the "real" type) and convert to/from
internal value as necessary?

[Philipp Keck, 2016-08-19 14:27:36]

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:91: int64_t
first_M54_start =
On 2016/08/19 13:37:17, Marc Treib wrote:
> On 2016/08/19 12:49:52, Philipp Keck wrote:
> > On 2016/08/19 11:41:12, Marc Treib wrote:
> > > nit: Move all this into a helper function? Then you could initialize
> > > creation_date_fallback_ in the initializer list.
> > 
> > True. Not sure if the code is best placed in a helper function because that
> > could be called anytime from anywhere. This code only makes sense to be
> executed
> > in the constructor, when the provider is created.
> 
> Well, you could argue the same for any method/function that has only a single
> call site, but sometimes it still makes sense to have them. Anyway, your call,
> it's also okay as is.

My point is that this code is special in that it's not like any method or
function that can move to its only call site -- or not only. An additional
characteristic is that it needs to run when Chrome starts, because it measures
the Time::Now() when M54 is first started. So the constructor of a
service/provider that is created at startup is inherently a good place for this.

But on the other hand, you can argue that it's just as well a good palce for
calling a helper function.

https://codereview.chromium.org/2256183004/diff/40002/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/40002/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:95: int64_t
first_M54_start;
On 2016/08/19 13:37:17, Marc Treib wrote:
> nit: Make this a base::Time (since that's the "real" type) and convert to/from
> internal value as necessary?

Done.

[Philipp Keck, 2016-08-19 14:27:56]

pke@google.com changed reviewers:
+ bauerb@chromium.org

[Philipp Keck, 2016-08-19 14:27:57]

bauerb@chromium.org: Please review changes in 
chrome/browser/prefs/browser_prefs.cc

[Bernhard Bauer, 2016-08-19 16:46:41]

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:142:
DCHECK(bookmarks_for_url.size());
Maybe DCHECK(!bookmarks_for_url.empty())? It would convey the intent a bit
better, I'd hope.

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:95:
base::Time first_M54_start;
Nit: the "m" would still be lowercase.

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:98:
pref_service->GetInt64(prefs::kBookmarksFirstM54Start));
You could just get the value unconditionally and set it to Now() if it's 0.

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:105:
base::Time::Now() - first_M54_start;
OCD-me is a bit concerned about having two separate calls to Now() here :) I
don't think it's likely that the time difference will be more than 42 days, but
it would still be nice if it was exactly 0 if this is the first M54 start.

[Philipp Keck, 2016-08-19 17:15:51]

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:142:
DCHECK(bookmarks_for_url.size());
On 2016/08/19 16:46:41, Bernhard Bauer wrote:
> Maybe DCHECK(!bookmarks_for_url.empty())? It would convey the intent a bit
> better, I'd hope.

Done.

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:95:
base::Time first_M54_start;
On 2016/08/19 16:46:41, Bernhard Bauer wrote:
> Nit: the "m" would still be lowercase.

Done.

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:98:
pref_service->GetInt64(prefs::kBookmarksFirstM54Start));
On 2016/08/19 16:46:41, Bernhard Bauer wrote:
> You could just get the value unconditionally and set it to Now() if it's 0.

:D That's what I did before. Marc argued for this because it's more readable.
For the other solution, you have to know that 0 is the default value (it's
specified in line 119) and then think that "default value returns means didn't
exist before". Normally, it's still readable because the pattern "variable =
getValueMaybe(); if (variable not set) variable = getOtherValue();" is pretty
common.

But in this case I see Marc's point: The else branch does not only set the
variable, it also writes a pref. And that's really logic that's independent of
the variable and the default value. If it was alone (i.e. if we didn't use
first_m54_start afterwards), it should read like this:
if (isFirstStart) setPref("firstStartTime", Now())
And "!HasPrefPath('firstStartTime')" comes pretty close to "isFirstStart", I
think, whereas the whole default value thing does not.

https://codereview.chromium.org/2256183004/diff/70001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:105:
base::Time::Now() - first_M54_start;
On 2016/08/19 16:46:41, Bernhard Bauer wrote:
> OCD-me is a bit concerned about having two separate calls to Now() here :) I
> don't think it's likely that the time difference will be more than 42 days,
but
> it would still be nice if it was exactly 0 if this is the first M54 start.

Done.

[Bernhard Bauer, 2016-08-19 17:37:32]

lgtm

https://codereview.chromium.org/2256183004/diff/90001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2256183004/diff/90001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:207:
DCHECK(bookmarks_for_url.size());
Can you use .empty() here as well?

[Philipp Keck, 2016-08-19 17:43:21]

https://codereview.chromium.org/2256183004/diff/90001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2256183004/diff/90001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:207:
DCHECK(bookmarks_for_url.size());
On 2016/08/19 17:37:32, Bernhard Bauer wrote:
> Can you use .empty() here as well?

Yes, obviously :-)

[Philipp Keck, 2016-08-19 17:44:31]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-19 17:44:31]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2256183004/#ps110001
(title: "Make another DCHECK more readable")

[commit-bot: I haz the power, 2016-08-19 17:45:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 18:35:20]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2016-08-19 18:36:57]

Description was changed from

==========
Use bookmark creation date fallback for 6 weeks after installing M54

Add pref that stores the time the BookmarkSuggestionsProvider was first
started on M54. Enable the creation_date_fallback only if that time is
at most 6 weeks in the past.

Fix minor nits from previous CL 2256643002/#ps20001.

BUG=638538
==========

to

==========
Use bookmark creation date fallback for 6 weeks after installing M54

Add pref that stores the time the BookmarkSuggestionsProvider was first
started on M54. Enable the creation_date_fallback only if that time is
at most 6 weeks in the past.

Fix minor nits from previous CL 2256643002/#ps20001.

BUG=638538

Committed: https://crrev.com/dc5e1eb66477e2efba986ce9dff6be91258709bd
Cr-Commit-Position: refs/heads/master@{#413193}
==========

[commit-bot: I haz the power, 2016-08-19 18:36:58]

Patchset 7 (id:??) landed as
https://crrev.com/dc5e1eb66477e2efba986ce9dff6be91258709bd
Cr-Commit-Position: refs/heads/master@{#413193}