[New CL] Add a tab helper to record the last visit date for each bookmark

Add a tab helper to record the last visit date for each bookmark

The NTP on Android will display a list of recently visited bookmarks.
This CL introduces a tab helper that stores current date to the meta
data of a bookmark when the bookmark gets visited.

The code is split into a part that depends on content (living in
chrome/browser/ntp_snippets) and a platform-independent part
(living in components/ntp_snippets).

The platform independent code also provides functions to retrieve the list of most recent bookmarks.

The code for iOS, analogous to the first part, will come in another CL.

BUG=631474
Committed: https://crrev.com/9ace14b3b2c1fba4261d6edb19965f7373355c4b
Cr-Commit-Position: refs/heads/master@{#410022}

[Philipp Keck, 2016-08-02 11:55:22]

Description was changed from

==========
Taking over jkrcal's CL https://codereview.chromium.org/2184263005/#ps40001

BUG=631474
==========

to

==========
Add a tab helper to record the last visit date for each bookmark

The NTP on Android will display a list of recently visited bookmarks.
This CL introduces a tab helper that stores current date to the meta
data of a bookmark when the bookmark gets visited.

The code is split into a part that depends on content (living in
chrome/browser/ntp_snippets) and a platform-independent part
(living in components/ntp_snippets).

The platform independent code also provides functions to retrieve the list of
most recent bookmarks.

The code for iOS, analogous to the first part, will come in another CL.

BUG=631474
==========

[Philipp Keck, 2016-08-02 13:12:21]

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

[Philipp Keck, 2016-08-02 13:12:21]

This is a copy of https://codereview.chromium.org/2184263005/#ps40001 because
jkrcal is on holiday.

Most comments from there are addressed except the "TabHelper" naming and the
OS_ANDROID vs. ANDROID_JAVA_UI question.


avi@chromium.org: Please review changes in 
chrome/browser/ui/tab_helpers.cc
and the naming of chrome/browser/ntp_snippets/bookmark_last_visit_updater.h

treib@chromium.org: Please review changes in 
components/ntp_snippets*
chrome/browser/ntp_snippets/*

sky@chromium.org: Please review changes in 
chrome/chrome_browser.gypi
chrome/browser/bookmarks/*

[Marc Treib, 2016-08-02 13:37:12]

LGTM with a whole bunch of nits :)

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_model_factory.h (right):

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_model_factory.h:29: static
bookmarks::BookmarkModel* GetForProfileIfExists(Profile* profile);
Maybe add a TODO that these two should be removed in favor of BrowserContext
versions? As it is, I'd wonder why both variants exist.
(Let's see what sky thinks about this!)

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right):

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:7: #include
"components/bookmarks/browser/bookmark_model.h"
This isn't strictly required, since you're not doing anything with the
BookmarkModel.

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:19:
DCHECK(web_contents);
This is superfluous - if it were null, we'd crash in the next line anyway, and
it doesn't provide any information to the reader since the next line obviously
assumes it's non-null.

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right):

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:38: friend class
content::WebContentsUserData<BookmarkLastVisitUpdater>;
nit: empty line before. I also think the friend declaration should come first?

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

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:47: // If there
are bookmark for |url|, set their last visit date to now.
nit: bookmarks, plural

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:51:
FormatLastVisitDate(now));
nit: You could pull this out of the loop

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:77: // Remove
bookmarks that have not been visited after |min_time|.
nit: The empty line should be before this

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:91:
std::max(max_visit_time, GetLastVisitDateForBookmark(node));
You could just do
if (GetLastVisitDateForBookmark(node) > min_visit_time) return false;
and "return true" after the loop.

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right):

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:8: #include
<memory>
Not needed

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:11: #include
"base/macros.h"
Not needed

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:14: #include
"components/bookmarks/browser/bookmark_node.h"
optional: You could just forward-declare stuff here, and include these only in
the .cc

[Philipp Keck, 2016-08-02 14:06:51]

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right):

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:7: #include
"components/bookmarks/browser/bookmark_model.h"
On 2016/08/02 13:37:11, Marc Treib wrote:
> This isn't strictly required, since you're not doing anything with the
> BookmarkModel.

Done.

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:19:
DCHECK(web_contents);
On 2016/08/02 13:37:11, Marc Treib wrote:
> This is superfluous - if it were null, we'd crash in the next line anyway, and
> it doesn't provide any information to the reader since the next line obviously
> assumes it's non-null.

Done.

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right):

https://codereview.chromium.org/2200113002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:38: friend class
content::WebContentsUserData<BookmarkLastVisitUpdater>;
On 2016/08/02 13:37:11, Marc Treib wrote:
> nit: empty line before. I also think the friend declaration should come first?

Done.

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

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:47: // If there
are bookmark for |url|, set their last visit date to now.
On 2016/08/02 13:37:11, Marc Treib wrote:
> nit: bookmarks, plural

Done.

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:51:
FormatLastVisitDate(now));
On 2016/08/02 13:37:11, Marc Treib wrote:
> nit: You could pull this out of the loop

Done.

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:77: // Remove
bookmarks that have not been visited after |min_time|.
On 2016/08/02 13:37:11, Marc Treib wrote:
> nit: The empty line should be before this

Done.

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:91:
std::max(max_visit_time, GetLastVisitDateForBookmark(node));
On 2016/08/02 13:37:11, Marc Treib wrote:
> You could just do
> if (GetLastVisitDateForBookmark(node) > min_visit_time) return false;
> and "return true" after the loop.

Done.

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right):

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:8: #include
<memory>
On 2016/08/02 13:37:12, Marc Treib wrote:
> Not needed

Done.

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:11: #include
"base/macros.h"
On 2016/08/02 13:37:12, Marc Treib wrote:
> Not needed

Done.

https://codereview.chromium.org/2200113002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:14: #include
"components/bookmarks/browser/bookmark_node.h"
On 2016/08/02 13:37:12, Marc Treib wrote:
> optional: You could just forward-declare stuff here, and include these only in
> the .cc

Done.

[Avi (use Gerrit), 2016-08-02 18:33:05]

Picking up from the other CL.

> So, if I understood that correctly, ANDROID_JAVA_UI is the subset of those
> OS_ANDROID where we don't only compile for the Android platform, but also use
> the whole Java UI stuff listed there.

Yes, the difference is compiling for Android (linking in crypto libraries, etc.)
vs the Android UI.

> The BookmarkLastVisitUpdater needs to run on all OS_ANDROID and all its
> dependencies are compiled for OS_ANDROID, not only for ANDROID_JAVA_UI.

I don't get that. It seems pretty clear that the last visit is being kept on
behalf of the code that presents it in a UI, so to me it seems like UI code. It
also seems to me that to be strictly correct, all its dependencies that are
OS_ANDROID should be ANDROID_JAVA_UI instead.

I've been talking to the Android people and they agree that the
ANDROID_JAVA_UI/OS_ANDROID split is a mess. In theory, there should be a clean
separation, but there isn't, and we get confusion.

> If you prefer, we can use ANDROID_JAVA_UI (below) instead of OS_ANDROID in
this
> file for consistency, even though it seems to be the deprecated one.

Yes. I can understand if you don't want to change up in this CL the entire
dependency chain that is OS_ANDROID, but at least keep consistency in the tab
helper file, please.

[Philipp Keck, 2016-08-03 11:30:40]

On 2016/08/02 18:33:05, Avi wrote:
> Picking up from the other CL.
> 
> > So, if I understood that correctly, ANDROID_JAVA_UI is the subset of those
> > OS_ANDROID where we don't only compile for the Android platform, but also
use
> > the whole Java UI stuff listed there.
> 
> Yes, the difference is compiling for Android (linking in crypto libraries,
etc.)
> vs the Android UI.
> 
> > The BookmarkLastVisitUpdater needs to run on all OS_ANDROID and all its
> > dependencies are compiled for OS_ANDROID, not only for ANDROID_JAVA_UI.
> 
> I don't get that. It seems pretty clear that the last visit is being kept on
> behalf of the code that presents it in a UI, so to me it seems like UI code.
It
> also seems to me that to be strictly correct, all its dependencies that are
> OS_ANDROID should be ANDROID_JAVA_UI instead.
> 
> I've been talking to the Android people and they agree that the
> ANDROID_JAVA_UI/OS_ANDROID split is a mess. In theory, there should be a clean
> separation, but there isn't, and we get confusion.
> 
> > If you prefer, we can use ANDROID_JAVA_UI (below) instead of OS_ANDROID in
> this
> > file for consistency, even though it seems to be the deprecated one.
> 
> Yes. I can understand if you don't want to change up in this CL the entire
> dependency chain that is OS_ANDROID, but at least keep consistency in the tab
> helper file, please.

Ok, so I moved everything to the ANDROID_JAVA_UI block, there is no more
OS_ANDROID in tab_helpers.cc now.

[Avi (use Gerrit), 2016-08-03 14:47:40]

https://codereview.chromium.org/2200113002/diff/80001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2200113002/diff/80001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:203:
Profile::FromBrowserContext(web_contents->GetBrowserContext())));
Alphabetical order, please.

[Philipp Keck, 2016-08-03 15:37:34]

https://codereview.chromium.org/2200113002/diff/80001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2200113002/diff/80001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:203:
Profile::FromBrowserContext(web_contents->GetBrowserContext())));
On 2016/08/03 14:47:40, Avi wrote:
> Alphabetical order, please.

Done.

[Avi (use Gerrit), 2016-08-03 15:46:54]

lgtm

👍

[sky, 2016-08-03 19:30:27]

https://codereview.chromium.org/2200113002/diff/120001/chrome/browser/bookmar...
File chrome/browser/bookmarks/bookmark_model_factory.h (right):

https://codereview.chromium.org/2200113002/diff/120001/chrome/browser/bookmar...
chrome/browser/bookmarks/bookmark_model_factory.h:27: static
bookmarks::BookmarkModel* GetForProfile(Profile* profile);
Remove this and convert to using new functions.

[Philipp Keck, 2016-08-04 11:25:48]

Thank you for reviewing.

https://codereview.chromium.org/2200113002/diff/120001/chrome/browser/bookmar...
File chrome/browser/bookmarks/bookmark_model_factory.h (right):

https://codereview.chromium.org/2200113002/diff/120001/chrome/browser/bookmar...
chrome/browser/bookmarks/bookmark_model_factory.h:27: static
bookmarks::BookmarkModel* GetForProfile(Profile* profile);
On 2016/08/03 19:30:27, sky wrote:
> Remove this and convert to using new functions.

Ok. Since that will affect almost 100 places in the codebase and because we have
feature freeze coming up with a couple of other CLs depending on this one, I'd
like to do these changes in a separate CL.

I added a GetForBrowserContextIfExists for consistency, and a TODO(pke) comment
for removing GetForProfile[IfExists]. The new CL will follow very soon, at the
latest right after feature freeze. I hope we can merge this CL as is -- or do
you need me to remove the two new methods for now and reinsert them later?

[Philipp Keck, 2016-08-04 17:46:12]

On 2016/08/04 11:25:48, Philipp Keck wrote:
> Thank you for reviewing.
> 
>
https://codereview.chromium.org/2200113002/diff/120001/chrome/browser/bookmar...
> File chrome/browser/bookmarks/bookmark_model_factory.h (right):
> 
>
https://codereview.chromium.org/2200113002/diff/120001/chrome/browser/bookmar...
> chrome/browser/bookmarks/bookmark_model_factory.h:27: static
> bookmarks::BookmarkModel* GetForProfile(Profile* profile);
> On 2016/08/03 19:30:27, sky wrote:
> > Remove this and convert to using new functions.
> 
> Ok. Since that will affect almost 100 places in the codebase and because we
have
> feature freeze coming up with a couple of other CLs depending on this one, I'd
> like to do these changes in a separate CL.
> 
> I added a GetForBrowserContextIfExists for consistency, and a TODO(pke)
comment
> for removing GetForProfile[IfExists]. The new CL will follow very soon, at the
> latest right after feature freeze. I hope we can merge this CL as is -- or do
> you need me to remove the two new methods for now and reinsert them later?

And here is the follow-up CL already: https://codereview.chromium.org/2216713002

[sky, 2016-08-04 19:32:19]

LGTM

https://codereview.chromium.org/2200113002/diff/180001/chrome/browser/ntp_sni...
File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right):

https://codereview.chromium.org/2200113002/diff/180001/chrome/browser/ntp_sni...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:38: explicit
BookmarkLastVisitUpdater(content::WebContents* web_contents,
explicit not necessary here.

[Philipp Keck, 2016-08-05 07:51:52]

https://codereview.chromium.org/2200113002/diff/180001/chrome/browser/ntp_sni...
File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right):

https://codereview.chromium.org/2200113002/diff/180001/chrome/browser/ntp_sni...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:38: explicit
BookmarkLastVisitUpdater(content::WebContents* web_contents,
On 2016/08/04 19:32:18, sky wrote:
> explicit not necessary here.

Done.

[Philipp Keck, 2016-08-05 07:51:59]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-05 07:52:00]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
avi@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2200113002/#ps200001
(title: "Remove unnecessary explicit")

[commit-bot: I haz the power, 2016-08-05 07:52:15]

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-05 08:55:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 08:55:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Philipp Keck, 2016-08-05 09:22:21]

The CQ bit was checked by pke@google.com

[commit-bot: I haz the power, 2016-08-05 09:22:28]

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-05 09:51:08]

Description was changed from

==========
Add a tab helper to record the last visit date for each bookmark

The NTP on Android will display a list of recently visited bookmarks.
This CL introduces a tab helper that stores current date to the meta
data of a bookmark when the bookmark gets visited.

The code is split into a part that depends on content (living in
chrome/browser/ntp_snippets) and a platform-independent part
(living in components/ntp_snippets).

The platform independent code also provides functions to retrieve the list of
most recent bookmarks.

The code for iOS, analogous to the first part, will come in another CL.

BUG=631474
==========

to

==========
Add a tab helper to record the last visit date for each bookmark

The NTP on Android will display a list of recently visited bookmarks.
This CL introduces a tab helper that stores current date to the meta
data of a bookmark when the bookmark gets visited.

The code is split into a part that depends on content (living in
chrome/browser/ntp_snippets) and a platform-independent part
(living in components/ntp_snippets).

The platform independent code also provides functions to retrieve the list of
most recent bookmarks.

The code for iOS, analogous to the first part, will come in another CL.

BUG=631474
==========

[commit-bot: I haz the power, 2016-08-05 09:51:09]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-08-05 09:53:17]

Description was changed from

==========
Add a tab helper to record the last visit date for each bookmark

The NTP on Android will display a list of recently visited bookmarks.
This CL introduces a tab helper that stores current date to the meta
data of a bookmark when the bookmark gets visited.

The code is split into a part that depends on content (living in
chrome/browser/ntp_snippets) and a platform-independent part
(living in components/ntp_snippets).

The platform independent code also provides functions to retrieve the list of
most recent bookmarks.

The code for iOS, analogous to the first part, will come in another CL.

BUG=631474
==========

to

==========
Add a tab helper to record the last visit date for each bookmark

The NTP on Android will display a list of recently visited bookmarks.
This CL introduces a tab helper that stores current date to the meta
data of a bookmark when the bookmark gets visited.

The code is split into a part that depends on content (living in
chrome/browser/ntp_snippets) and a platform-independent part
(living in components/ntp_snippets).

The platform independent code also provides functions to retrieve the list of
most recent bookmarks.

The code for iOS, analogous to the first part, will come in another CL.

BUG=631474

Committed: https://crrev.com/9ace14b3b2c1fba4261d6edb19965f7373355c4b
Cr-Commit-Position: refs/heads/master@{#410022}
==========

[commit-bot: I haz the power, 2016-08-05 09:53:18]

Patchset 11 (id:??) landed as
https://crrev.com/9ace14b3b2c1fba4261d6edb19965f7373355c4b
Cr-Commit-Position: refs/heads/master@{#410022}