Marking last_visited date for bookmarks created from an open tab

Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date
when last_visited info is missing. This does not work as bookmarks
created by sync get the time they are synced to the device as their
creation date. Thus, on a freshly synced device, one gets all
bookmarks as recently visited. This CL also removes the fallback.

Lastly, the CL also adds marking last_visited dates upon
server-side redirects. E.g. if you have a bit.ly/foo link to foo.com
and bookmark foo.com, going there through bit.ly/foo also marks this
bookmark as visited. Previously only the bit.ly/foo URL is checked.

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

[jkrcal, 2016-08-11 12:20:33]

Description was changed from

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

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

to

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date when
last_visited info is missing. This does not work as bookmarks created by sync
get the time they are synced to the device as their creation date. Thus, on a
freshly synced device, one gets all bookmarks as recently visited. This CL also
removes the fallback.

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

[jkrcal, 2016-08-11 12:21:56]

Description was changed from

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date when
last_visited info is missing. This does not work as bookmarks created by sync
get the time they are synced to the device as their creation date. Thus, on a
freshly synced device, one gets all bookmarks as recently visited. This CL also
removes the fallback.

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

to

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date 
when last_visited info is missing. This does not work as bookmarks 
created by sync get the time they are synced to the device as their 
creation date. Thus, on a freshly synced device, one gets all 
bookmarks as recently visited. This CL also removes the fallback.

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

[jkrcal, 2016-08-11 12:24:28]

jkrcal@chromium.org changed reviewers:
+ treib@chromium.org

[jkrcal, 2016-08-11 12:24:29]

Marc, PTAL!

[Marc Treib, 2016-08-11 13:03:44]

https://codereview.chromium.org/2234393002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (left):

https://codereview.chromium.org/2234393002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:65: // Use
creation date if no last visit info present.
I think the header has a comment that mentions the fallback - does that need to
be updated?

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

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:22:
BookmarkLastVisitUpdater::~BookmarkLastVisitUpdater() {
Why move this? (The ordering is kinda weird anyway, but at least before it was
consistent with the header)

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:63: if
(new_bookmark_url == web_contents_->GetLastCommittedURL()) {
What's this check for?

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

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:39: //
BookmarkModelObserver overrides
All these can be private.
Also, nit: Choose a consistent style for this comment and the corresponding one
for WebContentsObserver

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:85: GURL
current_navigation_url_;
Unused?

[jkrcal, 2016-08-11 13:56:07]

Description was changed from

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date 
when last_visited info is missing. This does not work as bookmarks 
created by sync get the time they are synced to the device as their 
creation date. Thus, on a freshly synced device, one gets all 
bookmarks as recently visited. This CL also removes the fallback.

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

to

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date 
when last_visited info is missing. This does not work as bookmarks 
created by sync get the time they are synced to the device as their 
creation date. Thus, on a freshly synced device, one gets all 
bookmarks as recently visited. This CL also removes the fallback.

Lastly, the CL also adds marking last_visited dates upon 
server-side redirects. E.g. if you have a bit.ly/foo link to foo.com
and bookmark foo.com, going there through bit.ly/foo also marks this
bookmark as visited. Previously only the bit.ly/foo URL is checked.


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

[jkrcal, 2016-08-11 13:56:16]

Description was changed from

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date 
when last_visited info is missing. This does not work as bookmarks 
created by sync get the time they are synced to the device as their 
creation date. Thus, on a freshly synced device, one gets all 
bookmarks as recently visited. This CL also removes the fallback.

Lastly, the CL also adds marking last_visited dates upon 
server-side redirects. E.g. if you have a bit.ly/foo link to foo.com
and bookmark foo.com, going there through bit.ly/foo also marks this
bookmark as visited. Previously only the bit.ly/foo URL is checked.


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

to

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date 
when last_visited info is missing. This does not work as bookmarks 
created by sync get the time they are synced to the device as their 
creation date. Thus, on a freshly synced device, one gets all 
bookmarks as recently visited. This CL also removes the fallback.

Lastly, the CL also adds marking last_visited dates upon 
server-side redirects. E.g. if you have a bit.ly/foo link to foo.com
and bookmark foo.com, going there through bit.ly/foo also marks this
bookmark as visited. Previously only the bit.ly/foo URL is checked.

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

[jkrcal, 2016-08-11 13:59:27]

Thanks a lot!
PTAL, again.

https://codereview.chromium.org/2234393002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (left):

https://codereview.chromium.org/2234393002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:65: // Use
creation date if no last visit info present.
On 2016/08/11 13:03:43, Marc Treib wrote:
> I think the header has a comment that mentions the fallback - does that need
to
> be updated?

Done.

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

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:22:
BookmarkLastVisitUpdater::~BookmarkLastVisitUpdater() {
On 2016/08/11 13:03:43, Marc Treib wrote:
> Why move this? (The ordering is kinda weird anyway, but at least before it was
> consistent with the header)

Done. I've forgotten about the header. Looking at the header now, I realized the
order makes sense (as the constructor is private) so I reverted this change.

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:63: if
(new_bookmark_url == web_contents_->GetLastCommittedURL()) {
On 2016/08/11 13:03:43, Marc Treib wrote:
> What's this check for?

Added a comment. Is it clearer now?
(the check is the reason why it is in a tab helper in the first place)

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

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:39: //
BookmarkModelObserver overrides
On 2016/08/11 13:03:43, Marc Treib wrote:
> All these can be private.
> Also, nit: Choose a consistent style for this comment and the corresponding
one
> for WebContentsObserver

Done.

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:85: GURL
current_navigation_url_;
On 2016/08/11 13:03:43, Marc Treib wrote:
> Unused?

Sorry, a left-over from previous implementation version.

[jkrcal, 2016-08-11 13:59:36]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 14:00:07]

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

[Marc Treib, 2016-08-11 14:06:23]

lgtm

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

https://codereview.chromium.org/2234393002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:63: if
(new_bookmark_url == web_contents_->GetLastCommittedURL()) {
On 2016/08/11 13:59:26, jkrcal wrote:
> On 2016/08/11 13:03:43, Marc Treib wrote:
> > What's this check for?
> 
> Added a comment. Is it clearer now?
> (the check is the reason why it is in a tab helper in the first place)

Yup, better, thanks!

[commit-bot: I haz the power, 2016-08-11 15:02:44]

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

[commit-bot: I haz the power, 2016-08-11 15:02:45]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-08-11 15:04:06]

The CQ bit was checked by jkrcal@chromium.org

[commit-bot: I haz the power, 2016-08-11 15:04:34]

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-11 15:08:23]

Description was changed from

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date 
when last_visited info is missing. This does not work as bookmarks 
created by sync get the time they are synced to the device as their 
creation date. Thus, on a freshly synced device, one gets all 
bookmarks as recently visited. This CL also removes the fallback.

Lastly, the CL also adds marking last_visited dates upon 
server-side redirects. E.g. if you have a bit.ly/foo link to foo.com
and bookmark foo.com, going there through bit.ly/foo also marks this
bookmark as visited. Previously only the bit.ly/foo URL is checked.

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

to

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date 
when last_visited info is missing. This does not work as bookmarks 
created by sync get the time they are synced to the device as their 
creation date. Thus, on a freshly synced device, one gets all 
bookmarks as recently visited. This CL also removes the fallback.

Lastly, the CL also adds marking last_visited dates upon 
server-side redirects. E.g. if you have a bit.ly/foo link to foo.com
and bookmark foo.com, going there through bit.ly/foo also marks this
bookmark as visited. Previously only the bit.ly/foo URL is checked.

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

[commit-bot: I haz the power, 2016-08-11 15:08:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-11 15:09:44]

Description was changed from

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date 
when last_visited info is missing. This does not work as bookmarks 
created by sync get the time they are synced to the device as their 
creation date. Thus, on a freshly synced device, one gets all 
bookmarks as recently visited. This CL also removes the fallback.

Lastly, the CL also adds marking last_visited dates upon 
server-side redirects. E.g. if you have a bit.ly/foo link to foo.com
and bookmark foo.com, going there through bit.ly/foo also marks this
bookmark as visited. Previously only the bit.ly/foo URL is checked.

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

to

==========
Marking last_visited date for bookmarks created from an open tab

Previously, last_visited date was noted only when a (previously
bookmarked) page is opened. This CL notes the date also when a new
bookmark is created (while the URL of the new bookmark is opened in
some browser tab).

Previously, we used creation date of a bookmark as a fallback date
when last_visited info is missing. This does not work as bookmarks
created by sync get the time they are synced to the device as their
creation date. Thus, on a freshly synced device, one gets all
bookmarks as recently visited. This CL also removes the fallback.

Lastly, the CL also adds marking last_visited dates upon
server-side redirects. E.g. if you have a bit.ly/foo link to foo.com
and bookmark foo.com, going there through bit.ly/foo also marks this
bookmark as visited. Previously only the bit.ly/foo URL is checked.

BUG=631474

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

[commit-bot: I haz the power, 2016-08-11 15:09:45]

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