Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184263005:
Add a tab helper to record the last visit date for each bookmark. (Closed)
|
Created:
4 years, 4 months ago by jkrcal Modified:
4 years, 4 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org, ntp-dev+reviews_chromium.org, Bernhard Bauer Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd 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
Patch Set 1 #
Total comments: 68
Patch Set 2 : Comments #1 #
Total comments: 18
Patch Set 3 : Comments #2 #
Total comments: 26
Dependent Patchsets: Messages
Total messages: 28 (10 generated)
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 analogous code for iOS 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 ==========
jkrcal@chromium.org changed reviewers: + sky@chromium.org, treib@chromium.org
Scott: PTAL at chrome/* Marc: PTAL at components/ntp_snippets/*
I also took a look at c/b/ntp_snippets :) https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/ntp_bookmark_helper.cc (right): https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Also here https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/ntp_bookmark_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. What year is it?! Also no "(c)" https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:19: class NavigationHandle; nit: class WebContents https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:22: using bookmarks::BookmarkModel; No using in headers please https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:24: namespace ntp_snippets { This shouldn't be in the ntp_snippets namespace; that's only used for the component itself. https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:36: static void CreateForWebContentsWithBookmarkModel( Is this a common pattern? What if there already is an instance for this WebContents? https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. . https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:13: const char kBookmarkLastVisitDate[] = "last_visited"; nit: empty line before https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:13: const char kBookmarkLastVisitDate[] = "last_visited"; Add "Key" or something like that? https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:16: int64_t date; Initialize this? (I think the Win compiler will complain if you don't) https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:18: return base::Time::UnixEpoch(); just base::Time() ? https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:27: if (node == nullptr) if (!node) https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:28: return base::Time::UnixEpoch(); Here too, just base::Time() ? https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:43: return (GetLastVisitDateImpl(a) - GetLastVisitDateImpl(b)).InMilliseconds() > return GetLastVisitDateImpl(a) > GetLastVisitDateImpl(b); https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:54: nit: no empty line https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:57: if (node == nullptr) if (!node) https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:68: return GetLastVisitDateImpl(node); Just put the code here and get rid of the Impl? https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:84: [&](const BookmarkModel::URLAndTitle& bookmark) { Don't use default captures; capture only what you actually need. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:86: bookmark_model->GetMostRecentlyAddedUserNodeForURL( Can this return null here? Also I'm not sure what to make of the "MostRecentlyAdded". Can there be multiple BookmarkNodes for a URL? Are we sure the most recent one is the correct one? https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:104: if (count > max_count) Just check bookmarks.size() and get rid of count? https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. . https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:17: using bookmarks::BookmarkNode; No using in headers (you can put them into the .cc though) https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:21: class BookmarkNavigationObserver { Any reason for the separate observer interface? The code in chrome/browser needs to know about the implementation anyway, so I think you could just remove this. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:24: const GURL& url); If you do keep the observer, then this should be pure virtual ("= 0"). https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:34: void OnURLVisitedInMainFrame(BookmarkModel* bookmark_model, The bookmark model will always be the same, right? Might as well pass it into the ctor then. Or alternatively, right now this class doesn't actually do anything. Might as well get rid of it and just have some global functions instead. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:41: std::vector<const BookmarkNode*>* bookmarks, Just return the vector? (Thanks to move semantics, that won't be expensive) If you do keep the output param, then the order is: first inputs, then outputs. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:43: const base::Time& min_time); min_visit_time?
blundell@chromium.org changed reviewers: + blundell@chromium.org
https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/ntp_bookmark_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:36: static void CreateForWebContentsWithBookmarkModel( On 2016/07/29 09:54:56, Marc Treib wrote: > Is this a common pattern? What if there already is an instance for this > WebContents? drive-by: Yes, this is the common pattern here. WebContentsUserData is different from KeyedService in that WCUD is not created lazily on a call to the getter; it's only created via an explicit call to CreateForWebContents(). In this CL, that's being added to tab_helpers.cc (the correct location).
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/ntp_bookmark_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:30: class NTPBookmarkHelper nit: I have an issue with any class or library having sufixes like 'helper' or 'utils'. If it's purpose is to bridge between the two classes why not give it a corresponding name? 'bridge' might be misleading as it's used in JNI context. Let's discuss this offline.
https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:28: class BookmarkLastVisitDateHelper so, I'd call this class something like LastVisitedBookmarks. Actually, I like the separation with the other interface, as it simplifies naming a lot (nicely separates the two roles the class is playing). You could then rename the other helper to BookmarkNavigationObserverAdapter or the like, as it's an adapter for this class to work with Android or iOS. WDYT?
https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:28: class BookmarkLastVisitDateHelper On 2016/07/29 10:33:00, tschumann wrote: > so, I'd call this class something like LastVisitedBookmarks. > > Actually, I like the separation with the other interface, as it simplifies > naming a lot (nicely separates the two roles the class is playing). > > You could then rename the other helper to BookmarkNavigationObserverAdapter or > the like, as it's an adapter for this class to work with Android or iOS. > > WDYT? I think there's no reason for a class at all, since it doesn't have any non-static members (the one non-static method might as well be static). So I'd prefer calling this what it is: A bunch of helper methods.
https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:28: class BookmarkLastVisitDateHelper On 2016/07/29 10:42:22, Marc Treib wrote: > On 2016/07/29 10:33:00, tschumann wrote: > > so, I'd call this class something like LastVisitedBookmarks. > > > > Actually, I like the separation with the other interface, as it simplifies > > naming a lot (nicely separates the two roles the class is playing). > > > > You could then rename the other helper to BookmarkNavigationObserverAdapter or > > the like, as it's an adapter for this class to work with Android or iOS. > > > > WDYT? > > I think there's no reason for a class at all, since it doesn't have any > non-static members (the one non-static method might as well be static). So I'd > prefer calling this what it is: A bunch of helper methods. right, if we don't need a class, then a function will do as well. But nevertheless, the functions have a pretty clear purpose and the file/library names should reflect that. With these generic helpers and utils libraries it's really hard to anticipate what functionality is in a given library. I've too often witnessed functionality of such libraries being hard to find or duplicated in other libraries (because they weren't found). They have a tendency to grow into a hodgepodge of vaguely related functionality.
Thanks! PTAL, again. https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/ntp_bookmark_helper.cc (right): https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/07/29 09:54:56, Marc Treib wrote: > Also here Done. https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/ntp_bookmark_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/07/29 09:54:56, Marc Treib wrote: > What year is it?! > Also no "(c)" Done. https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:19: class NavigationHandle; On 2016/07/29 09:54:56, Marc Treib wrote: > nit: class WebContents Done. Out of curiosity: Do I need to redeclare it even if the included header (web_cont_observer) declares it? https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:22: using bookmarks::BookmarkModel; On 2016/07/29 09:54:56, Marc Treib wrote: > No using in headers please Done. https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:24: namespace ntp_snippets { On 2016/07/29 09:54:56, Marc Treib wrote: > This shouldn't be in the ntp_snippets namespace; that's only used for the > component itself. Done. https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:30: class NTPBookmarkHelper On 2016/07/29 10:18:43, tschumann wrote: > nit: I have an issue with any class or library having sufixes like 'helper' or > 'utils'. > If it's purpose is to bridge between the two classes why not give it a > corresponding name? 'bridge' might be misleading as it's used in JNI context. > Let's discuss this offline. Done. https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:36: static void CreateForWebContentsWithBookmarkModel( On 2016/07/29 09:59:23, blundell wrote: > On 2016/07/29 09:54:56, Marc Treib wrote: > > Is this a common pattern? What if there already is an instance for this > > WebContents? > > drive-by: Yes, this is the common pattern here. WebContentsUserData is different > from KeyedService in that WCUD is not created lazily on a call to the getter; > it's only created via an explicit call to CreateForWebContents(). In this CL, > that's being added to tab_helpers.cc (the correct location). Acknowledged. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/07/29 09:54:57, Marc Treib wrote: > . Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:13: const char kBookmarkLastVisitDate[] = "last_visited"; On 2016/07/29 09:54:57, Marc Treib wrote: > nit: empty line before Done (both). https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:16: int64_t date; On 2016/07/29 09:54:57, Marc Treib wrote: > Initialize this? (I think the Win compiler will complain if you don't) Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:18: return base::Time::UnixEpoch(); On 2016/07/29 09:54:57, Marc Treib wrote: > just base::Time() ? Result of base::Time() is platform-dependent (can use different epochs). Probably not important as on any platform, this will be long in the past but I like UnixEpoch() for being more predictable. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:27: if (node == nullptr) On 2016/07/29 09:54:57, Marc Treib wrote: > if (!node) Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:28: return base::Time::UnixEpoch(); On 2016/07/29 09:54:57, Marc Treib wrote: > Here too, just base::Time() ? Same as above. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:43: return (GetLastVisitDateImpl(a) - GetLastVisitDateImpl(b)).InMilliseconds() > On 2016/07/29 09:54:57, Marc Treib wrote: > return GetLastVisitDateImpl(a) > GetLastVisitDateImpl(b); error: cannot convert 'base::TimeDelta' to 'int' in return. The return value should be actually bool, I fixed it. Does not change anything, however. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:54: On 2016/07/29 09:54:57, Marc Treib wrote: > nit: no empty line Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:57: if (node == nullptr) On 2016/07/29 09:54:57, Marc Treib wrote: > if (!node) Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:68: return GetLastVisitDateImpl(node); On 2016/07/29 09:54:57, Marc Treib wrote: > Just put the code here and get rid of the Impl? Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:84: [&](const BookmarkModel::URLAndTitle& bookmark) { On 2016/07/29 09:54:57, Marc Treib wrote: > Don't use default captures; capture only what you actually need. Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:86: bookmark_model->GetMostRecentlyAddedUserNodeForURL( On 2016/07/29 09:54:57, Marc Treib wrote: > Can this return null here? > Also I'm not sure what to make of the "MostRecentlyAdded". Can there be multiple > BookmarkNodes for a URL? Are we sure the most recent one is the correct one? It can return null but GetLastVisitDate() handles it. As regards MostRecentlyAdded: good point. This is consistent with what I do in OnURLVisitedInMainFrame. Only the MostRecentlyAdded has the last_visit meta data. If we need to distinguish which of the equivalent bookmarks is actually visited, I would need to observe at other places as well (where I would have more information than the URL). I suggest to keep it simple for now, adding a TODO to investigate further (after M54). https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:104: if (count > max_count) On 2016/07/29 09:54:57, Marc Treib wrote: > Just check bookmarks.size() and get rid of count? Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/07/29 09:54:57, Marc Treib wrote: > . Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:17: using bookmarks::BookmarkNode; On 2016/07/29 09:54:57, Marc Treib wrote: > No using in headers (you can put them into the .cc though) Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:21: class BookmarkNavigationObserver { On 2016/07/29 09:54:57, Marc Treib wrote: > Any reason for the separate observer interface? The code in chrome/browser needs > to know about the implementation anyway, so I think you could just remove this. Agreed. Not much persuaded this adds any advantage, after all. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:24: const GURL& url); On 2016/07/29 09:54:57, Marc Treib wrote: > If you do keep the observer, then this should be pure virtual ("= 0"). Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:28: class BookmarkLastVisitDateHelper On 2016/07/29 12:38:56, tschumann wrote: > On 2016/07/29 10:42:22, Marc Treib wrote: > > On 2016/07/29 10:33:00, tschumann wrote: > > > so, I'd call this class something like LastVisitedBookmarks. > > > > > > Actually, I like the separation with the other interface, as it simplifies > > > naming a lot (nicely separates the two roles the class is playing). > > > > > > You could then rename the other helper to BookmarkNavigationObserverAdapter > or > > > the like, as it's an adapter for this class to work with Android or iOS. > > > > > > WDYT? > > > > I think there's no reason for a class at all, since it doesn't have any > > non-static members (the one non-static method might as well be static). So I'd > > prefer calling this what it is: A bunch of helper methods. > > right, if we don't need a class, then a function will do as well. > But nevertheless, the functions have a pretty clear purpose and the file/library > names should reflect that. > With these generic helpers and utils libraries it's really hard to anticipate > what functionality is in a given library. I've too often witnessed functionality > of such libraries being hard to find or duplicated in other libraries (because > they weren't found). > They have a tendency to grow into a hodgepodge of vaguely related functionality. Done. I still call the component code "_util" to make clear these are global functions. I believe the whole name is specific enough to keep it clear and focused. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:34: void OnURLVisitedInMainFrame(BookmarkModel* bookmark_model, On 2016/07/29 09:54:57, Marc Treib wrote: > The bookmark model will always be the same, right? Might as well pass it into > the ctor then. > Or alternatively, right now this class doesn't actually do anything. Might as > well get rid of it and just have some global functions instead. Done global functions. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:41: std::vector<const BookmarkNode*>* bookmarks, On 2016/07/29 09:54:57, Marc Treib wrote: > Just return the vector? (Thanks to move semantics, that won't be expensive) > If you do keep the output param, then the order is: first inputs, then outputs. Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.h:43: const base::Time& min_time); On 2016/07/29 09:54:57, Marc Treib wrote: > min_visit_time? Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/ntp_bookmark_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:19: class NavigationHandle; On 2016/07/29 12:42:41, jkrcal wrote: > On 2016/07/29 09:54:56, Marc Treib wrote: > > nit: class WebContents > > Done. Out of curiosity: Do I need to redeclare it even if the included header > (web_cont_observer) declares it? Generally the rule is to include/forward-declare everything you use. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:18: return base::Time::UnixEpoch(); On 2016/07/29 12:42:41, jkrcal wrote: > On 2016/07/29 09:54:57, Marc Treib wrote: > > just base::Time() ? > > Result of base::Time() is platform-dependent (can use different epochs). > Probably not important as on any platform, this will be long in the past but I > like UnixEpoch() for being more predictable. The advantage would be that you could check with .is_null() whether it's a proper time. But I guess that doesn't matter here, so okay. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:43: return (GetLastVisitDateImpl(a) - GetLastVisitDateImpl(b)).InMilliseconds() > On 2016/07/29 12:42:41, jkrcal wrote: > On 2016/07/29 09:54:57, Marc Treib wrote: > > return GetLastVisitDateImpl(a) > GetLastVisitDateImpl(b); > > error: cannot convert 'base::TimeDelta' to 'int' in return. > The return value should be actually bool, I fixed it. Does not change anything, > however. ">", not "-". https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:86: bookmark_model->GetMostRecentlyAddedUserNodeForURL( On 2016/07/29 12:42:41, jkrcal wrote: > On 2016/07/29 09:54:57, Marc Treib wrote: > > Can this return null here? > > Also I'm not sure what to make of the "MostRecentlyAdded". Can there be > multiple > > BookmarkNodes for a URL? Are we sure the most recent one is the correct one? > > It can return null but GetLastVisitDate() handles it. > > As regards MostRecentlyAdded: good point. This is consistent with what I do in > OnURLVisitedInMainFrame. Only the MostRecentlyAdded has the last_visit meta > data. > > If we need to distinguish which of the equivalent bookmarks is actually visited, > I would need to observe at other places as well (where I would have more > information than the URL). I suggest to keep it simple for now, adding a TODO to > investigate further (after M54). My point is mostly: Could the "MostRecentlyAdded" have changed since we stored the metadata? Then we'd effectively lose the visit date. https://codereview.chromium.org/2184263005/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right): https://codereview.chromium.org/2184263005/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:24: // abstract interface ntp_snippets::BookmarkNavigationObserver. Doesn't exist anymore :) https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right): https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:32: const BookmarkNode* b) { misaligned https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:34: ntp_snippets::GetLastVisitDateForBookmark(b)) optionally, you could move the anonymous namespace into the ntp_snippets namespace, then you don't have to say "ntp_snippets::" here. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:43: BookmarkModel* bookmark_model, fits on the previous line? https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:56: const BookmarkNode* node) { fits on the previous line? https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:76: std::vector<BookmarkModel::URLAndTitle> all_bookmarks; nit: I'd just call this bookmarks, since you filter some out, so it's not all anymore. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:98: std::vector<const BookmarkNode*> bookmarks; Ah, then you'll also have to rename this... result maybe? https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:103: if (bookmarks.size() >= (unsigned) max_count) static_cast<size_t> ? It's ugly, but at least it's honest, and we don't use unsigned. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right): https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:18: // Comment?
Thanks, again! PTAL. https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/ntp_bookmark_helper.h (right): https://codereview.chromium.org/2184263005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/ntp_bookmark_helper.h:19: class NavigationHandle; On 2016/07/29 14:36:42, Marc Treib wrote: > On 2016/07/29 12:42:41, jkrcal wrote: > > On 2016/07/29 09:54:56, Marc Treib wrote: > > > nit: class WebContents > > > > Done. Out of curiosity: Do I need to redeclare it even if the included header > > (web_cont_observer) declares it? > > Generally the rule is to include/forward-declare everything you use. Thanks. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... File components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc (right): https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:18: return base::Time::UnixEpoch(); On 2016/07/29 14:36:42, Marc Treib wrote: > On 2016/07/29 12:42:41, jkrcal wrote: > > On 2016/07/29 09:54:57, Marc Treib wrote: > > > just base::Time() ? > > > > Result of base::Time() is platform-dependent (can use different epochs). > > Probably not important as on any platform, this will be long in the past but I > > like UnixEpoch() for being more predictable. > > The advantage would be that you could check with .is_null() whether it's a > proper time. But I guess that doesn't matter here, so okay. I see. No, it does not matter here. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:43: return (GetLastVisitDateImpl(a) - GetLastVisitDateImpl(b)).InMilliseconds() > On 2016/07/29 14:36:42, Marc Treib wrote: > On 2016/07/29 12:42:41, jkrcal wrote: > > On 2016/07/29 09:54:57, Marc Treib wrote: > > > return GetLastVisitDateImpl(a) > GetLastVisitDateImpl(b); > > > > error: cannot convert 'base::TimeDelta' to 'int' in return. > > The return value should be actually bool, I fixed it. Does not change > anything, > > however. > > ">", not "-". Done. https://codereview.chromium.org/2184263005/diff/1/components/ntp_snippets/boo... components/ntp_snippets/bookmarks/bookmark_last_visit_date_helper.cc:86: bookmark_model->GetMostRecentlyAddedUserNodeForURL( On 2016/07/29 14:36:42, Marc Treib wrote: > On 2016/07/29 12:42:41, jkrcal wrote: > > On 2016/07/29 09:54:57, Marc Treib wrote: > > > Can this return null here? > > > Also I'm not sure what to make of the "MostRecentlyAdded". Can there be > > multiple > > > BookmarkNodes for a URL? Are we sure the most recent one is the correct one? > > > > It can return null but GetLastVisitDate() handles it. > > > > As regards MostRecentlyAdded: good point. This is consistent with what I do in > > OnURLVisitedInMainFrame. Only the MostRecentlyAdded has the last_visit meta > > data. > > > > If we need to distinguish which of the equivalent bookmarks is actually > visited, > > I would need to observe at other places as well (where I would have more > > information than the URL). I suggest to keep it simple for now, adding a TODO > to > > investigate further (after M54). > > My point is mostly: Could the "MostRecentlyAdded" have changed since we stored > the metadata? Then we'd effectively lose the visit date. Correct, thanks! I changed it to consider the most recent visit date over all bookmarks with the same URL. https://codereview.chromium.org/2184263005/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right): https://codereview.chromium.org/2184263005/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:24: // abstract interface ntp_snippets::BookmarkNavigationObserver. On 2016/07/29 14:36:42, Marc Treib wrote: > Doesn't exist anymore :) Done. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right): https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:32: const BookmarkNode* b) { On 2016/07/29 14:36:43, Marc Treib wrote: > misaligned Done. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:34: ntp_snippets::GetLastVisitDateForBookmark(b)) On 2016/07/29 14:36:42, Marc Treib wrote: > optionally, you could move the anonymous namespace into the ntp_snippets > namespace, then you don't have to say "ntp_snippets::" here. Done. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:43: BookmarkModel* bookmark_model, On 2016/07/29 14:36:43, Marc Treib wrote: > fits on the previous line? Done. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:56: const BookmarkNode* node) { On 2016/07/29 14:36:42, Marc Treib wrote: > fits on the previous line? Done. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:76: std::vector<BookmarkModel::URLAndTitle> all_bookmarks; On 2016/07/29 14:36:43, Marc Treib wrote: > nit: I'd just call this bookmarks, since you filter some out, so it's not all > anymore. Done. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:98: std::vector<const BookmarkNode*> bookmarks; On 2016/07/29 14:36:43, Marc Treib wrote: > Ah, then you'll also have to rename this... result maybe? Done. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:103: if (bookmarks.size() >= (unsigned) max_count) On 2016/07/29 14:36:43, Marc Treib wrote: > static_cast<size_t> ? It's ugly, but at least it's honest, and we don't use > unsigned. Done. https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right): https://codereview.chromium.org/2184263005/diff/20001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:18: // On 2016/07/29 14:36:43, Marc Treib wrote: > Comment? Done.
LGTM with a few more nits https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right): https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:50: FormatLastVisitDate(base::Time::Now())); nit: You could pull the base::Time::Now() out of the loop, to make sure they all get the exact same time. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:77: std::vector<const BookmarkNode*> bookmarks_for_url; Can you declare this variable inside the lambda? https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:82: &bookmarks_for_url](const BookmarkModel::URLAndTitle& bookmark) { Is this line break correct? Looks super weird https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:88: for (auto* node : bookmarks_for_url) { const auto* ? https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:91: max_visit_time = node_visit_time; optional nit: max_visit_time = std::max(max_visit_time, GetLastVisitDateForBookmark(node)); https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:93: return (max_visit_time <= min_visit_time); nit: no parens required https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:99: const BookmarkModel::URLAndTitle& b) { Is this the correct indentation? Looks super weird to me... https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right): https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:30: // Returns the list of most recently visited bookmarked URLs. For each such URL nit: It really returns the bookmarks, not just the bookmarked URLs.
https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right): https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:14: void BookmarkLastVisitUpdater::CreateForWebContentsWithBookmarkModel( nit: make order match header. https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right): https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:25: class BookmarkLastVisitUpdater Generally we name these classes with TabHelper at the end, e.g. BookmarkLastVisitUpdateTabHelper or maybe just BookmarkLastUpdateTabHelper.
https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right): https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:25: class BookmarkLastVisitUpdater On 2016/08/01 15:15:47, sky wrote: > Generally we name these classes with TabHelper at the end, e.g. > BookmarkLastVisitUpdateTabHelper or maybe just BookmarkLastUpdateTabHelper. :-) In the initial rounds of the review, we just had the reverse naming discussion (Jan was suggesting a 'Helper' name). I argued against it as helpers don't add any value to the name. It's purpose is to update the Bookmark's last-visit information so why not just call it after what it does. I realize that consistency also adds value, so if you insist we can change the name (although codesearch shows many naming flavors =)). It's just that from our code's point of view a 'TabHelper' suffix might actually be misleading and unnecessarily expose implementation details. Thanks for the review! --Tim
avi@chromium.org changed reviewers: + avi@chromium.org
https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... chrome/browser/ui/tab_helpers.cc:82: #include "chrome/browser/bookmarks/bookmark_model_factory.h" Why is this different than the ANDROID_JAVA_UI block above here and therefore in need of its own header block? https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... chrome/browser/ui/tab_helpers.cc:230: Profile::FromBrowserContext(web_contents->GetBrowserContext()))); You should totally fix BookmarkModelFactory::GetForProfile to take a BrowserContext* instead so you don't need the FromBrowserContext here. Also, again, how is this not ANDROID_JAVA_UI?
Avi is more familiar with the naming here than I. Avi, do you think they should stick with TabHelper in the name? -Scott On Mon, Aug 1, 2016 at 9:57 AM, <tschumann@chromium.org> wrote: > > https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... > File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right): > > https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... > chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:25: class > BookmarkLastVisitUpdater > On 2016/08/01 15:15:47, sky wrote: >> Generally we name these classes with TabHelper at the end, e.g. >> BookmarkLastVisitUpdateTabHelper or maybe just > BookmarkLastUpdateTabHelper. > > :-) In the initial rounds of the review, we just had the reverse naming > discussion (Jan was suggesting a 'Helper' name). I argued against it as > helpers don't add any value to the name. > It's purpose is to update the Bookmark's last-visit information so why > not just call it after what it does. > > I realize that consistency also adds value, so if you insist we can > change the name (although codesearch shows many naming flavors =)). It's > just that from our code's point of view a 'TabHelper' suffix might > actually be misleading and unnecessarily expose implementation details. > > Thanks for the review! > > --Tim > > https://codereview.chromium.org/2184263005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm taking over this CL, the new CL is here: https://codereview.chromium.org/2200113002/ Marc's comments are addressed in the second patch set (https://codereview.chromium.org/2200113002/#ps20001), sky's comments in the third (https://codereview.chromium.org/2200113002/#ps40001). The "TabHelper" naming question and the OS_ANDROID vs. ANDROID_JAVA_UI question are still open. Please take a look over in the other CL. https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right): https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:14: void BookmarkLastVisitUpdater::CreateForWebContentsWithBookmarkModel( On 2016/08/01 15:15:47, sky wrote: > nit: make order match header. Done. https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/bookmark_last_visit_updater.h (right): https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/bookmark_last_visit_updater.h:25: class BookmarkLastVisitUpdater On 2016/08/01 16:57:26, tschumann wrote: > On 2016/08/01 15:15:47, sky wrote: > > Generally we name these classes with TabHelper at the end, e.g. > > BookmarkLastVisitUpdateTabHelper or maybe just BookmarkLastUpdateTabHelper. > > :-) In the initial rounds of the review, we just had the reverse naming > discussion (Jan was suggesting a 'Helper' name). I argued against it as helpers > don't add any value to the name. > It's purpose is to update the Bookmark's last-visit information so why not just > call it after what it does. > > I realize that consistency also adds value, so if you insist we can change the > name (although codesearch shows many naming flavors =)). It's just that from our > code's point of view a 'TabHelper' suffix might actually be misleading and > unnecessarily expose implementation details. > > Thanks for the review! > > --Tim Acknowledged. Leaving as is for the moment, since Avi hasn't responded yet. https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... chrome/browser/ui/tab_helpers.cc:82: #include "chrome/browser/bookmarks/bookmark_model_factory.h" On 2016/08/01 17:01:41, Avi wrote: > Why is this different than the ANDROID_JAVA_UI block above here and therefore in > need of its own header block? The only documentation I found on these flags is this: https://codesearch.chromium.org/chromium/src/chrome/common/features.gni?q=and... 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. Most things in the header block above seem to be pretty UI specific ("zoom", "banners", and everything from c/b/ui). The AppBannerManager, for instance, is only compiled if ANDROID_JAVA_UI is set (https://codesearch.chromium.org/chromium/src/chrome/chrome_browser.gypi?q=app...). So probably that's the reason why they are in that section. The BookmarkLastVisitUpdater needs to run on all OS_ANDROID and all its dependencies are compiled for OS_ANDROID, not only for ANDROID_JAVA_UI. So if we use ANDROID_JAVA_UI here, we're missing the cases where OS_ANDROID && !ANDROID_JAVA_UI. According to bauerb@, those cases might not exist, because the two are always the same (the gni file linked above also indicates that). If, in the future, ANDROID_JAVA_UI is removed completely, the two header blocks can become one again. Probably the right direction would be to use OS_ANDROID throughout this file, but I don't want to do these changes on this CL. 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. https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... chrome/browser/ui/tab_helpers.cc:230: Profile::FromBrowserContext(web_contents->GetBrowserContext()))); On 2016/08/01 17:01:41, Avi wrote: > You should totally fix BookmarkModelFactory::GetForProfile to take a > BrowserContext* instead so you don't need the FromBrowserContext here. Added BookmarkModelFactory::GetForBrowserContext(content::BrowserContext*) instead, seems to be the usual naming convention. I couldn't find any GetForProfile(content::BrowserContext*) in the codebase. Theoretically, all places that call GetForProfile could now call GetForBrowserContext instead and we could remove GetForProfile, but that would need to go on a separate CL. > Also, again, how is this not ANDROID_JAVA_UI? See above. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right): https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:50: FormatLastVisitDate(base::Time::Now())); On 2016/07/30 14:38:09, Marc Treib wrote: > nit: You could pull the base::Time::Now() out of the loop, to make sure they all > get the exact same time. Done. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:77: std::vector<const BookmarkNode*> bookmarks_for_url; On 2016/07/30 14:38:09, Marc Treib wrote: > Can you declare this variable inside the lambda? Done. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:82: &bookmarks_for_url](const BookmarkModel::URLAndTitle& bookmark) { On 2016/07/30 14:38:09, Marc Treib wrote: > Is this line break correct? Looks super weird Looks fine to me, though now it looks different because bookmarks_for_url is now in the lambda. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:88: for (auto* node : bookmarks_for_url) { On 2016/07/30 14:38:09, Marc Treib wrote: > const auto* ? Done. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:91: max_visit_time = node_visit_time; On 2016/07/30 14:38:09, Marc Treib wrote: > optional nit: > max_visit_time = std::max(max_visit_time, GetLastVisitDateForBookmark(node)); Done. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:93: return (max_visit_time <= min_visit_time); On 2016/07/30 14:38:09, Marc Treib wrote: > nit: no parens required Done. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:99: const BookmarkModel::URLAndTitle& b) { On 2016/07/30 14:38:09, Marc Treib wrote: > Is this the correct indentation? Looks super weird to me... Done. https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h (right): https://codereview.chromium.org/2184263005/diff/40001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:30: // Returns the list of most recently visited bookmarked URLs. For each such URL On 2016/07/30 14:38:09, Marc Treib wrote: > nit: It really returns the bookmarks, not just the bookmarked URLs. Done.
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... chrome/browser/ui/tab_helpers.cc:82: #include "chrome/browser/bookmarks/bookmark_model_factory.h" On 2016/08/02 13:05:33, Philipp Keck wrote: > On 2016/08/01 17:01:41, Avi wrote: > > Why is this different than the ANDROID_JAVA_UI block above here and therefore > in > > need of its own header block? > > The only documentation I found on these flags is this: > https://codesearch.chromium.org/chromium/src/chrome/common/features.gni?q=and... > > 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. > > Most things in the header block above seem to be pretty UI specific ("zoom", > "banners", and everything from c/b/ui). The AppBannerManager, for instance, is > only compiled if ANDROID_JAVA_UI is set > (https://codesearch.chromium.org/chromium/src/chrome/chrome_browser.gypi?q=app...). > So probably that's the reason why they are in that section. > > The BookmarkLastVisitUpdater needs to run on all OS_ANDROID and all its > dependencies are compiled for OS_ANDROID, not only for ANDROID_JAVA_UI. So if we > use ANDROID_JAVA_UI here, we're missing the cases where OS_ANDROID && > !ANDROID_JAVA_UI. > > According to bauerb@, those cases might not exist, because the two are always > the same (the gni file linked above also indicates that). If, in the future, > ANDROID_JAVA_UI is removed completely, the two header blocks can become one > again. Probably the right direction would be to use OS_ANDROID throughout this > file, but I don't want to do these changes on this CL. > > 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. Does this code work with the webui based NTP or just the java based android one? If the latter, then it seems pretty straight forward to follow the conventions of ANDROID_JAVA_UI. You are correct that the current state of the world is somewhat confusing. ANDROID_JAVA_UI was introduced with the hope that we could sever the pretty strong dependencies we have in the code that on Android we are using the java based UI. OS_ANDROID would remain for things that apply on Android regardless of the UI built on top. While this might conceptually fall into the latter, I'd be very strongly tempted to leave it in ANDROID_JAVA_UI in this class for consistency (especially if the logic only applies to the java NTP). I'll try to figure out from sievers@ (the introducer of ANDROID_JAVA_UI) what the goals are so we can avoid this going forward, but I'd vote (for this change) that we err on the side of consistency.
On 2016/08/03 01:11:40, Ted C wrote: > https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... > File chrome/browser/ui/tab_helpers.cc (right): > > https://codereview.chromium.org/2184263005/diff/40001/chrome/browser/ui/tab_h... > chrome/browser/ui/tab_helpers.cc:82: #include > "chrome/browser/bookmarks/bookmark_model_factory.h" > On 2016/08/02 13:05:33, Philipp Keck wrote: > > On 2016/08/01 17:01:41, Avi wrote: > > > Why is this different than the ANDROID_JAVA_UI block above here and > therefore > > in > > > need of its own header block? > > > > The only documentation I found on these flags is this: > > > https://codesearch.chromium.org/chromium/src/chrome/common/features.gni?q=and... > > > > 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. > > > > Most things in the header block above seem to be pretty UI specific ("zoom", > > "banners", and everything from c/b/ui). The AppBannerManager, for instance, is > > only compiled if ANDROID_JAVA_UI is set > > > (https://codesearch.chromium.org/chromium/src/chrome/chrome_browser.gypi?q=app...). > > So probably that's the reason why they are in that section. > > > > The BookmarkLastVisitUpdater needs to run on all OS_ANDROID and all its > > dependencies are compiled for OS_ANDROID, not only for ANDROID_JAVA_UI. So if > we > > use ANDROID_JAVA_UI here, we're missing the cases where OS_ANDROID && > > !ANDROID_JAVA_UI. > > > > According to bauerb@, those cases might not exist, because the two are always > > the same (the gni file linked above also indicates that). If, in the future, > > ANDROID_JAVA_UI is removed completely, the two header blocks can become one > > again. Probably the right direction would be to use OS_ANDROID throughout this > > file, but I don't want to do these changes on this CL. > > > > 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. > > Does this code work with the webui based NTP or just the java based android one? > > If the latter, then it seems pretty straight forward to follow the conventions > of > ANDROID_JAVA_UI. > > You are correct that the current state of the world is somewhat confusing. > ANDROID_JAVA_UI was introduced with the hope that we could sever the pretty > strong dependencies we have in the code that on Android we are using the > java based UI. OS_ANDROID would remain for things that apply on Android > regardless of the UI built on top. > > While this might conceptually fall into the latter, I'd be very strongly tempted > to leave it in ANDROID_JAVA_UI in this class for consistency (especially if > the logic only applies to the java NTP). > > I'll try to figure out from sievers@ (the introducer of ANDROID_JAVA_UI) what > the goals are so we can avoid this going forward, but I'd vote (for this change) > that we err on the side of consistency. Sorry, didn't see this comment. Yes, it is currently only used with the Java based Android UI. The new CL (https://codereview.chromium.org/2200113002/diff/140001/chrome/browser/ui/tab_...) does not change anything about the flags, for consistency as you recommended. |
