Refactoring recently closed tab filtering

Refactoring recently closed tab filtering

The separation between the view and the model in the NTP code for
viewing "recently closed tabs" when displayed in a smallish popup
menu should be more generic and available in the model, not just
in the view. 

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102365

[sky, 2011-09-19 16:23:15]

Also, please write a test for coverage.

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
File chrome/browser/sessions/tab_restore_service.cc (right):

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.cc:311: 
remove one of these lines.

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.cc:325: else
no else

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.cc:336:
TabRestoreService::Entries::const_iterator iter;
put iterator into for loop.

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.h:153: void
FilteredEntries(TabRestoreService::Entries* filteredEntries) const;
If this is only going to be used by RecentlyClosedTabHandler I would rather see
the code there. If that isn't the case pull this into a static method on a class
name like SessionUtil in this directory.

[sky, 2011-09-19 20:58:17]

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.h:153: void
FilteredEntries(TabRestoreService::Entries* filteredEntries) const;
On 2011/09/19 16:23:15, sky wrote:
> If this is only going to be used by RecentlyClosedTabHandler I would rather
see
> the code there. If that isn't the case pull this into a static method on a
class
> name like SessionUtil in this directory.

When you move this to a separate class make the method take a const Entries&.
That'll be easier to write tests for.

[noyau (Ping after 24h), 2011-09-20 15:46:58]

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
File chrome/browser/sessions/tab_restore_service.cc (right):

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.cc:311: 
On 2011/09/19 16:23:15, sky wrote:
> remove one of these lines.
Done in session_utils.cc

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.cc:325: else
On 2011/09/19 16:23:15, sky wrote:
> no else

Done in session_utils.cc

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.cc:336:
TabRestoreService::Entries::const_iterator iter;
On 2011/09/19 16:23:15, sky wrote:
> put iterator into for loop.

Done in session_utils.cc

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/7931027/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service.h:153: void
FilteredEntries(TabRestoreService::Entries* filteredEntries) const;
On 2011/09/19 16:23:15, sky wrote:
> If this is only going to be used by RecentlyClosedTabHandler I would rather
see
> the code there. If that isn't the case pull this into a static method on a
class
> name like SessionUtil in this directory.

This will be used in some other context. SessionUtils created.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/tab_...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/tab_...
chrome/browser/sessions/tab_restore_service.h:177: Entries entries_;
I had to move the entries to be protected in order to be able to construct a
mock for testing purpose.

[sky, 2011-09-20 16:24:46]

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils.cc (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.cc:12: // This class is used to compare
two entries and considers that they are
newline between 11 and 12.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.cc:15: class EntryComparator {
this should not be indented.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils.h (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:18: static void
FilteredEntries(TabRestoreService* service,
Don't make this depend upon TabRestoreService. Instead either make filtered_tabs
be initialized to services->entries() or make this take two Entries, one const&
and the other a pointer.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:19: TabRestoreService::Entries*
filteredTabs);
filtered_tabs. But filtered_entries is a better name since the type is entries.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:20: };
add a private field and DISALLOW_IMPLICIT_CONSTRUCTORS.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils_unittest.cc (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:5: #include
"testing/gtest/include/gtest/gtest.h"
test files have a similar include layout to that of a .cc file. That is, include
session_utils.h first, newline, then non-system includes.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:16: : TabNavigation(0,      
// index
indent this by 4.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:25: class
TabRestoreServiceMock : public TabRestoreService {
When you change FilteredEntries to not take a TabRestoreService then you won't
need this class. Instead you'll be able to pass in the entries directly. This is
one of the reasons I didn't want the function taking a TabRestoreService.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:59: 
remove one of these.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:76: TabRestoreService::Entries
filteredEntries;
filtered_entries.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:89: 
remove these newlines.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/tab_...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/tab_...
chrome/browser/sessions/tab_restore_service.h:177: Entries entries_;
Use entries() instead of making this field protected.

[noyau (Ping after 24h), 2011-09-20 17:09:35]

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils.cc (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.cc:12: // This class is used to compare
two entries and considers that they are
On 2011/09/20 16:24:46, sky wrote:
> newline between 11 and 12.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.cc:15: class EntryComparator {
On 2011/09/20 16:24:46, sky wrote:
> this should not be indented.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils.h (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:18: static void
FilteredEntries(TabRestoreService* service,
On 2011/09/20 16:24:46, sky wrote:
> Don't make this depend upon TabRestoreService. Instead either make
filtered_tabs
> be initialized to services->entries() or make this take two Entries, one
const&
> and the other a pointer.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:19: TabRestoreService::Entries*
filteredTabs);
On 2011/09/20 16:24:46, sky wrote:
> filtered_tabs. But filtered_entries is a better name since the type is
entries.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:20: };
On 2011/09/20 16:24:46, sky wrote:
> add a private field and DISALLOW_IMPLICIT_CONSTRUCTORS.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils_unittest.cc (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:5: #include
"testing/gtest/include/gtest/gtest.h"
On 2011/09/20 16:24:46, sky wrote:
> test files have a similar include layout to that of a .cc file. That is,
include
> session_utils.h first, newline, then non-system includes.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:16: : TabNavigation(0,      
// index
On 2011/09/20 16:24:46, sky wrote:
> indent this by 4.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:25: class
TabRestoreServiceMock : public TabRestoreService {
On 2011/09/20 16:24:46, sky wrote:
> When you change FilteredEntries to not take a TabRestoreService then you won't
> need this class. Instead you'll be able to pass in the entries directly. This
is
> one of the reasons I didn't want the function taking a TabRestoreService.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:59: 
On 2011/09/20 16:24:46, sky wrote:
> remove one of these.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:76: TabRestoreService::Entries
filteredEntries;
On 2011/09/20 16:24:46, sky wrote:
> filtered_entries.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:89: 
On 2011/09/20 16:24:46, sky wrote:
> remove these newlines.

Done.

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/tab_...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/7931027/diff/6001/chrome/browser/sessions/tab_...
chrome/browser/sessions/tab_restore_service.h:177: Entries entries_;
On 2011/09/20 16:24:46, sky wrote:
> Use entries() instead of making this field protected.
Reverted as this class is no longer mocked.

[sky, 2011-09-20 17:21:30]

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils.h (right):

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:13: // Fill the passed list with the
entries() from the TabRestoreService
Update your comment. Also, document that TabRestoreService owns the objects in
the returned list.

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils_unittest.cc (right):

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:56: entries_.empty();
. empty is better thought of IsEmpty, meaning it doesn't remove anything from
the list.
. This leaks the TabMocks.
. Why do you need a TabMock? You can create a Tab directly and set whatever you
need to.

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:75: };
no semicolon.

http://codereview.chromium.org/7931027/diff/8009/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7931027/diff/8009/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:2011: 'browser/sessions/session_utils.h',
sort

[noyau (Ping after 24h), 2011-09-20 21:53:31]

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils.h (right):

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:13: // Fill the passed list with the
entries() from the TabRestoreService
On 2011/09/20 17:21:30, sky wrote:
> Update your comment. Also, document that TabRestoreService owns the objects in
> the returned list.

Done.

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils_unittest.cc (right):

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:56: entries_.empty();
On 2011/09/20 17:21:30, sky wrote:
> . empty is better thought of IsEmpty, meaning it doesn't remove anything from
> the list.
> . This leaks the TabMocks.
Ooops! A clear case of typing before thinking.

> . Why do you need a TabMock? You can create a Tab directly and set whatever
you
> need to.
I prefer it this way. In order to keep the fixture readable removing the TabMock
would means creating a small utility function to do the same thing. If you feel
strongly about it I'll change it.

http://codereview.chromium.org/7931027/diff/8009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:75: };
On 2011/09/20 17:21:30, sky wrote:
> no semicolon.

Done.

http://codereview.chromium.org/7931027/diff/8009/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7931027/diff/8009/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:2011: 'browser/sessions/session_utils.h',
On 2011/09/20 17:21:30, sky wrote:
> sort

Done.

[sky, 2011-09-20 22:10:28]

LGTM

http://codereview.chromium.org/7931027/diff/7009/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils.h (right):

http://codereview.chromium.org/7931027/diff/7009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:15: // the list with things that look
like duplicates. A user can still access the
This last sentence doesn't really make sense here.

http://codereview.chromium.org/7931027/diff/7009/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils_unittest.cc (right):

http://codereview.chromium.org/7931027/diff/7009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:56:
std::list<TabRestoreService::Entry*>::iterator iter = entries_.begin();
STLDeleteElements

[noyau (Ping after 24h), 2011-09-20 22:43:58]

http://codereview.chromium.org/7931027/diff/7009/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils.h (right):

http://codereview.chromium.org/7931027/diff/7009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils.h:15: // the list with things that look
like duplicates. A user can still access the
On 2011/09/20 22:10:28, sky wrote:
> This last sentence doesn't really make sense here.
True. It is more related to the user experience, and has nothing to do in the
model. Removed.

http://codereview.chromium.org/7931027/diff/7009/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_utils_unittest.cc (right):

http://codereview.chromium.org/7931027/diff/7009/chrome/browser/sessions/sess...
chrome/browser/sessions/session_utils_unittest.cc:56:
std::list<TabRestoreService::Entry*>::iterator iter = entries_.begin();
On 2011/09/20 22:10:28, sky wrote:
> STLDeleteElements
Didn't even knew this existed. Thanks.

[commit-bot: I haz the power, 2011-09-22 18:12:21]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/noyau%40chromium.org/7931027/11005

[commit-bot: I haz the power, 2011-09-22 21:12:31]

Change committed as 102365